Windows GC: harden conservative stack scan (getRegisters, suspend retry, RegisterState)

Source/JavaScriptCore/heap/MachineStackMarker.cpp

@@ -118,7 +118,13 @@ static void NODELETE copyMemory(void* dst, const void* src, size_t size)
 // See: https://bugs.webkit.org/show_bug.cgi?id=146297
 void MachineThreads::tryCopyOtherThreadStack(const ThreadSuspendLocker& locker, Thread& thread, void* buffer, size_t capacity, size_t* size)
 {
-    PlatformRegisters registers;
+    // Value-initialize so that any bytes getRegisters() does not populate are zero rather
+    // than uninitialized collector-thread stack. On Windows in particular, PlatformRegisters
+    // is the full CONTEXT struct but only the integer/control portion is requested; the
+    // remainder otherwise carries stale JSCell* from prior SlotVisitor frames and gets
+    // scanned as false roots. This must stay malloc-free (the target thread is suspended),
+    // which value-initialization of an aggregate is.
+    PlatformRegisters registers { };
     size_t registersSize = thread.getRegisters(locker, registers);
 
     // This is a workaround for <rdar://problem/27607384>. libdispatch recycles work
@@ -172,6 +178,16 @@ bool MachineThreads::tryCopyOtherThreadStacks(const AbstractLocker& locker, void
                         WTFReportError(__FILE__, __LINE__, WTF_PRETTY_FUNCTION,
                             "JavaScript garbage collection encountered an invalid thread (err 0x%x): Thread [%d/%d: %p].",
                             result.error(), index, threads.size(), thread.ptr());
+#elif OS(WINDOWS)
+                        // Thread::suspend already retries transient SuspendThread /
+                        // GetThreadContext failures. A thread that has finished didExit() is
+                        // no longer in this set (removal happens under the group lock we hold),
+                        // so any thread we iterate is live at the WTF level. If it still cannot
+                        // be suspended, proceeding would skip scanning its stack for this GC
+                        // cycle, dropping its roots; objects it references could then be swept
+                        // while still live. Crash now with a real signature rather than risk a
+                        // use-after-free several GCs later.
+                        RELEASE_ASSERT_NOT_REACHED(static_cast<uint64_t>(result.error()), index, threads.size());
 #endif
                     }
                 }

Source/JavaScriptCore/heap/RegisterState.h

@@ -31,7 +31,10 @@
 
 namespace JSC {
 
-#if !OS(WINDOWS)
+// clang-cl accepts GNU inline asm but does not define __GNUC__, so COMPILER(CLANG) is
+// checked in addition to COMPILER(GCC_COMPATIBLE). MSVC proper falls through to the
+// setjmp fallback below.
+#if COMPILER(GCC_COMPATIBLE) || COMPILER(CLANG)
 
 // ALLOCATE_AND_GET_REGISTER_STATE has to ensure that the GC sees callee-saves. It achieves this by
 // ensuring that the callee-saves are either spilled to the stack or saved in the RegisterState. The code
@@ -55,9 +58,37 @@ struct RegisterState {
     SAVE_REG(edi, registers.edi); \
     SAVE_REG(esi, registers.esi)
 
+#elif CPU(X86_64) && OS(WINDOWS)
+// Win64 calling convention: rdi and rsi are callee-saved (unlike SysV).
+struct RegisterState {
+    uint64_t rbx;
+    uint64_t rbp;
+    uint64_t rdi;
+    uint64_t rsi;
+    uint64_t r12;
+    uint64_t r13;
+    uint64_t r14;
+    uint64_t r15;
+};
+
+#define SAVE_REG(regname, where) \
+    asm volatile ("movq %%" #regname ", %0" : "=m"(where) : : "memory")
+
+#define ALLOCATE_AND_GET_REGISTER_STATE(registers) \
+    RegisterState registers; \
+    SAVE_REG(rbx, registers.rbx); \
+    SAVE_REG(rbp, registers.rbp); \
+    SAVE_REG(rdi, registers.rdi); \
+    SAVE_REG(rsi, registers.rsi); \
+    SAVE_REG(r12, registers.r12); \
+    SAVE_REG(r13, registers.r13); \
+    SAVE_REG(r14, registers.r14); \
+    SAVE_REG(r15, registers.r15)
+
 #elif CPU(X86_64)
 struct RegisterState {
     uint64_t rbx;
+    uint64_t rbp;
     uint64_t r12;
     uint64_t r13;
     uint64_t r14;
@@ -70,6 +101,7 @@ struct RegisterState {
 #define ALLOCATE_AND_GET_REGISTER_STATE(registers) \
     RegisterState registers; \
     SAVE_REG(rbx, registers.rbx); \
+    SAVE_REG(rbp, registers.rbp); \
     SAVE_REG(r12, registers.r12); \
     SAVE_REG(r13, registers.r13); \
     SAVE_REG(r14, registers.r14); \
@@ -158,7 +190,7 @@ struct RegisterState {
     SAVE_REG(23, registers.r23)
 
 #endif
-#endif // !OS(WINDOWS)
+#endif // COMPILER(GCC_COMPATIBLE) || COMPILER(CLANG)
 
 #ifndef ALLOCATE_AND_GET_REGISTER_STATE
 

Source/WTF/wtf/win/ThreadingWin.cpp

@@ -217,24 +217,39 @@ auto Thread::suspend(const ThreadSuspendLocker&) -> Expected<void, PlatformSuspe
     // currentMayBeNull, not currentSingleton: the libpas scavenger calls this while holding
     // the heap lock, and currentSingleton would lazy-allocate a Thread for it.
     RELEASE_ASSERT_WITH_MESSAGE(this != Thread::currentMayBeNull(), "We do not support suspending the current thread itself.");
-    DWORD result = SuspendThread(m_handle);
-    if (result == (DWORD)-1)
-        return makeUnexpected(result);
     // SuspendThread only requests a suspension; on multi-core Windows the target may
     // continue to execute in user mode briefly after SuspendThread returns. Callers that
     // read registers (MachineStackMarker) happen to force a sync via GetThreadContext,
     // but callers that only need "target is stopped" -- notably the libpas scavenger's
     // pasSuspenderBeginSuspend -- would otherwise decommit TLC pages while the mutator
     // is still writing to them. Force the suspension to complete synchronously here so
     // every Thread::suspend caller can rely on the invariant.
-    CONTEXT ctx;
-    ctx.ContextFlags = CONTEXT_INTEGER;
-    if (!GetThreadContext(m_handle, &ctx)) {
-        DWORD error = GetLastError();
-        ResumeThread(m_handle);
-        return makeUnexpected(error);
+    //
+    // GetThreadContext can fail transiently (the target is in early start, in exit, or in
+    // certain kernel transitions). CoreCLR and Go both retry SuspendThread/GetThreadContext
+    // with a short backoff for exactly this reason. Follow the same pattern here rather
+    // than immediately returning failure, because a failure here causes MachineThreads to
+    // skip scanning this thread's stack for the current GC cycle, dropping its roots.
+    constexpr unsigned maxAttempts = 100;
+    constexpr unsigned spinAttempts = 8;
+    DWORD lastError = 0;
+    for (unsigned attempt = 0; attempt < maxAttempts; ++attempt) {
+        DWORD result = SuspendThread(m_handle);
+        if (result != (DWORD)-1) {
+            CONTEXT ctx;
+            ctx.ContextFlags = CONTEXT_INTEGER;
+            if (GetThreadContext(m_handle, &ctx))
+                return { };
+            lastError = GetLastError();
+            ResumeThread(m_handle);
+        } else
+            lastError = GetLastError();
+        if (attempt < spinAttempts)
+            SwitchToThread();
+        else
+            Sleep(1);
     }
-    return { };
+    return makeUnexpected(lastError);
 }
 
 // During resume, suspend or resume should not be executed from the other threads.
@@ -246,8 +261,24 @@ void Thread::resume(const ThreadSuspendLocker&)
 size_t Thread::getRegisters(const ThreadSuspendLocker&, PlatformRegisters& registers)
 {
     registers.ContextFlags = CONTEXT_INTEGER | CONTEXT_CONTROL;
-    GetThreadContext(m_handle, &registers);
+    if (!GetThreadContext(m_handle, &registers)) [[unlikely]] {
+        // On failure the CONTEXT (including the stack pointer) is undefined; the caller
+        // would compute a bogus stack range and OOM in MachineThreads::growBuffer. Crash
+        // with the Win32 error instead of wandering off into a huge allocation.
+        RELEASE_ASSERT_NOT_REACHED(static_cast<uint64_t>(GetLastError()));
+    }
+    // We only requested CONTEXT_INTEGER | CONTEXT_CONTROL, so only that prefix of CONTEXT
+    // is populated. Return just that range so the conservative root scan does not copy the
+    // remainder of the struct (which the caller zero-initializes; see MachineStackMarker).
+#if CPU(X86_64)
+    static_assert(offsetof(CONTEXT, Rax) < offsetof(CONTEXT, Rsp) && offsetof(CONTEXT, Rsp) < offsetof(CONTEXT, Rip));
+    return offsetof(CONTEXT, Rip) + sizeof(registers.Rip);
+#elif CPU(ARM64)
+    static_assert(offsetof(CONTEXT, Sp) < offsetof(CONTEXT, Pc));
+    return offsetof(CONTEXT, Pc) + sizeof(registers.Pc);
+#else
     return sizeof(CONTEXT);
+#endif
 }
 
 Thread& Thread::initializeCurrentTLS()