workaround: MinGW thread_local use-after-free in ThreadContext - #318
workaround: MinGW thread_local use-after-free in ThreadContext#318ryanofsky wants to merge 1 commit into
Conversation
MinGW has a bug where thread_local destructors can run on already-freed memory at thread exit, causing heap corruption. MSVC builds and non-Windows platforms are unaffected. This caused intermittent STATUS_HEAP_CORRUPTION in MinGW CI builds when ~ThreadContext walked freed memory at thread exit. See GThreadContext() in proxy-io.h for the full explanation and upstream bug references. Workaround: replace g_thread_context with a GThreadContext() accessor that on MinGW returns a heap-allocated object held by a trivially-destructible pointer, skipping destructor registration at thread exit. The object is deliberately leaked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste |
Use new CurrentThread function to be compatible with windows mingw bug workaround bitcoin-core/libmultiprocess#318
|
Updated 1243dd3 -> 1500701 ( |
|
In commit 1500701 "workaround: MinGW thread_local use-after-free in ThreadContext" nit: The commit body still mentions |
|
Code review ACK 1500701 Compiled and ran tests on macOS. Tried to reproduce the heap corruption with a My understanding: MinGW's machinery doesn't guarantee a safe cleanup order for The cost of this leakage is that exiting client threads won't release the Other target builds remain unaffected. The documentation is well-written, and it's clear to any developer why this workaround exists. This PR looks good to me. |
| //! This was observed in Bitcoin Core Windows CI as intermittent | ||
| //! STATUS_HEAP_CORRUPTION (0xC0000374) exit code 3221226356 crashes of both |
There was a problem hiding this comment.
Is this specific to MSVCRT, or does it occur with UCRT as well?
|
Wrapping My agent also couldn't reproduce the original issue on Linux with Wine, initially not on Windows 11 either, but I'll keep trying. I didn't verify the Windows-specific rationale, but the change itself seems simple enough is well documented. |
|
ACK 1500701 It figured it out: https://gist.github.com/Sjors/4b70c60a5d2989bf2ba4adb408d2554c |
|
Seems fine, but given that this was already fixed in mingw-w64/mingw-w64@8e06daa / 13.0.0 , I wonder if it would be easier to just build that version (or later). This is with the background that the Ubuntu/Debian packages are already broken and possibly unmaintained, so we may want to consider self-building anyway? Ref: bitcoin/bitcoin#33593 (comment) Either self-building directly, or using something like https://github.com/0xB10C/bitcoind-gunix/blob/v31.1/nix/win64/toolchain.nix ? Edit: mingw in guix was bumped to v13 in bitcoin/bitcoin@31eb46f |
Work around mingw bug
thread_localbug https://sourceforge.net/p/mingw-w64/bugs/527/ that causes failures in windows "test cross-built" CI jobs in bitcoin/bitcoin#32387 by skippingthread_localdestructors in mingw.The workaround will result in resource leaks in mingw builds, that might be noticeable if a lot of threads are created and destroyed, but should not be a significant problem in practice.
This change is just a refactoring in other builds (including MSVC) not affected by this bug.