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
|
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 LLM Linter (✨ experimental)Possible typos and grammar issues:
2026-07-30 16:26:42 |
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?
There was a problem hiding this comment.
re: #318 (comment)
Is this specific to MSVCRT, or does it occur with UCRT as well?
I don't think it has anything do to with the C runtime, although agent noted it did not see this error under UCRT builds for some reason (see "since the underlying bug is CRT-independent" comment below)
This error is pretty easy to reproduce by disabling the #ifdef __MINGW32__ workaround in bitcoin/bitcoin#32387 and seeing the cross-compiled CI job in interface_ipc_cli.py and interface_ipc_mining.py tests. You can see the ci errors looking at win-work branch pushes from Jul 22 in `https://github.com/ryanofsky/bitcoin/actions like https://github.com/ryanofsky/bitcoin/actions/runs/29931590196/job/88964021272
|
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 |
…i: switch Bitcoin Core to master 66298c7 ci: Switch back to Bitcoin Core's master branch (Hennadii Stepanov) 86b4810 refactor: access ThreadContext through CurrentThread() (Sjors Provoost) Pull request description: This is a minimal subset from #318 needed to match the shim introduced by bitcoin/bitcoin#35084. Should unblock #322, see #322 (comment). Include #322 here, because CI will not pass now since bitcoin/bitcoin#35084 merged. ACKs for top commit: xyzconstant: ACK 66298c7 hebasto: ACK 66298c7. ViniciusCestarii: ACK 66298c7 ryanofsky: Code review ACK 66298c7. This nicely separates the `CurrentThread` refactoring from the mingw bug workaround. But it's misleading to call this a minimal fix for compatibility, because a minimal fix would just be: Tree-SHA512: 04cdb9f72c01279a7d9d8bd5735be5114dc6c37a5c66c5cab3e199d48c42da30d2db4c0bcd4f3bcf5508a04a52c78d63945b7ea9b605624353478872d38c5ebe
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 CurrentThread() in proxy-io.h for the full explanation and upstream bug references. Workaround: change CurrentThread() accessor in MinGW builds to return 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>
ryanofsky
left a comment
There was a problem hiding this comment.
re: #318 (comment)
mingw in guix was bumped to v13 in bitcoin/bitcoin@31eb46f
Thanks for these details. Would have to look into this more but would be nice if this is already fixed in guix builds and the workaround is unnecessary there.
I think we still might want this code change (maybe narrowed to depend on mingw version or whether the fix is present) if it is easier than self-building mingw in CI. A motivation for this change is getting
interface_ipc_cli.py and interface_ipc_mining.py tests to pass in bitcoin/bitcoin#32387 cross-compiled CI jobs
Rebased 1500701 -> 608d09a (pr/win-tls.2 -> pr/win-tls.3, compare) due to silent conflicts with #323
| //! 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.
re: #318 (comment)
Is this specific to MSVCRT, or does it occur with UCRT as well?
I don't think it has anything do to with the C runtime, although agent noted it did not see this error under UCRT builds for some reason (see "since the underlying bug is CRT-independent" comment below)
This error is pretty easy to reproduce by disabling the #ifdef __MINGW32__ workaround in bitcoin/bitcoin#32387 and seeing the cross-compiled CI job in interface_ipc_cli.py and interface_ipc_mining.py tests. You can see the ci errors looking at win-work branch pushes from Jul 22 in `https://github.com/ryanofsky/bitcoin/actions like https://github.com/ryanofsky/bitcoin/actions/runs/29931590196/job/88964021272
It is just a guess that this is already fixed, since I don't have Windows to test. One could check by using The general idea is that we are having problems cross-compiling with the apt system packages, so using guix or nix instead could help with that, and possibly also help with making the CI closer to the release bins, and possibly even align the cross-compile docs to produce bins identical to the ones from contrib/guix. So trying to use a mingw from nix (possibly in combination with a apt system gcc, or so) would be interesting and could possibly fix a bunch of issues (or introduce more issues, heh). |
Use new CurrentThread function to be compatible with windows mingw bug workaround bitcoin-core/libmultiprocess#318
If you want, you can merge bitcoin/bitcoin#35877 (mingw 13) into 32387, to see if it passes CI. |
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.