Repository navigation
fix(windows): use matching deallocator for CRT aligned blocks - #2281
Conversation
Signed-off-by: News <[email protected]>
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you for chasing this after #2260. A fault inside _aligned_free is the kind of crash that is hard to pin down, and your explanation of the deallocator mismatch is clear.
I tried to reproduce it on our Windows ARM64 VM (MSYS2 CLANGARM64, lld) before merging, and found something I'd like to check with you:
- A production build of main (
CBM_MEM_GLOBAL_OVERRIDE=1) indexed a 1,572-file C/C++ tree with parallel workers without a fault. - In that binary,
operator delete(void*, std::align_val_t)loads its target from__imp___wrap__aligned_free. That pointer leads to our__wrap__aligned_free, not UCRT's_aligned_free. The other five aligned delete entry points tail-call into it. So on this toolchain lld already rewrites the__imp__aligned_freereference, which is the reference the PR says--wrapmisses.
That makes me think the difference is in the toolchain. Could you share:
- the toolchain and linker you built with (LLVM-MinGW release or MSYS2 CLANG64, and its version, x64 I assume),
- the crash stack, or at least the frame that called
_aligned_free, - if you can, a public C++ project that reproduces it.
The first hunk, which sends a CRT-owned aligned block to __real__aligned_free instead of __real_free, is correct on its own: _aligned_free is the matching deallocator. We are happy to take that part as it is. For the six operator delete definitions, we'd like to know which toolchain needs them. If the release toolchain doesn't need them, they would silently replace libc++abi's own definitions for no benefit. A comment naming the toolchain that does need them would also help the next reader.
Thank you again for the careful work on the Windows allocator path.
Signed-off-by: News <[email protected]>
knewstimek
changed the title
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @knewstimek, for coming back with real data rather than just trimming the diff. Your x64 check (LLVM-MinGW 20260908 / lld 23.1.1) shows the aligned operator delete import already going to __imp___wrap__aligned_free. That's exactly the x64 check we couldn't run ourselves. Together with our arm64 finding, it settles that the operator delete definitions aren't needed, and dropping them keeps libc++abi's own definitions in place. We appreciate the careful reasoning in the updated description.
The remaining change is correct as it stands:
_aligned_freeis in the wrapped symbol set, so__real__aligned_freeresolves to UCRT's_aligned_freeon both architectures.- Mimalloc blocks are still caught first by
mi_is_in_heap_regionand go tomi_free, unchanged. - Only a block mimalloc didn't make, which means a real CRT
_aligned_mallocblock, reaches the new line. For that block_aligned_freeis the matching deallocator, and plainfreeon the offset pointer was the bug.
Approved. On our side we'll add a small Windows-only regression test before merging: allocate through the CRT's _aligned_malloc, check it isn't a mimalloc block, free it through the wrapper, and validate the heap. We'll run it on our Windows VM, where it should fail on main and pass here. That way the fix stays pinned. You'll be credited on it, of course.
One question out of curiosity, not blocking: you mention a retained production binary with a direct CRT import. If that was one of our release archives, could you tell us which version and asset? We'd like to understand how that import survived the link. Thank you again for the careful work on the Windows allocator path!
|
Thank you for the review and for adding the regression test. To clarify the retained binary's provenance: it was a local source build, not an upstream release archive, so there is no upstream release version or asset to identify. I haven't recovered the exact original link command. |
|
Thank you, @knewstimek, for tracking that down and saying so plainly. Knowing it was a local source build rather than one of our release archives tells us our release link isn't leaving a direct CRT import behind, which is what we wanted to rule out. There's no need to recover the old link command. We'll add the Windows-only regression test we mentioned (a CRT |
DeusData
added
bug
|
Thanks again for the allocator fix and the provenance clarification. The existing approval stands in the thread; the promised regression and Windows verification are still maintainer-side steps. There is no need to recover the old link or make another change for this sweep. |
…apper __wrap__aligned_free hands a block mimalloc did not make back to the CRT. A block from the CRT's _aligned_malloc starts past the CRT's bookkeeping header, so its matching deallocator is _aligned_free; plain free receives an interior pointer. This branch routes such blocks to __real__aligned_free. The new Windows-only mem test allocates through __real__aligned_malloc (the CRT's own allocator under --wrap), asserts mimalloc does not own the block, frees it through the wrapper by name, and checks errno and HeapValidate on the process heap. Freeing the interior pointer with plain free is a bad-free report under ASan; on an uninstrumented heap the free fails with errno set or the process terminates. Signed-off-by: Martin Vogel <[email protected]>
|
Thank you again, @knewstimek. As promised, we have added the Windows-only regression test for your allocator fix, as one new commit on top of your branch. Your two commits are untouched.
The We will merge once CI is green on the new head. Thank you for the careful work, and for coming back with the x64 link data that settled the |
Signed-off-by: Martin Vogel <[email protected]>
|
Thank you for the Windows aligned-allocation fix and its regression coverage! I merged current main into your branch in 67695d4 to bring in the shared CI repairs and restart the required checks. Your two-file fix is unchanged byte-for-byte, and all existing contributor commits and sign-offs are preserved. The earlier runs stopped at the shared language-count precheck; the refreshed run can now validate the fix against current main. |
|
Thank you again for the Windows aligned-allocation fix. The October updates record the regression and baseline refresh as completed on our side; no repeat contributor work is requested. Sorry for the delay in the remaining maintainer follow-up—I’ll follow up soon. |
|
Merged - thank you, @knewstimek! Your fix makes the aligned-free wrapper hand a block the CRT allocated back to _aligned_free, its matching deallocator, instead of plain free with a pointer that sits past the CRT's bookkeeping header. A fault deep inside the allocator is one of the hardest crashes to trace from the outside, and you narrowed it to exactly the right line. Thank you as well for coming back with the x64 LLVM-MinGW link data: it settled that the operator delete overrides were not needed, which kept this to one precise fix and left the runtime's own definitions in place. The Windows regression test we added on top ran on this PR's Windows CI leg and keeps the route pinned from now on. Thanks again for the careful work on the Windows allocator path, and sorry it took us a while to land. |
CRT-owned aligned allocations carry a bookkeeping header and must be released with
_aligned_free. The owner-aware aligned wrapper currently sends foreign aligned pointers to plainfree; route them to__real__aligned_freeinstead.This PR now contains only that matching-deallocator fix. The six aligned C++ delete overrides have been removed: a fresh x64 LLVM-MinGW 20260908 / clang and lld 23.1.1 link inspection, including the production allocator wrappers, redirects the C++ import pointer to
__imp___wrap__aligned_freewithout those definitions. A retained production binary has a direct CRT import, but the original caller and complete link provenance are not established, so it does not justify adding those overrides here.Validation: a Windows x64 production-allocator smoke releases both a CRT-owned aligned allocation and an aligned C++ allocation successfully.