Skip to content

workaround: MinGW thread_local use-after-free in ThreadContext - #318

Open
ryanofsky wants to merge 1 commit into
bitcoin-core:masterfrom
ryanofsky:pr/win-tls
Open

workaround: MinGW thread_local use-after-free in ThreadContext#318
ryanofsky wants to merge 1 commit into
bitcoin-core:masterfrom
ryanofsky:pr/win-tls

Conversation

@ryanofsky

Copy link
Copy Markdown
Collaborator

Work around mingw bug thread_local bug https://sourceforge.net/p/mingw-w64/bugs/527/ that causes failures in windows "test cross-built" CI jobs in bitcoin/bitcoin#32387 by skipping thread_local destructors 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.

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>
@DrahtBot

DrahtBot commented Jul 22, 2026

Copy link
Copy Markdown

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers
ACK xyzconstant, Sjors

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jul 23, 2026
Use new CurrentThread function to be compatible with windows mingw bug
workaround bitcoin-core/libmultiprocess#318
@ryanofsky

Copy link
Copy Markdown
Collaborator Author

Updated 1243dd3 -> 1500701 (pr/win-tls.1 -> pr/win-tls.2, compare) renaming GThreadContext to CurrentThread

@xyzconstant

Copy link
Copy Markdown
Contributor

In commit 1500701 "workaround: MinGW thread_local use-after-free in ThreadContext"

nit: The commit body still mentions GThreadContext() instead of CurrentThread

@xyzconstant

Copy link
Copy Markdown
Contributor

Code review ACK 1500701

Compiled and ran tests on macOS. Tried to reproduce the heap corruption with a mingw-w64 cross-build run under Wine following some of the instructions in the bug report links but didn't have any success (maybe luck?).

My understanding: MinGW's machinery doesn't guarantee a safe cleanup order for thread_local variables. The idea is to skip the __cxa_thread_atexit call entirely (which registers ~ThreadContext, a non-trivial destructor to run at thread exit) so its scheduled destructor doesn't run on already-freed memory and thereby avoid heap corruption. The changes in this PR implement that by modifying the g_thread_context variable (now returned by CurrentThread()) from a value of type ThreadContext to a pointer of type ThreadContext * on MinGW. This results in only the emutls cleanup running. This frees the pointer's storage at destruction time, and the context object is, as a result, leaked.

The cost of this leakage is that exiting client threads won't release the Thread capability, which means the server threads aren't closed until the connection is closed (closing the connection destroys those OS threads).

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.

Comment thread include/mp/proxy-io.h
Comment on lines +978 to +979
//! This was observed in Bitcoin Core Windows CI as intermittent
//! STATUS_HEAP_CORRUPTION (0xC0000374) exit code 3221226356 crashes of both

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this specific to MSVCRT, or does it occur with UCRT as well?

@Sjors

Sjors commented Jul 28, 2026

Copy link
Copy Markdown
Member

Wrapping g_thread_context in CurrentThread(), and then dropping it entirely, is nice in general. Maybe make it a prep refactor commit(s), so the windows change is more focussed. See sjors/2026/07/mingw.

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.

@Sjors

Sjors commented Jul 28, 2026

Copy link
Copy Markdown
Member

@maflcko

maflcko commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants