Windows GC: harden conservative stack scan (getRegisters, suspend retry, RegisterState) - #382
Windows GC: harden conservative stack scan (getRegisters, suspend retry, RegisterState)#382robobun wants to merge 4 commits into
Conversation
…d return only the requested prefix GetThreadContext can fail, leaving CONTEXT (including Rsp/Sp) undefined; the GC caller would then compute a bogus stack range and OOM in growBuffer. Crash immediately with the Win32 error code instead. Only CONTEXT_INTEGER | CONTEXT_CONTROL is requested, so return a prefix that ends after Rip (x64) / Pc (arm64) rather than sizeof(CONTEXT). The rest of the struct is never populated and need not be copied into the conservative root buffer.
…isters Any bytes getRegisters() leaves untouched are now zero rather than whatever was on the collector thread's stack (typically stale JSCell* from the previous SlotVisitor::drain), so they no longer scan as false conservative roots. Value-initialization of an aggregate is malloc-free, preserving the no-allocation requirement while the target thread is suspended.
bf27cb2 to
f9c3527
Compare
WalkthroughThe change expands Windows register capture, retries Windows thread context retrieval, returns architecture-specific register data, clears unpopulated registers, and asserts when stack scanning cannot process a live thread. ChangesWindows stack scanning
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/WTF/wtf/win/ThreadingWin.cpp`:
- Around line 220-253: Update the retry loop in Thread::suspend so SuspendThread
failures are retried with the same bounded backoff as GetThreadContext failures
instead of returning immediately. Record GetLastError, apply SwitchToThread or
Sleep according to the attempt count, and only return the final error after
maxAttempts is exhausted; preserve cleanup for attempts where suspension
succeeded but GetThreadContext fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 671c2b50-64d7-4e4a-b5e0-69cd47292d52
📒 Files selected for processing (3)
Source/JavaScriptCore/heap/MachineStackMarker.cppSource/JavaScriptCore/heap/RegisterState.hSource/WTF/wtf/win/ThreadingWin.cpp
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Preview Builds
|
…text failures; crash if a live thread still cannot be suspended during GC SuspendThread and GetThreadContext can both transiently fail while the target is in early start, exit, or certain kernel transitions. Retry the SuspendThread/GetThreadContext pair with a short spin-then-sleep backoff (up to 100 attempts) before returning failure. In MachineThreads::tryCopyOtherThreadStacks, on Windows, if suspend() still fails, RELEASE_ASSERT with the Win32 error instead of silently skipping that thread's stack for the current GC cycle. Any thread still in the iterated set is live at the WTF level (didExit() removes the thread from the group under the same lock we hold before setting m_didExit), so there is no exited-thread case to tolerate here. Skipping a live mutator's stack drops its roots and can sweep objects that are still referenced.
…in64 variant Gate the inline-asm capture on COMPILER(GCC_COMPATIBLE) || COMPILER(CLANG) instead of !OS(WINDOWS). clang-cl accepts GNU inline asm (it defines __clang__ but not __GNUC__), so Windows clang-cl builds now take the explicit path instead of falling back to setjmp into a 256-byte jmp_buf that is scanned in full. Add a Win64 x86_64 RegisterState capturing the Windows x64 callee-saved GPRs (rbx, rbp, rdi, rsi, r12-r15; rdi/rsi are callee-saved on Win64 unlike SysV). Add rbp to the existing SysV x86_64 list. The setjmp fallback is kept for MSVC proper.
f9c3527 to
87d87c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/JavaScriptCore/heap/MachineStackMarker.cpp`:
- Around line 181-190: Update the Windows failure handling in
tryCopyOtherThreadStacks() so an exit-related Thread::suspend() failure skips
that thread instead of reaching RELEASE_ASSERT_NOT_REACHED. Check the thread’s
exit state while iterating m_threads, preserve the existing assertion for other
suspension failures, and retain normal stack scanning for successfully suspended
live threads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4bd2382c-3b55-444b-8a66-2ef927a66e83
📒 Files selected for processing (3)
Source/JavaScriptCore/heap/MachineStackMarker.cppSource/JavaScriptCore/heap/RegisterState.hSource/WTF/wtf/win/ThreadingWin.cpp
There was a problem hiding this comment.
Thanks — 87d87c1 addresses both prior comments (SuspendThread now retried; the dead hasExited() guard replaced with an unconditional RELEASE_ASSERT_NOT_REACHED). I didn't find further issues, but this touches the GC conservative-root path and Windows thread suspension, so it's worth a human look.
What was reviewed:
- Suspend/resume count stays balanced across the retry loop (Resume before backoff on
GetThreadContextfailure; no Resume onSuspendThreadfailure). - Truncated
getRegistersreturn sizes are 8-byte aligned on both x64 (Rip+8) and arm64 (Pc+8), so thecopyMemoryCPURegister-alignment asserts still hold;Rsp/Spremain covered forMachineContext::stackPointer. - Win64 callee-save set (
rbx rbp rdi rsi r12–r15) matches the MS x64 ABI; ARM64 clang-cl now hits the existing x19–x28 branch, which matches Win ARM64 AAPCS. - Backoff runs while earlier threads in the set are suspended, but only
SwitchToThread/Sleepare called (no allocation/locking), so no new deadlock vector.
Extended reasoning...
Overview
Four related hardening changes to the Windows conservative-stack-scan path: (1) Thread::getRegisters now asserts on GetThreadContext failure and returns only the populated CONTEXT_INTEGER|CONTEXT_CONTROL prefix instead of sizeof(CONTEXT); (2) tryCopyOtherThreadStack value-initializes PlatformRegisters so unpopulated bytes are zero rather than stale collector-stack; (3) Thread::suspend retries transient SuspendThread/GetThreadContext failures with spin-then-sleep backoff, and tryCopyOtherThreadStacks now RELEASE_ASSERT_NOT_REACHED on persistent Windows suspend failure instead of silently skipping the thread; (4) RegisterState.h gates the explicit callee-save capture on COMPILER(GCC_COMPATIBLE) || COMPILER(CLANG) instead of !OS(WINDOWS), adds a Win64 x86_64 variant, and adds rbp to the SysV list.
Security risks
This is squarely in memory-safety territory. Conservative root scanning is what keeps the GC from sweeping live objects; getting it wrong yields use-after-free (missed roots) or unbounded retention (false roots). The PR moves in the safe direction on both axes — zero-init and size-trim reduce false roots; the retry + assert convert a silent root-drop into a bounded retry then deterministic crash. The new RELEASE_ASSERT_NOT_REACHED paths are a deliberate behavioral change (previously silent skip → now crash) that could surface in production if a Windows environment has a persistently unsuspendable mutator; that's the stated intent, but it's a policy call a human should ratify. No auth/crypto/injection surface.
Level of scrutiny
High. This is core GC + Win32 threading, where correctness depends on CONTEXT struct layout, MS x64 / Win-ARM64 calling conventions, and SuspendThread semantics — none of which are exercised by non-Windows CI, and errors manifest as rare heisenbugs. The retry loop also runs while other mutator threads are already suspended, so any call made during backoff must be allocation- and lock-free; SwitchToThread/Sleep satisfy that, but it's the kind of invariant that deserves a second pair of eyes.
Other factors
Both prior review comments (mine on the dead hasExited() guard, CodeRabbit's on SuspendThread not being retried) were addressed in 87d87c1 and the current diff reflects those fixes. I re-checked the updated retry loop for suspend-count balance, verified the truncated getRegisters sizes remain sizeof(CPURegister)-aligned so copyMemory's RELEASE_ASSERTs don't fire, confirmed Rsp/Sp are within the returned prefix so MachineContext::stackPointer still works, and checked the Win64 callee-save GPR list against the MS x64 ABI and the ARM64 branch against Win-ARM64 AAPCS. Everything looks correct, but given the blast radius (every Windows GC cycle) and the platform-specific subtlety, I'm deferring rather than approving.
Picks up oven-sh/WebKit#382 (87d87c14), which is on top of WebKit main 9f8f24db19. Four changes to the Windows conservative stack scan: - Thread::getRegisters: RELEASE_ASSERT on GetThreadContext failure and return only the populated CONTEXT_INTEGER|CONTEXT_CONTROL prefix (256 bytes on x64 instead of sizeof(CONTEXT)=1232). - MachineThreads::tryCopyOtherThreadStack: value-initialize PlatformRegisters so unpopulated bytes scan as zero, not stale collector stack. - Thread::suspend: retry transient SuspendThread/GetThreadContext failures (spin then Sleep, up to 100 attempts). tryCopyOtherThreadStacks now RELEASE_ASSERTs if a live thread still cannot be suspended rather than silently skipping its roots. - RegisterState: use the explicit callee-save inline-asm capture on clang-cl (Win64 variant with rbx/rbp/rdi/rsi/r12-r15) instead of the 256-byte setjmp fallback. Also adds rbp to the SysV x86_64 list. Also pulls in the intervening WebKit-main commits 34c01d13..9f8f24db (shorthand-in-arrow arguments-capture fix, CodeBlock-aging gating, Dockerfile.windows CI fixes).
2 participants
Four targeted hardening changes to the Windows conservative-stack-scan path. Each is a separate commit.
1.
Thread::getRegisters(ThreadingWin.cpp)GetThreadContextwas called without checking the return value, andsizeof(CONTEXT)(~1232 bytes on x64) was returned even though onlyCONTEXT_INTEGER | CONTEXT_CONTROL(~256 bytes) was filled.Rspand compute a bogus stack range, OOMing ingrowBuffer/fastMalloc. NowRELEASE_ASSERTs withGetLastError()in the crash info.offsetof(CONTEXT, Rip) + sizeof(Rip)on x64,offsetof(CONTEXT, Pc) + sizeof(Pc)on arm64).2.
MachineThreads::tryCopyOtherThreadStack(MachineStackMarker.cpp)PlatformRegisters registers;is now value-initialized ({ }) so any bytesgetRegistersdoes not populate are zero rather than stale collector-thread stack (typically last cycle'sSlotVisitor::drainframes full ofJSCell*). This is the platform-independent fix for the false-retention issue in (1); the size trim above is belt-and-braces. Value-initialization of an aggregate is malloc-free, preserving the no-allocation requirement while the target is suspended.3.
Thread::suspendretry + GC assert on persistent failureSuspendThreadandGetThreadContextcan both transiently fail while the target is in early start, exit, or certain kernel transitions; CoreCLR and Go both retry for this reason.Thread::suspendnow retries theSuspendThread/GetThreadContextpair with a short spin-then-sleep backoff (up to 100 attempts) before returning failure.In
MachineThreads::tryCopyOtherThreadStacks, on Windows, ifsuspend()still fails,RELEASE_ASSERTwith the Win32 error instead of silently skipping that thread's stack for the cycle. A thread that has completeddidExit()is already removed from the group under the lock held here, so any thread still in the iterated set is live; there is no exited-thread case to tolerate. Skipping a live mutator drops its roots and can sweep objects that are still referenced; a deterministic crash with a real signature is preferable to a use-after-free several GCs later.4.
RegisterState.hThe explicit callee-save capture was gated on
!OS(WINDOWS), so clang-cl builds fell back tosetjmpinto a 256-bytejmp_buf(xmm6-15, mxcsr, ...) that all gets scanned. The gate is nowCOMPILER(GCC_COMPATIBLE) || COMPILER(CLANG)(clang-cl defines__clang__but not__GNUC__, and accepts GNU inline asm). A Win64 x86_64 variant captures the Windows callee-saved GPRs (rbx, rbp, rdi, rsi, r12-r15; rdi/rsi are callee-saved on Win64 unlike SysV).rbpis also added to the SysV x86_64 list. MSVC proper keeps thesetjmpfallback.JSC uses SysV ABI for JIT operations on Windows x64, but the Win64 callee-saved set is a strict superset of SysV's, and a
sysv_abifunction calling Win64 C++ can keep a value inrdi/rsiacross the call (clang knows the Win64 callee preserves them), so capturing the Win64 set is both sufficient and necessary. The previoussetjmpfallback also capturedrdi/rsi, so coverage is unchanged minus the xmm noise.Expected impact
No behaviour change on macOS/Linux beyond the zero-init in (2) and the extra
rbpword in (4). On Windows: fewer false roots per GC (addresses Windows-only flaky memory growth), and failures in the suspend/context path now crash with a signature instead of producing an incomplete root set (addresses Windows-heavySlotVisitor/marking crash reports).