fix(core): declare SetThreadDescription/GetThreadDescription for MinGW - #5375
fix(core): declare SetThreadDescription/GetThreadDescription for MinGW#5375NF0T wants to merge 1 commit into
Conversation
The mingw-w64 header snapshot bundled with Qt's mingw1310_64 toolchain does not declare SetThreadDescription or GetThreadDescription in <processthreadsapi.h>, even though kernel32.dll exports both (Windows 10 1607+) and MSVC's Windows SDK already declares them. Widening _WIN32_WINNT/WINVER does not help — the prototypes are absent outright, confirmed with an isolated repro against this toolchain. Introduced by bc34585 (aethersdr#5246, System Info dialog Threads/Logs tabs), which added the first calls to these APIs in this codebase. It builds cleanly under MSVC because that SDK already has the declarations, so a MinGW build is the only way to catch this. Declare both functions manually, dllimport from kernel32 (already linked in all three files), guarded to __MINGW32__ so MSVC and non-Windows builds are untouched. Verified the declared signatures against the real Windows SDK header actually shipped on this machine.
There was a problem hiding this comment.
Issue fit
Yes, with one claim I could not confirm. #5374 reports three MinGW compile errors — AsyncLogWriter.cpp:350, ThreadName.cpp:53 (both SetThreadDescription) and SystemInfo.cpp:240 (GetThreadDescription). I grepped the head checkout for ThreadDescription: those are the only three call sites in the tree, and this diff guards all three. The declarations sit after #include <windows.h> in each file, so HANDLE/PCWSTR/PWSTR/HRESULT/WINAPI are all defined at that point — no ordering trap. Signatures match the SDK prototypes (HRESULT WINAPI SetThreadDescription(HANDLE, PCWSTR) / GetThreadDescription(HANDLE, PWSTR*)), including the PWSTR* out-parameter that SystemInfo.cpp:247 LocalFrees. The shape mirrors the #622 precedent still living at RadioConnection.cpp:21.
The unconfirmed part is whether it links — see nit 2.
Scope
| File | What it changes | Claimed by title/body? | Verdict |
|---|---|---|---|
src/core/AsyncLogWriter.cpp |
MinGW-guarded SetThreadDescription decl |
Yes | In scope |
src/core/SystemInfo.cpp |
MinGW-guarded GetThreadDescription decl |
Yes | In scope |
src/core/ThreadName.cpp |
MinGW-guarded SetThreadDescription decl |
Yes | In scope |
Everything in the diff is explained by the issue. Purely additive — no - lines, so no guard, early return or symptom-naming comment was removed. No new public surface, no settings key, no default change, no UI touched, no CHANGELOG.md entry (correct). Nothing to unbundle.
Blockers
None.
Nits (non-blocking)
- The
__MINGW32__guard is unconditional, so it also fires on kits that already declare these. Newer mingw-w64 snapshots do carry both prototypes in<processthreadsapi.h>. I looked at whether that second declaration is harmful: mingw-w64 declares them asWINBASEAPI HRESULT WINAPI …, andWINBASEAPIexpands toDECLSPEC_IMPORT→__declspec(dllimport)outside a kernel32 build, so the redeclaration should be attribute-identical to yours and benign. I'm stating that as reasoning from how those headers are written, not as something I compiled — I have no MinGW toolchain here and no CI lane exercises one. A__MINGW64_VERSION_MAJOR/_MINORversion test would take the question off the table, at the cost of needing a version to pin. Your call; I would not hold the PR for it. - The load-bearing claim isn't evidenced. The commit message says the signatures were "verified against the real Windows SDK header actually shipped on this machine" — that establishes the prototype is right, not that the build now completes.
__declspec(dllimport)emits a reference to__imp_SetThreadDescription, which has to exist in that kit'slibkernel32.a; "kernel32.dll exports it" is the runtime authority, not the link-time one. If the header snapshot is old enough to omit the prototype, the import lib is at least worth a glance. One line in the body — "cmake --build buildcompletes and links on mingw1310_64" — settles it entirely, and I suspect it already did on your machine. - The same six lines are duplicated three times.
AsyncLogWriter.cpp:14-19explains why that file deliberately does not route throughThreadName.cpp(51 test targets vs 6), so a new.cppis genuinely off the table — but a header-only helper would keep the workaround in one place without undoing that. Worth considering if a fourth call site ever appears; not worth churning this PR. - No CI lane covers this.
ci.yml'scheck-windowsandwindows-installer.ymlare bothwindows-latest+msvc-dev-cmd(win64_msvc2022_64), which is exactly why #5246 landed green. I am explicitly not asking for a test: a MinGW-only missing-prototype gap has no socket-free CTest seam under the AGENTS test-layer boundary, and the only real coverage would be a MinGW build job — a maintainer scope call, not a merge gate on this fix.
What I tried to break
- Missed sibling call sites. Grepped
ThreadDescriptionacross the whole head checkout: 3 call sites, 3 guards, plus two comment-only mentions (SystemInfo.h:76,SystemInfo.cpp:243). Nothing left unfixed — the PR does not read as "fixed" while a fourth surface still breaks. - Declaration placed before the types it needs. Checked each file: all three sit after
<windows.h>(and after<tlhelp32.h>inSystemInfo.cpp), inside the existing_WIN32/Q_OS_WINarm, before the#eliffor Apple/Linux. Non-Windows and MSVC preprocess to byte-identical output. - Wrong signature, especially the out-param.
GetThreadDescriptiontakesPWSTR*(notPWSTR), and the caller atSystemInfo.cpp:247passes&descriptionandLocalFrees it — consistent.SetThreadDescriptiontakesPCWSTR; both callers pass aconst-compatible wide string.WINAPI(__stdcall) is present, so no calling-convention or x86 name-decoration mismatch. - A stealth behavior change hiding in a build fix. There are no
-lines at all and no runtime code is touched, so the MSVC binary is unchanged. Nothing here is a preference change. - CI status. All four checks green on
e7ddfb0(build,check-windows,check-macos,Static checks) — but none of them is a MinGW build, so green here is evidence the fix broke nothing, not evidence it works.
Everything above is reasoned from the diff and the head checkout; I ran no build and executed no tests, and I have no MinGW toolchain available.
Recommendation
Approve with nits. This is a correctly-scoped, minimal, precedent-following build fix that covers every affected call site and cannot affect MSVC or non-Windows output. Concrete next step: add one line to the PR body confirming the MinGW build actually links (nit 2), and say a word on whether you want the guard narrowed to a __MINGW64_VERSION_MAJOR test (nit 1) or left broad. Neither needs new code from my side of the review.
Thanks for chasing this down with an isolated repro and filing the issue separately — the root-cause attribution to bc345850 made this quick to verify.
🤖 aethersdr-agent · cost: $5.3989 · model: claude-opus-5
| #define NOMINMAX | ||
| #endif | ||
| #include <windows.h> | ||
| #if defined(__MINGW32__) |
There was a problem hiding this comment.
This guard fires on every mingw-w64 kit, including newer snapshots that already declare SetThreadDescription in <processthreadsapi.h> — so on those the compiler sees two declarations.
I checked whether that's harmful rather than assuming it: mingw-w64 declares it as WINBASEAPI HRESULT WINAPI SetThreadDescription(HANDLE, PCWSTR), and WINBASEAPI expands to DECLSPEC_IMPORT → __declspec(dllimport) outside a kernel32 build, which makes it attribute-identical to yours. So this most likely compiles clean on newer kits too. That's reasoning from how those headers are written, though — I have no MinGW toolchain and no CI lane exercises one, so it's unverified either way.
Narrowing to #if defined(__MINGW32__) && __MINGW64_VERSION_MAJOR < N would remove the question entirely (at the cost of pinning N). Non-blocking — your call.
The same guard appears verbatim at SystemInfo.cpp:18 and ThreadName.cpp:11; whatever you decide should apply to all three.
| // though kernel32.dll exports it (Windows 10 1607+, MSVC's SDK already has | ||
| // it). Widening _WIN32_WINNT doesn't help — the prototype is absent outright. | ||
| extern "C" __declspec(dllimport) HRESULT WINAPI | ||
| SetThreadDescription(HANDLE hThread, PCWSTR lpThreadDescription); |
There was a problem hiding this comment.
Small precision on the comment above, and the one claim I couldn't verify: "kernel32.dll exports it" is the runtime authority, not the link-time one.
__declspec(dllimport) makes the compiler emit a reference to __imp_SetThreadDescription, which the linker resolves out of that kit's libkernel32.a — not out of the DLL. A header snapshot old enough to omit the prototype is at least worth checking for the matching import stub, otherwise the build moves from a compile error to undefined reference rather than succeeding.
The commit message evidences the signature ("verified against the real Windows SDK header actually shipped on this machine") but not the build. One line in the PR body — cmake --build build completes and links on mingw1310_64 — would settle this completely, and I'd guess you already have that result in hand.
| #if defined(__MINGW32__) | ||
| // This mingw-w64 header snapshot doesn't declare SetThreadDescription even | ||
| // though kernel32.dll exports it (Windows 10 1607+, MSVC's SDK already has | ||
| // it). Widening _WIN32_WINNT doesn't help — the prototype is absent outright. | ||
| extern "C" __declspec(dllimport) HRESULT WINAPI | ||
| SetThreadDescription(HANDLE hThread, PCWSTR lpThreadDescription); | ||
| #endif |
There was a problem hiding this comment.
Third verbatim copy of this block (also AsyncLogWriter.cpp:25-31, SystemInfo.cpp:18-24).
Worth naming, not worth churning the PR for: AsyncLogWriter.cpp:14-19 documents why that file must not depend on ThreadName.cpp (51 test targets vs 6), so a shared .cpp is genuinely ruled out — but a header-only WindowsThreadDescription.h would satisfy that constraint and keep the workaround in one place if a fourth call site ever shows up. Fine to leave as-is at three.
|
Antares Detector — candidate vulnerable file(s)
The AsyncLogWriter uses GetThreadDescription (Windows kernel32.dll export) and platform-specific implementations that may call into untrusted thread descriptions. The mingw-w64 header snapshot declares SetThreadDescription but doesn't provide an implementation, so a MinGW/mingw build could rely on missing or unsafe fallback code. This can lead to undefined behavior if the implementation uses unsafe memory operations (e.g., memcpy) or fails to validate thread description strings before passing them to kernel APIs. Localized by Cisco Foundation AI Antares-1B running locally in the AetherClaude sandbox, seeded by the Cartographer security map. Advisory only — please verify before acting. 🤖 aethersdr-agent · cost: $5.5304 · model: claude-opus-5 |
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
#5374: MinGW-w64's header snapshot doesn't declare SetThreadDescription/GetThreadDescription even though kernel32 exports them, so #5246's System Info work fails to compile there. The fix declares both manually behind #if defined(__MINGW32__). Verified: the prototypes match the real SDK exactly (SetThreadDescription(HANDLE, PCWSTR), GetThreadDescription(HANDLE, PWSTR*)), the guard is a strict no-op on MSVC/macOS/Linux, and all three call sites (AsyncLogWriter, ThreadName, SystemInfo) are covered. Same shape as the project's own #622 (TCP_INFO_v0) MinGW precedent. Clear-root-cause build fix, RFC-exempt.
Scope
Three files, on-issue. CHANGELOG.md untouched. Preflight: no sockets.
Blockers
None.
Nits (non-blocking)
- The duplication rationale cites the
AsyncLogWriterprecedent, but that precedent is about not sharing a header that pulls in a linkable source (the 51-vs-6 test-target concern). A pure prototype header has no source to link, so the concern doesn't transfer — a one-lineThreadDescriptionShims.hwould carry this identically without the three-way copy. Minor; the copy is harmless. - Forward-compat note: if a future mingw-w64 snapshot adds these declarations, the guard produces a redeclaration — benign, since it's an identical
extern "C" __declspec(dllimport)prototype, but worth a#if !defined(...)-style comment so a maintainer knows the copy can be dropped once the toolchain catches up.
What was verified vs read
- Verified: prototype signatures against the SDK shape; guard is
__MINGW32__-only; all three call sites present. - Not re-run: the MSVC no-op claim (the body honestly marks it unconfirmed) — the guard makes it a preprocessor no-op, so low risk, but CI's
check-windowsis MSVC and is green.
Recommendation: approve with nits — mechanical, correct, precedent-backed.
Summary
Fixes #5374. A MinGW-w64 build fails to compile after #5246 (System Info dialog) because this mingw-w64 header snapshot doesn't declare
SetThreadDescription/GetThreadDescriptionin<processthreadsapi.h>, even thoughkernel32.dllexports both (Windows 10 1607+) and MSVC's SDK already has them — see the issue for the repro, the exact compile error, and why widening_WIN32_WINNTdoesn't help.extern "C" __declspec(dllimport)), matching the real Windows SDK prototype exactly (verified against the SDK header shipped on this machine, not just from memory)#if defined(__MINGW32__)— zero effect on MSVC, macOS, or Linux buildssrc/core/AsyncLogWriter.cpp,src/core/ThreadName.cpp,src/core/SystemInfo.cppAsyncLogWriter.cpp(its own comment explains why it duplicatesThreadName.cpp's platform block instead of including it: this file compiles into 51 test targets vs. 6, so sharing a header with a linkable source isn't free — this is the same idiom, just for a pure prototype)TCP_INFO_v0/SIO_TCP_INFO)No RFC needed — plain build fix, explicitly exempted per
GOVERNANCE.md.Test plan
mingw1310_64) compiles clean and linksAetherSDR.exe— confirmed by rebuilding from a failing state to a clean 1367-target build with noFAILED/error:linescl.exe, but flagging as unconfirmed rather than assumed)👨🏼💻 Co-authored by Claude Sonnet 5