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
Code review found 2 important issues
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/jsc/bindings/BunCPUProfiler.cpp:973-982 |
s_retainedTraces UAF: bun:jsc profile() clears m_liveCellPointers while retained traces still reference cells |
| 🔴 Important | src/jsc/bindings/BunCPUProfiler.cpp:89-93 |
Stale isCPUProfilerRunning() guard in JSWorker.cpp defeats implicit-owner acquisition |
| 🟡 Nit | test/js/node/inspector/inspector-profiler.test.ts:471-473 |
Wall-clock upper-bound assertion bDurationMicros < 550_000 is a potential CI flake |
| 🟡 Nit | src/jsc/bindings/JSInspectorProfiler.cpp:27-33 |
Missing ThrowScope/RETURN_IF_EXCEPTION after toUInt32() in jsFunction_stopCPUProfiler |
Annotations
Check failure on line 982 in src/jsc/bindings/BunCPUProfiler.cpp
claude / Claude Code Review
s_retainedTraces UAF: bun:jsc profile() clears m_liveCellPointers while retained traces still reference cells
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()`, `clearDat
Check failure on line 93 in src/jsc/bindings/BunCPUProfiler.cpp
claude / Claude Code Review
Stale isCPUProfilerRunning() guard in JSWorker.cpp defeats implicit-owner acquisition
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 `kEmptyCpuProfil
Check warning on line 473 in test/js/node/inspector/inspector-profiler.test.ts
claude / Claude Code Review
Wall-clock upper-bound assertion bDurationMicros < 550_000 is a potential CI flake
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, whi
Check warning on line 33 in src/jsc/bindings/JSInspectorProfiler.cpp
claude / Claude Code Review
Missing ThrowScope/RETURN_IF_EXCEPTION after toUInt32() in jsFunction_stopCPUProfiler
`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`