node:inspector: owner-refcount the shared CPU profiler; drop post() command errors without a callback - #36019
node:inspector: owner-refcount the shared CPU profiler; drop post() command errors without a callback#36019robobun wants to merge 1 commit into
Conversation
…ommand errors without a callback BunCPUProfiler wraps the VM-singleton JSC::SamplingProfiler. Previously a thread-local bool tracked whether it was running, so any Session.disconnect() or Profiler.disable would pause() and clearData() the one shared instance, emptying the buffer `--cpu-prof` writes at exit, and a second concurrent Session would adopt (on start) or consume (on stop) the first Session's samples. Each caller now acquires a per-owner token that records its own start timestamp. The sampler starts on the first owner and stays running while any owner is live; stopping one owner drains the sampler into a retained trace buffer, returns only traces inside that owner's window (so a late-starting Session no longer reports pre-start samples), and only the last owner to release pauses/clears. The CLI path and per-Worker profiling keep their existing entry points via a single implicit owner held in their stead. Session.post() without a callback now drops command failures (method not found, -32000 protocol errors) instead of throwing, matching Node's fire-and-forget contract. Argument validation and ERR_INSPECTOR_NOT_CONNECTED still throw synchronously, as Node does.
|
Warning Review limit reached
Next review available in: 4 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
|
Status: ready for review, waiting on CI. Reproduced with the two-session start/stop sequence in the PR body: before this change session A's Locally green: |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Drain newly-collected traces into the retained buffer. releaseStackTraces() | ||
| // moves m_stackTraces out but leaves m_liveCellPointers intact, so the | ||
| // executables/callees referenced by s_retainedTraces stay GC-live until the | ||
| // final owner's clearData() below. | ||
| { | ||
| auto newTraces = profiler->releaseStackTraces(); | ||
| s_retainedTraces.reserveCapacity(s_retainedTraces.size() + newTraces.size()); | ||
| for (auto& trace : newTraces) | ||
| s_retainedTraces.append(WTF::move(trace)); | ||
| } |
There was a problem hiding this comment.
🔴 The GC-safety of s_retainedTraces depends on the SamplingProfiler's m_liveCellPointers surviving until the last owner's clearData(), but require('bun:jsc').profile() (functionRunProfiler in src/jsc/modules/BunJSCModule.h:715-716, and its reportFailure twin at :731-732) calls samplingProfiler.pause() + clearData() directly, outside the owner-refcount system. If ≥2 owners are live, one releases (populating s_retainedTraces), then user code calls bun:jsc profile(), clearData() wipes m_liveCellPointers, GC frees a sampled executable, and the remaining owner's buildProfileOutput dereferences a freed cell — before this PR that external clearData() was data-loss only, but s_retainedTraces upgrades it to a use-after-free. functionRunProfiler/reportFailure should route through acquireCPUProfilerOwner/releaseCPUProfilerOwner (or s_retainedTraces must not retain raw cell pointers across GC under an assumption an unrelated public API can violate).
Extended reasoning...
What the bug is
This PR introduces s_retainedTraces — a thread-local Vector<SamplingProfiler::StackTrace> that persists drained traces across calls when more than one profiler owner is live. Each StackFrame inside those traces holds raw ExecutableBase* / JSObject* pointers. The PR's own comment at BunCPUProfiler.cpp:52-57 and the drain site here document the GC-safety invariant: those raw pointers stay live because releaseStackTraces() moves m_stackTraces out but leaves m_liveCellPointers intact, and SamplingProfiler::visit() marks that set during GC — until the last owner calls clearData().
But functionRunProfiler in src/jsc/modules/BunJSCModule.h:712-717 — the user-reachable require('bun:jsc').profile(fn) API — calls samplingProfiler.pause() + samplingProfiler.clearData() directly on the same VM-singleton profiler, entirely outside the new owner-refcount system. Its reportFailure lambda at :727-732 does the same. clearData() clears m_liveCellPointers, breaking the invariant s_retainedTraces depends on.
Code path that triggers it
--cpu-prof(or inspector Session A'sProfiler.start) →acquireCPUProfilerOwner— one owner.- Inspector Session B:
Profiler.start→ second owner. - Session B:
Profiler.stop→releaseCPUProfilerOwner. Not the last owner, so it drainsreleaseStackTraces()intos_retainedTracesand does not callclearData().s_retainedTracesnow holdsStackFrames whoseexecutable/calleecells are rooted only via the profiler'sm_liveCellPointers. - User code calls
require('bun:jsc').profile(fn)→ thereport(orreportFailure) lambda runssamplingProfiler.clearData(), wipingm_liveCellPointers. - GC runs. A short-lived sampled executable (an IIFE, an inline callback that already returned) whose only root was
m_liveCellPointersis collected. - The remaining owner releases (
--cpu-profat process exit, or Session A'sProfiler.stop) →releaseCPUProfilerOwnercallsbuildProfileOutputovers_retainedTraces, which invokesframe.displayName(vm),frame.sourceProviderAndID(), anddynamicDowncast<JSC::FunctionExecutable>(frame.executable)on a freed cell → use-after-free.
Why existing code doesn't prevent it
The DeferGC inside releaseCPUProfilerOwner only covers that single call — it does not span step 3 → step 6. Between those steps, arbitrary JS runs and GC is free to collect. The owner-refcount system is closed: it knows about --cpu-prof, per-Worker profiling, and node:inspector Sessions, but functionRunProfiler is a fourth sibling that touches the same VM-singleton SamplingProfiler and was never wired in. Nothing in BunCPUProfiler.cpp guards against an external clearData().
Why this is new in this PR
Before this PR, stopCPUProfiler held traces only on-stack, under a single DeferGC, then immediately called clearData() itself. An external clearData() from bun:jsc profile() racing with that path caused data loss (an empty profile), never memory corruption. This PR introduces the first cross-call, cross-GC retention of raw StackFrame cell pointers (s_retainedTraces in the !lastOwner branch), which upgrades the pre-existing conflict to a memory-safety bug.
Impact
User-reachable UAF. bun:jsc profile() is public API; --cpu-prof + a node:inspector Session is exactly the concurrent-owner scenario this PR is designed to enable. Per REVIEW.md, memory safety ("Never let a pointer or slice outlive the memory it points into") is the most-blocked category, and "Fix the whole class in the same PR" applies — functionRunProfiler is a sibling site touching the shared SamplingProfiler that the owner-refcount design must account for.
Fix
Route functionRunProfiler (and reportFailure) through Bun::acquireCPUProfilerOwner / Bun::releaseCPUProfilerOwner instead of direct ensureSamplingProfiler/start/pause/clearData. Alternatively, if bun:jsc profile() must keep its own trace-consumption path, have releaseCPUProfilerOwner clear s_retainedTraces whenever it observes m_liveCellPointers may have been cleared externally, or don't retain raw cell pointers past DeferGC at all (e.g. eagerly stringify frame metadata at drain time so s_retainedTraces holds no GC pointers).
| void startCPUProfiler(JSC::VM& vm) | ||
| { | ||
| // Capture the wall clock time when profiling starts (before creating stopwatch) | ||
| // This will be used as the profile's startTime | ||
| s_profilingStartTime = MonotonicTime::now().approximate<WTF::WallTime>().secondsSinceEpoch().value() * 1000000.0; | ||
|
|
||
| // Create a stopwatch and start it | ||
| auto stopwatch = WTF::Stopwatch::create(); | ||
| stopwatch->start(); | ||
|
|
||
| JSC::SamplingProfiler& samplingProfiler = vm.ensureSamplingProfiler(WTF::move(stopwatch)); | ||
| samplingProfiler.setTimingInterval(WTF::Seconds::fromMicroseconds(s_samplingInterval)); | ||
| samplingProfiler.noticeCurrentThreadAsJSCExecutionThread(); | ||
| samplingProfiler.start(); | ||
| s_isProfilerRunning = true; | ||
| if (s_implicitProfilerOwner == 0) | ||
| s_implicitProfilerOwner = acquireCPUProfilerOwner(vm); | ||
| } |
There was a problem hiding this comment.
🔴 The if (!Bun::isCPUProfilerRunning()) Bun::startCPUProfiler(...) guard in JSWorker.cpp (jsWorkerPrototypeFunction_startCpuProfileInternalBody, line 818) was not updated for the new semantics: isCPUProfilerRunning() now returns true when any owner (including an explicit node:inspector Session owner) exists, so worker.startCpuProfile() skips acquiring the implicit owner and handle.stop() then hits the new s_implicitProfilerOwner == 0 early-return, giving the parent kEmptyCpuProfileJSON — a regression from before this PR. Drop the guard (and the mirror at line 843); startCPUProfiler() is now internally idempotent on s_implicitProfilerOwner.
Extended reasoning...
What the bug is
This PR changes the semantics of two helpers in BunCPUProfiler.cpp:
startCPUProfiler()used to mean "unconditionally start the VM sampler"; it now means "acquire the implicit owner (s_implicitProfilerOwner) if not already held".isCPUProfilerRunning()used to read a singles_isProfilerRunningbool; it now returns!s_profilerOwners.isEmpty(), i.e. true when any owner exists — implicit or explicit (anode:inspectorSession that calledProfiler.start).
The header comment at BunCPUProfiler.h:25-27 explicitly names "per-Worker profiling" as an implicit-owner client. But that caller was not updated. src/jsc/bindings/webcore/JSWorker.cpp:818-819 still does:
if (!Bun::isCPUProfilerRunning())
Bun::startCPUProfiler(workerCtx.vm());and the stop side at :843-844:
if (Bun::isCPUProfilerRunning())
Bun::stopCPUProfiler(workerCtx.vm(), &result, nullptr);Under the old single-bool model this guard just avoided a redundant re-start. Under the new model it prevents the Worker's implicit owner from ever being acquired whenever any other owner exists on that thread.
Step-by-step trigger
All profiler state is thread_local and postTaskToWorkerGlobalScope runs the lambda on the worker's JS thread, so a Session inside the worker and the parent's worker.startCpuProfile() share the same s_profilerOwners / s_implicitProfilerOwner.
- Inside a Worker, user code creates a
node:inspectorSession and callsProfiler.start. This routes tojsFunction_startCPUProfiler→acquireCPUProfilerOwner, which appends an explicit owner tos_profilerOwners.s_implicitProfilerOwnerstays0. - The parent calls
worker.startCpuProfile(). The posted task atJSWorker.cpp:818evaluatesisCPUProfilerRunning()→!s_profilerOwners.isEmpty()→ true, so it skipsstartCPUProfiler().s_implicitProfilerOwnerstays0. - The parent later calls
handle.stop(). The posted task atJSWorker.cpp:843seesisCPUProfilerRunning()== true and callsstopCPUProfiler(). - The new
stopCPUProfiler()checkss_implicitProfilerOwner == 0, hits the early-return, and writes*outJSON = WTF::String(). - Back in
JSWorker.cpp:845-846,result.isEmpty()is true, so the parent receiveskEmptyCpuProfileJSON— zero samples.
Why existing code doesn't prevent it
The guard at :818 was correct pre-PR because isCPUProfilerRunning() and startCPUProfiler() operated on the same single bool. Post-PR they operate on different state: the guard checks "any owner" but the call it guards only manipulates the implicit owner. Nothing else on this path touches s_implicitProfilerOwner.
Impact
Before this PR the same sequence returned a populated profile (the old stopCPUProfiler unconditionally drained the shared buffer). After this PR it silently returns an empty stub. This is exactly the Session×implicit-owner interaction the PR is fixing for Session×Session and Session×--cpu-prof, but the per-Worker path — which the PR's own header comment names as an implicit-owner client — is left with a stale guard that defeats it. Per REVIEW.md: "Fix the whole class in the same PR… every caller of a changed helper. Prefer moving the guard into the shared helper."
Fix
Drop both guards — startCPUProfiler() is now internally idempotent on s_implicitProfilerOwner, and stopCPUProfiler() already handles the "not held" case:
// JSWorker.cpp:818
Bun::startCPUProfiler(workerCtx.vm());// JSWorker.cpp:843
Bun::stopCPUProfiler(workerCtx.vm(), &result, nullptr);| // consumed the shared buffer and the second got -32000 "not started"). | ||
| expect(aSamples).toBeGreaterThan(0); | ||
| expect(bSamples).toBeGreaterThan(0); |
There was a problem hiding this comment.
🟡 This is a hard wall-clock upper bound on b.endTime - b.startTime, where B's window is a while (performance.now() - t < 300) busy loop running under test.concurrent alongside two other CPU-spinning subprocesses — if the child is descheduled mid-loop on an oversubscribed CI runner, the loop overshoots and this fails on a correct build. Consider asserting the invariant structurally instead: emit a.startTime/b.startTime and assert b.startTime - a.startTime > 200_000 (a lower bound, which scheduling jitter can only widen) — before this PR the two shared s_profilingStartTime so the difference was 0, after it's ≥ the work(400) between them.
Extended reasoning...
What this asserts
bDurationMicros is b.endTime - b.startTime, where b.startTime is the wall-clock time at B's acquireCPUProfilerOwner() and b.endTime is the wall-clock timestamp of the last sample recorded before B's Profiler.stop. Between those two points the child runs work(300), defined as:
const work = ms => { const t = performance.now(); let x = 0; while (performance.now() - t < ms) x += Math.sqrt(x + 1); };This loop is wall-clock-bounded, so debug/ASAN slowness does not stretch it — it always exits at ~300ms wall time regardless of how slow each iteration is. The flake window is a single OS deschedule: if the child process is preempted for a contiguous ≥250ms between the last performance.now() check and the synchronous stopCPUProfiler inside s.post, the SamplingProfiler's timer thread keeps timestamping samples while the JS thread is parked, so b.endTime advances past 550ms and the assertion fails on a correct build.
Why this shape is flagged
The three test.concurrent cases in this describe block each spawn a subprocess that busy-spins the CPU (work(200)+work(200), work(400)+work(300)+work(150), work(200)), all running simultaneously on a shared CI host. REVIEW.md's test rules call out this shape directly: "Await the actual observable condition … Don't raise per-test timeouts to make a slow test pass" and CLAUDE.md's "You are not testing the TIME PASSING, you are testing the CONDITION". A hard microsecond ceiling on real elapsed time is exactly the pattern those rules warn about.
Step-by-step failure trace
- Three
test.concurrentsubprocesses start together on a 2-core CI runner already running other test files. - In the B-window subprocess,
work(300)enters its loop at t₀. - At t₀+298ms the loop condition check passes (298 < 300); the process is then descheduled.
- The OS gives the quantum to the other two spinning children plus whatever else is on the box; this child gets no CPU for 260ms.
- At t₀+558ms the child is rescheduled, the next
performance.now()check sees 558 ≥ 300 and exits the loop;Profiler.stopruns. - The SamplingProfiler's timer thread was never descheduled with the JS thread, so it kept appending samples during the 260ms gap; the last sample's wall-clock time is ~t₀+558ms.
bDurationMicros ≈ 558_000→toBeLessThan(550_000)fails.
Why existing code doesn't prevent it
The 250ms slack (83% over the 300ms target) is generous — typical Linux CFS timeslices are 1-10ms, so a single 250ms starvation event with only ~3 competing processes is uncommon. This may well never fire in practice, which is why it's a nit. But it's an upper bound on wall-clock time under concurrent CPU contention, and nothing in the test bounds how long a single deschedule can last.
Structural alternative
The invariant being proven is "B's profile does not include A's pre-B window" — i.e., that B got its own startTime rather than inheriting the shared s_profilingStartTime. That can be asserted without a wall-clock ceiling by also emitting a.startTime and b.startTime and checking:
expect(bStartTime - aStartTime).toBeGreaterThan(200_000);Before this PR both sessions shared s_profilingStartTime so the difference was 0; after the fix B's startTime is captured after work(400), so the difference is ≥400ms. This is a lower bound on elapsed wall-clock time, which scheduling jitter can only make larger — it cannot flake on an oversubscribed runner and still discriminates the fixed from unfixed build.
| JSC_DEFINE_HOST_FUNCTION(jsFunction_stopCPUProfiler, (JSGlobalObject * globalObject, CallFrame* callFrame)) | ||
| { | ||
| auto& vm = globalObject->vm(); | ||
| uint32_t owner = callFrame->argument(0).toUInt32(globalObject); | ||
| WTF::String result; | ||
| Bun::stopCPUProfiler(vm, &result, nullptr); | ||
| Bun::releaseCPUProfilerOwner(vm, owner, &result, nullptr); | ||
| return JSValue::encode(jsString(vm, result)); |
There was a problem hiding this comment.
🟡 toUInt32(globalObject) is a coercion that can throw, but there's no DECLARE_THROW_SCOPE / RETURN_IF_EXCEPTION around it — REVIEW.md's exception-check rule requires one (or scope.assertNoException() with a comment when the argument is provably a number), and the sibling jsFunction_setCPUSamplingInterval right below follows the convention. In practice this can't trip the validator because the only caller (the builtin src/js/node/inspector.ts) always passes a plain Int32 and toUInt32 takes the isInt32() fast path without constructing a ThrowScope, so it's just a hygiene fix.
Extended reasoning...
What the finding is
jsFunction_stopCPUProfiler now reads its argument via callFrame->argument(0).toUInt32(globalObject) without a DECLARE_THROW_SCOPE in scope and without RETURN_IF_EXCEPTION afterwards. REVIEW.md's memory-safety section is explicit about this pattern:
Every call that can throw or run user code (toString/toNumber, getIfPropertyExists, getIndex, coercions, callbacks) needs RETURN_IF_EXCEPTION under a ThrowScope (C++) … before its result is used. … Verify with
BUN_JSC_validateExceptionChecks=1instead of adding suppression-list entries; for calls that provably cannot throw, use scope.assertNoException() with a comment.
toUInt32 is a coercion — in the general case it dispatches to toNumber/toNumberSlowCase, which declare their own ThrowScope and can throw. The sibling function jsFunction_setCPUSamplingInterval in the same file follows the discipline (declares a scope, checks after validateInteger), and elsewhere in the codebase (JSNodePerformanceHooksHistogramPrototype.cpp, NodeFSStatBinding.cpp) toInt32/toUInt32 calls are consistently paired with a ThrowScope and RETURN_IF_EXCEPTION.
Why this is a nit, not a runtime bug
The only caller of this host function is the builtin src/js/node/inspector.ts, which always passes this.#cpuProfilerOwner. That field is initialized to 0 and only ever assigned the return value of startCPUProfiler(), which is jsNumber(uint32_t) — a boxed Int32. JSValue::toUInt32 → toInt32 hits the if (isInt32()) return asInt32(); fast path, which returns without ever entering toNumberSlowCase and therefore without constructing any ThrowScope. Because no inner scope is created on the reachable path, BUN_JSC_validateExceptionChecks=1 has nothing to reconcile and will not abort here in CI.
There is also precedent for the no-scope pattern on internal-only $newCppFunction callees whose argument is provably a number — BunDebugger.cpp's jsFunctionCreateConnection does the same argument(0).toUInt32(globalObject) without a scope.
Step-by-step trace
Profiler.start→this.#cpuProfilerOwner = startCPUProfiler()→jsFunction_startCPUProfilerreturnsJSValue::encode(jsNumber(owner))whereowneris auint32_t.jsNumber(uint32_t)yields an Int32-tagged JSValue for values ≤ INT32_MAX (owner IDs start at 1 and increment).Profiler.stop/Profiler.disable/disconnect()callstopCPUProfiler(this.#cpuProfilerOwner).jsFunction_stopCPUProfilerexecutescallFrame->argument(0).toUInt32(globalObject). The argumentisInt32(), sotoInt32returnsasInt32()immediately — no ThrowScope constructed, no exception possible.- Because no nested ThrowScope was created and no exception was set, the missing outer scope/check is unobservable at runtime and to the exception-check validator.
Impact
None at runtime on any reachable path. This is a coding-discipline violation against the repo's own review rules, and a divergence from the sibling function's style in the same file. It becomes a real footgun only if a future caller (or a refactor of the JS side) passes something other than a plain number.
Suggested fix
Add the two lines to match the sibling and the REVIEW.md rule:
JSC_DEFINE_HOST_FUNCTION(jsFunction_stopCPUProfiler, (JSGlobalObject * globalObject, CallFrame* callFrame))
{
auto& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);
uint32_t owner = callFrame->argument(0).toUInt32(globalObject);
RETURN_IF_EXCEPTION(scope, {});
WTF::String result;
Bun::releaseCPUProfilerOwner(vm, owner, &result, nullptr);
return JSValue::encode(jsString(vm, result));
}Alternatively, since the argument is provably an Int32 from a trusted builtin, scope.assertNoException() with a one-line comment is also acceptable per REVIEW.md.
2 participants
Problem
BunCPUProfilerwraps the VM-singletonJSC::SamplingProfilerbehind a thread-locals_isProfilerRunningbool with no notion of who started it. That makes everynode:inspectorSession and the--cpu-profCLI flag fight over one shared instance:Session.disconnect()checked the global flag and unconditionally ranpause()+releaseStackTraces()+clearData(), so a bareconnect()/disconnect()emptied the buffer that--cpu-profwrites at exit, leaving astartTime == endTimestub with no samples.Profiler.disabledid the same.Profiler.starton a second Session silently adopted the already-running buffer (and the shareds_profilingStartTime, so its profile carried pre-start samples);Profiler.stopon the first Session to stop consumed everyone's data and every later stop got-32000 Profiler is not started.Separately,
Session.post()without a callback threw the Error /kProtocolErrorresult returned by#handleMethod, while Node'spost()without a callback is fire-and-forget and drops error responses.Repro (all cells pass on Node, all but the control fail on Bun before this change):
Fix
Owner-refcount the VM profiler (
BunCPUProfiler.cpp). Each caller acquires an owner token that records its own monotonic start timestamp and wall-clockstartTime. The sampler starts on the 0→1 owner transition and keeps running while any owner is live. Releasing an owner drainsreleaseStackTraces()into a retained trace buffer, builds that owner's profile from traces withtimestamp >= owner.start(so a late-starting Session reports only its own window), and only the last owner to release pauses and clears. The retained buffer is trimmed to the earliest remaining owner's start between releases; the executables it references stay GC-live via the profiler'sm_liveCellPointersuntil the finalclearData(). The CLI--cpu-profpath and per-Worker profiling keep their existingstartCPUProfiler/stopCPUProfilerentry points via a single implicit owner held in their stead.node:inspector's Session now holds a per-session owner token (#cpuProfilerOwner).Profiler.startacquires one,Profiler.stop/Profiler.disable/disconnect()release only that session's claim.post()without a callback now drops command failures (method-not-found,-32000protocol errors) and returnsundefined, matching Node's fire-and-forget contract. The pre-dispatch validation throws (ERR_INVALID_ARG_TYPEfor method/params/callback,ERR_INSPECTOR_NOT_CONNECTED) are unchanged; Node throws those synchronously too.Verification
New tests in
test/js/node/inspector/inspector-profiler.test.ts:a bare Session connect/disconnect does not clear --cpu-prof's samplesconcurrent Sessions each get a profile for their own window(both non-empty, second session's duration bounded by its own window)Profiler.disable on one Session does not stop another Session's profilepost() without a callback drops command failures instead of throwing/still throws for argument validation and not-connectedAll fail on main (
-32000 Profiler is not started, B duration ~700ms for a 300ms window,ERR_INSPECTOR_COMMANDthrown) and pass with this change. Existingcpu-prof.test.tsand inspector tests still pass.