SamplingProfiler: stop the stack walk at a null entry frame instead of dereferencing vmEntryRecord(nullptr) - #395
SamplingProfiler: stop the stack walk at a null entry frame instead of dereferencing vmEntryRecord(nullptr)#395robobun wants to merge 2 commits into
Conversation
…the sampling profiler's walk SamplingProfiler::takeSample only gates on vm.entryScope, but vm.topEntryFrame can be null while entryScope is set: VMEntryScope is created and destroyed in C++ around vmEntryToJavaScript, while topCallFrame/topEntryFrame are stored and restored inside doVMEntry (after the O(paddedArgCount) argument copy loops). A sample landing in those windows walks a half-built entry frame or a stale topCallFrame with FrameWalker::m_entryFrame == nullptr. The first walked frame whose caller slot reads null then matches the null entry frame, and unsafeCallerFrame dereferences vmEntryRecord(nullptr), which faults just below address zero (0xFFFFFFFFFFFFFFC8 on Windows x64). Bail out when currEntryFrame is null: there is no VMEntryRecord to consult, and returning null ends the walk the same way reaching prevTopCallFrame == nullptr at the outermost record does. unsafeCallerFrame's only caller is the sampling profiler's FrameWalker::advanceToParentFrame.
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a two-line defensive null check that turns a near-null dereference into a clean end-of-walk.
What was reviewed:
- Confirmed
unsafeCallerFrameis called only fromSamplingProfiler.cpp'sFrameWalker::advanceToParentFrame, so the change is confined to the sampler. - Returning
nullptrsetsm_callFrame = nullptr, whichisAtTop()/resetAtMachineFrame()already treat as end-of-walk — same path as reaching the outermostprevTopCallFrame == nullptr. - The non-
unsafecallerFrame()and the fall-throughstatic_cast<CallFrame*>(unsafeCallerFrameOrEntryFrame())path are untouched.
Extended reasoning...
Overview
The PR adds a single if (!currEntryFrame) return nullptr; guard (plus an explanatory comment) inside CallFrame::unsafeCallerFrame in Source/JavaScriptCore/interpreter/CallFrame.cpp. When the sampling profiler's frame walker reaches a frame whose caller slot compares equal to a null currEntryFrame, the old code called vmEntryRecord(nullptr) and read m_prevTopEntryFrame at a negative offset from address zero, faulting at 0xFFFFFFFFFFFFFFC8 on Windows x64. The new code returns nullptr instead, which the walker's isAtTop() check treats as a clean end of the walk.
Security risks
None. This is crash-hardening in the sampling profiler's unsafe stack walker (already SUPPRESS_ASAN and designed to tolerate reading paused-thread state). It does not touch auth, crypto, permissions, or user-controlled data paths; it strictly replaces a segfault with an early return.
Level of scrutiny
Low-to-moderate. The change is two lines of logic in a function with exactly one call site (FrameWalker::advanceToParentFrame in SamplingProfiler.cpp:172, verified via grep). The behavior change is monotonic: every input that previously produced a valid result still does (the guard only fires when both unsafeCallerFrameOrEntryFrame() and currEntryFrame are null, a case that previously crashed). Even if the PR's race-window analysis were incomplete, the added null check is harmless — it cannot make a valid walk terminate early, since a real entry frame is never null while JS is on the stack.
Other factors
- Verified the caller handles a
nullptrreturn:advanceToParentFrameassigns it tom_callFrame, thenresetAtMachineFrame()checksisAtTop()(!m_callFrame) and returns, and the walk loop exits — identical to the existing outermost-record termination path. - The safe variant
CallFrame::callerFrameis unchanged; only the profiler-specificunsafevariant is touched. - The PR description provides a symbolized minidump stack that lines up exactly with the code path being guarded, and a plausible mechanism (
vm.entryScopeset whilevm.topEntryFrameis still null duringdoVMEntryprologue/epilogue). - No prior human or bot reviews to reconcile; CodeRabbit was rate-limited and did not review.
Preview Builds
|
…null entry frame A zeroed frame whose caller slot reads null, walked with a null EntryFrame cursor, models the state the sampling profiler hits when a sample lands inside a VM entry/exit transition. Without the guard the test faults in VMEntryRecord::unsafePrevTopEntryFrame reading just below address zero, the same location as the CI crash; with it unsafeCallerFrame reports the end of the stack.
There was a problem hiding this comment.
LGTM — a strictly defensive null guard in the sampling profiler's unsafe stack walk. The inline nit about JS_EXPORT_PRIVATE is a portability concern for shared-library ports only and doesn't affect Bun's static-JSC builds; worth the one-word fix but not a blocker.
Checked that unsafeCallerFrame's only caller is FrameWalker::advanceToParentFrame (SamplingProfiler.cpp:172), and that returning nullptr there terminates the walk cleanly via isAtTop() → !m_callFrame.
Verified the guard only fires when currEntryFrame == nullptr — valid walks (non-null entry frame) are untouched, so no behavior change on the happy path.
Confirmed resetAtMachineFrame() early-returns on a null m_callFrame, so no follow-on deref after the new bail-out.
Extended reasoning...
Overview
The PR adds a two-line null guard inside CallFrame::unsafeCallerFrame (CallFrame.cpp) so that when the sampling profiler's EntryFrame* cursor is null and a walked frame's caller slot also reads null, the walk ends instead of dereferencing vmEntryRecord(nullptr) (which faults just below address zero). It also adds a deterministic TestWebKitAPI unit test that constructs this exact state, and wires it into TestJavaScriptCore_SOURCES in CMake.
Security risks
None. This is a defensive null check inside a SUPPRESS_ASAN helper that exists specifically for the sampling profiler's racy reads of another thread's stack. It reads no attacker-controlled data, adds no new dereference, and can only cause the profiler to record a shorter stack trace in a race window where it previously segfaulted. There is no auth, crypto, or permission surface here.
Level of scrutiny
Medium — it touches interpreter/CallFrame.cpp, but the change is confined to unsafeCallerFrame, whose sole caller I confirmed is SamplingProfiler.cpp:172. The guard is inside the unsafeCallerFrameOrEntryFrame() == currEntryFrame branch and only triggers when both are null, so any walk with a real (non-null) entry frame is bit-for-bit unchanged. Returning nullptr sets m_callFrame = nullptr, isAtTop() returns true, and resetAtMachineFrame() early-returns on null — the termination path is the same one already used when the outermost VMEntryRecord yields a null prevTopCallFrame. The PR description is unusually thorough, matching the CI minidump fault address to vmEntryRecord(nullptr) under both Windows and SysV frame layouts, and the ASAN test reproduces the fault without the guard and passes with it.
Other factors
The one finding from the bug-hunting pass is a nit: unsafeCallerFrame in CallFrame.h:227 lacks JS_EXPORT_PRIVATE, so the new test won't link on ports that build JSC as a shared library with hidden visibility. Bun's CI builds with ENABLE_STATIC_JSC=ON and does not build the TestWebKitAPI target, so this repo's builds are unaffected — it's a one-word portability fix for upstreaming, not a correctness or merge blocker here. No prior human reviews or outstanding comments on the PR; CodeRabbit was rate-limited and did not review. Preview build for the fix commit succeeded.
| JSC::CallFrame* callFrame = JSC::CallFrame::create(reinterpret_cast<JSC::Register*>(zeroedFrame)); | ||
|
|
||
| JSC::EntryFrame* entryFrame = nullptr; | ||
| EXPECT_EQ(callFrame->unsafeCallerFrame(entryFrame), nullptr); |
There was a problem hiding this comment.
🟡 unsafeCallerFrame is declared in CallFrame.h:227 without JS_EXPORT_PRIVATE (unlike the adjacent callerFrame on line 228), so on ports that build JavaScriptCore as a shared library with hidden visibility (Win, GTK, WPE, Cocoa, PlayStation, JSCOnly without ENABLE_STATIC_JSC) this test won't link. Bun's own CI uses ENABLE_STATIC_JSC=ON and doesn't build TestWebKitAPI so nothing breaks here, but adding JS_EXPORT_PRIVATE to the declaration is a one-word fix that keeps the test portable and eases upstreaming.
Extended reasoning...
What the bug is
The new test at Tools/TestWebKitAPI/Tests/JavaScriptCore/UnsafeCallerFrame.cpp:48 calls callFrame->unsafeCallerFrame(entryFrame). In Source/JavaScriptCore/interpreter/CallFrame.h that method is declared as:
CallFrame* unsafeCallerFrame(EntryFrame*&) const; // line 227 — no export
JS_EXPORT_PRIVATE CallFrame* callerFrame(EntryFrame*&) const; // line 228 — exportedand defined out-of-line in CallFrame.cpp (not inline in the header). Without JS_EXPORT_PRIVATE, the symbol is not exported from a shared JavaScriptCore library.
The code path that triggers it
The PR adds Tests/JavaScriptCore/UnsafeCallerFrame.cpp unconditionally to TestJavaScriptCore_SOURCES under if (ENABLE_JAVASCRIPTCORE). Several ports build TestJavaScriptCore and set JavaScriptCore_LIBRARY_TYPE SHARED with hidden default visibility:
OptionsGTK.cmake:491— SHARED, plusCXX_VISIBILITY_PRESET hiddenOptionsWin.cmake:193— SHARED (DLL export table)OptionsCocoa.cmake:409— SHARED,-fvisibility=hiddenOptionsPlayStation.cmake— SHARED, hiddenOptionsJSCOnly.cmake:124— SHARED whenENABLE_STATIC_JSCis OFF, with hidden visibility (:7)
On any of those, linking TestJavaScriptCore will fail with an undefined reference / unresolved external for JSC::CallFrame::unsafeCallerFrame(JSC::EntryFrame*&) const.
Why existing code doesn't prevent it
Until this PR, unsafeCallerFrame's only caller was SamplingProfiler.cpp, which is compiled into the JavaScriptCore library itself, so the symbol never needed to cross the library boundary. The new test is the first out-of-library caller. The PR was verified only on a JSCOnly Linux build with ENABLE_STATIC_JSC=ON, where all symbols are visible regardless of the export macro, so the missing export was not observed.
Step-by-step proof
- Configure GTK (or JSCOnly with
-DENABLE_STATIC_JSC=OFF): CMake setsJavaScriptCore_LIBRARY_TYPE=SHAREDandCXX_VISIBILITY_PRESET hidden. CallFrame::unsafeCallerFrameis compiled intolibJavaScriptCore.sowith default (hidden) visibility because its declaration lacksJS_EXPORT_PRIVATE(which expands to__attribute__((visibility("default")))/__declspec(dllexport)).nm -D libJavaScriptCore.so | c++filt | grep unsafeCallerFrame→ no output; the symbol is local.TestJavaScriptCorecompilesUnsafeCallerFrame.cpp, which references_ZNK3JSC9CallFrame17unsafeCallerFrameERPNS_10EntryFrameE.ldfails:undefined reference to 'JSC::CallFrame::unsafeCallerFrame(JSC::EntryFrame*&) const'.
Impact
None on this fork's CI: .github/workflows/build.yml builds with ENABLE_STATIC_JSC=ON and does not build the TestWebKitAPI target. But it will break the TestJavaScriptCore build on every shared-library port and would block upstreaming this fix to WebKit as-is.
Fix
One word — add JS_EXPORT_PRIVATE to the declaration in CallFrame.h:227, matching the adjacent callerFrame:
JS_EXPORT_PRIVATE CallFrame* unsafeCallerFrame(EntryFrame*&) const;1 participant
Crash
Bun CI (Windows 2019 x64,
bun --cpu-prof,test-cpu-prof-dir-worker.jssampling at 50us) intermittently segfaults in thejsc.sampling-profiler.threadat address0xFFFFFFFFFFFFFFC8. Symbolized stack from the CI minidump (oven-sh/bun Buildkite build 90390):The fault address is
vmEntryRecord(nullptr)->m_prevTopEntryFrame:vmEntryRecord(entryFrame)isentryFrame - VMEntryTotalFrameSize, so with a null entry frame the load lands just below address zero (-0x38with the Windows x64 frame layout).Mechanism
SamplingProfiler::takeSampleonly requiresvm.entryScopeto be set, but there are windows where that holds whilevm.topEntryFrameis null and the walk starts from memory that is not a live frame chain:VMEntryScopeis constructed and destroyed in C++ aroundvmEntryToJavaScript, whilevm.topCallFrame/vm.topEntryFrameare stored and restored insidedoVMEntry, and the store happens after theO(paddedArgCount)argument copy loops. On an outermost entry, everything fromVMEntryScopesetup to that store runs withentryScopeset andtopEntryFramenull; the same applies on exit betweendoVMEntry's restore and theVMEntryScopeteardown.vmEntryToJavaScriptlies inside the LLInt PC range, so a sample landing in those loops takes thetopFrameIsLLIntpath and walks from the machine frame pointer, which is the half-built entry frame. A sample landing in the surrounding C++ takes thetopCallFramefallback ("We resort to topCallFrame"), which after the restore is stale.Either way
FrameWalker::m_entryFramesnapshotsvm.topEntryFrame == nullptr, and the walker advances through dead or half-initialized stack memory (isValidFramePointeronly checks stack bounds, andresetAtMachineFrameaccepts any frame whose CodeBlock slot reads null). The first walked frame whose caller slot reads null compares equal to the nullm_entryFrameinunsafeCallerFrame, which then dereferencesvmEntryRecord(nullptr).Fix
Bail out of the matched branch when
currEntryFrameis null: with no entry frame there is noVMEntryRecordto consult, and returning null ends the walk exactly like reachingprevTopCallFrame == nullptrat the outermost record does. Valid walks are unaffected, since while JS is on the stack a caller slot matchesm_entryFrameonly at real, non-null entry frames.unsafeCallerFrame's only caller is the sampling profiler'sFrameWalker::advanceToParentFrame, so the change is confined to the sampler.Upstream has the same code: https://github.com/WebKit/WebKit/blob/2e8a96a8c5857b071f073feea215c90803f22520/Source/JavaScriptCore/interpreter/CallFrame.cpp#L178
Testing
Tools/TestWebKitAPI/Tests/JavaScriptCore/UnsafeCallerFrame.cppconstructs the raced state deterministically: a zeroed frame (caller slot reads null) walked with a nullEntryFramecursor. On a JSCOnly debug+ASAN Linux build of theTestJavaScriptCoretarget:AddressSanitizer: SEGV ... VMEntryRecord.h:60:66 in JSC::VMEntryRecord::unsafePrevTopEntryFrame(), with the record pointer at0xffffffffffffffb0(vmEntryRecord(nullptr)under the SysV frame layout; the Windows layout puts the faulting load at the CI crash address0xFFFFFFFFFFFFFFC8)TestJavaScriptCoresuite is unaffected (the twoMarkedVector.GCLivenessInlineBuffer_*failures on that configuration are pre-existing and reproduce with this test filtered out)Reproduction of the live race
The race needs a sample to land inside a per-entry window of microseconds, and the walk from there to hit a null caller slot before hitting something that fails
isValidCodeBlock. It fired about once per fifteen builds on Bun's Windows 2019 CI runners. It did not reproduce locally: several hundred probe runs across Windows Server 2019 and Linux stayed clean, including 100 runs of the exact workload under the exact crashing binary and runs with the entry window artificially widened (callbacks declared with 30k parameters invoked fromsetImmediate, making the pad loop before thetopEntryFramestore dominate the runtime, confirmed clean on both platforms at a 10us sampling interval). The stress form is deliberately not the regression guard; the constructed-state test above is.