Unpin ArrayBuffers without classInfo() so blob finalizers are safe during GC sweep - #37008
Unpin ArrayBuffers without classInfo() so blob finalizers are safe during GC sweep#37008robobun wants to merge 1 commit into
Conversation
WalkthroughThe PR updates ChangesBuffer extraction and GC safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 2
🤖 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 `@test/js/bun/s3/s3-stream-error-gc.test.ts`:
- Around line 10-15: Add plain Uint8Array coverage in the loop of the S3 stream
error GC test by passing a Uint8Array instance to the relevant file
constructor(s), alongside the existing Buffer and DataView cases, so the
non-Buffer typed-array path is exercised.
- Around line 29-39: Update the subprocess assertion in the GC test to stop
snapshotting or requiring an empty stderr value, while continuing to drain
stderr. Assert only the normalized stdout and exitCode, preserving the existing
expected values and success behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0027aa8-9157-4da9-835d-5a0eec7271ae
📒 Files selected for processing (2)
src/jsc/bindings/bindings.cpptest/js/bun/s3/s3-stream-error-gc.test.ts
| expect({ | ||
| stdout: normalizeBunSnapshot(stdout), | ||
| stderr: normalizeBunSnapshot(stderr), | ||
| exitCode, | ||
| }).toMatchInlineSnapshot(` | ||
| { | ||
| "exitCode": 0, | ||
| "stderr": "", | ||
| "stdout": "ok", | ||
| } | ||
| `); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not require empty stderr in this GC subprocess test.
Bun.gc(true) can produce benign debug or ASAN diagnostics. The stderr: "" snapshot can fail although the regression succeeds. Continue to drain stderr, but assert stdout and exitCode only.
Based on learnings: avoid stderr: "" assertions in aggressive-GC subprocess tests.
Proposed assertion change
- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+ const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({
stdout: normalizeBunSnapshot(stdout),
- stderr: normalizeBunSnapshot(stderr),
exitCode,
}).toMatchInlineSnapshot(`
{
"exitCode": 0,
- "stderr": "",
"stdout": "ok",
}
`);🤖 Prompt for 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.
In `@test/js/bun/s3/s3-stream-error-gc.test.ts` around lines 29 - 39, Update the
subprocess assertion in the GC test to stop snapshotting or requiring an empty
stderr value, while continuing to drain stderr. Assert only the normalized
stdout and exitCode, preserving the existing expected values and success
behavior.
Source: Learnings
|
Addressed the review in 438c856:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // | ||
| // JSType + static_cast instead of dynamicDowncast/possiblySharedBuffer(): | ||
| // unpin is reached from drop paths that can run inside a GC sweep (a Blob | ||
| // store releasing a pinned path buffer in its finalizer), where | ||
| // JSCell::classInfo() asserts. Same pattern as JSC::Weak<T>::get(). A pinned | ||
| // view is always Wasteful/DataView (pinning materialized its buffer), so the | ||
| // sweep-reachable branches below are plain field/butterfly reads. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Fast/Oversize: materializes the buffer (allocates). Only reached at | ||
| // pin time, during a host call. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Updated 1:44 PM PT - Aug 16th, 2026
✅ @robobun, your commit 4a0a28afc4b460cf4958c033e3991d29e521d60d passed in 🧪 To try this PR locally: bunx bun-pr 37008That installs a local version of the PR into your bun-37008 --bun |
|
Shortened the arrayBufferImpl comments in d4bff11. The code itself is the fix rather than a workaround (classInfo-free impl resolution, the pattern JSC uses for finalizer-reachable code), so the block now states just the one constraint that matters: unpin runs from finalizers during GC sweep, where classInfo() is forbidden. Rebuilt and re-verified the repro and the regression test. |
| // | ||
| // JSType + static_cast, not dynamicDowncast: unpin runs from finalizers | ||
| // during GC sweep, where JSCell::classInfo() (reached by every debug cast | ||
| // assert) is forbidden. Same pattern as JSC::Weak<T>::get(). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Tightened it again in the latest commit: the note is now two lines inside the function body, stating only the constraint (finalizers unpin during GC sweep, where classInfo() is off limits). I want to keep those two lines: without them the natural cleanup is to fold this back into dynamicDowncast, which reintroduces the fuzzer crash this PR fixes. |
| // JSType + static_cast, not dynamicDowncast: finalizers unpin during GC | ||
| // sweep, where JSCell::classInfo() is forbidden (as in JSC::Weak<T>::get()). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this touches finalizer-reachable JSC binding code whose correctness rests on GC-internal invariants (what's safe during sweep, when a pinned view is guaranteed wasteful), so it's worth a human look from someone JSC-familiar.
Checked: JSType dispatch covers the same set as the old dynamicDowncast<JSArrayBufferView> (DataView handled separately; isTypedArrayType excludes DataView so no overlap); JSDataView::possiblySharedBuffer() is a field read, not the base-class impl; the non-wasteful fallback is only reachable at pin time since pinning materializes the buffer to wasteful mode; test follows harness conventions and mirrors the sibling GC test's assertion shape.
Extended reasoning...
Overview
Rewrites arrayBufferImpl() in src/jsc/bindings/bindings.cpp to resolve the backing ArrayBuffer* via JSType + static_cast instead of dynamicDowncast, so unpinArrayBuffer can run from a blob finalizer during GC sweep without tripping validateIsNotSweeping (debug-only classInfo() assertion). Adds a subprocess GC-sweep regression test covering Buffer/Uint8Array/DataView/ArrayBuffer paths for both Bun.file and S3Client.file.
Security risks
None. No user-facing surface change; release-build codegen is equivalent to the old downcasts.
Level of scrutiny
High. This is native JSC binding code on a GC-finalizer path. The fix is small and the reasoning in the PR body is thorough, but correctness depends on JSC internals: that JSDataView defines its own possiblySharedBuffer() hiding the base-class one (so no uncheckedDowncast → classInfo()), that isWastefulTypedArray(mode) covers every mode a pinned view can be in at unpin time, and that reading the butterfly indexing header is safe mid-sweep. I traced these and they check out against how possiblySharedBufferImpl itself dispatches, but a JSC-familiar maintainer should confirm — the repo review guide flags this exact area ("JSC GC reference … use-after-free, collected too early, finalize") as needing expert eyes.
Other factors
- The comment-cop bot is still flagging the two-line in-body comment on the latest commit. The author's rationale for keeping it (prevents someone folding this back into
dynamicDowncast) is sound and the comment states a non-obvious constraint the code can't express, so I'd treat that as a linter false-positive — but a human should make that call. - One CodeRabbit note (drop the
stderr: ""snapshot) is unresolved; the author's response (bunEnv quiets debug logs, sibling test asserts the same shape, stderr surfaces the actual assertion text on regression) is reasonable and matches the existing test in the file. - CI build #89406 was still in progress at review time.
|
CI status: the remaining red on build 89419 is an infrastructure failure, the debian 13 x64 test lane timed out downloading the build artifact ("buildkite-agent artifact download timed out after 120s"), so its tests never ran. The other two entries passed on retry. The previous run's failures were also unrelated flakes across install, webview, napi, and bundler tests. The new GC sweep test passed on every lane that executed it. Ready for review; the failing lane just needs a retrigger. |
There was a problem hiding this comment.
I didn't find any bugs, but this reworks how arrayBufferImpl resolves the backing buffer on a path reachable from GC finalizers, and its correctness leans on JSC-internal invariants (JSDataView's shadowing possiblySharedBuffer() being a plain field read, and pinned typed arrays always being in a wasteful mode so the allocating fallthrough is never reached during sweep). Worth a quick look from someone with JSC/GC context.
Checked: JSType dispatch covers the same set as the old dynamicDowncast pair (ArrayBuffer, DataView, typed arrays) with no behavioral change for pin-time callers; cell->type()/view->mode()/butterfly indexing-header reads are field loads, not classInfo() consumers. The regression test is hermetic, drains both pipes, and the mechgate evidence shows it fails on unfixed debug and passes with the fix.
Extended reasoning...
Overview
The PR rewrites the static helper arrayBufferImpl() in src/jsc/bindings/bindings.cpp (used by JSC__JSValue__pinArrayBuffer / unpinArrayBuffer) to resolve a JSC::ArrayBuffer* without going through dynamicDowncast or JSArrayBufferView::possiblySharedBufferImpl, both of which touch JSCell::classInfo() in debug builds. The unpin path is reachable from ~JSS3File / ~JSBlob finalizers during Heap::finalize (mutator state == Sweeping), where classInfo() is asserted against by validateIsNotSweeping(). The new code dispatches on cell->type() and uses static_cast; for wasteful typed arrays it reads the buffer straight out of butterfly()->indexingHeader()->arrayBuffer(). A subprocess regression test is added covering Buffer/Uint8Array/DataView/ArrayBuffer paths through both Bun.file and S3Client.file under forced GC.
Security risks
None. This is an internal impl-resolution helper; no user-facing API surface, parsing, or trust boundary changes. Release-build behavior is claimed unchanged (the old downcasts already compiled to these type checks).
Level of scrutiny
High. This is finalizer-reachable code in the JSC bindings — the repo's most-blocked review category. Correctness depends on JSC-internal invariants: that JSDataView::possiblySharedBuffer() (resolved via the static type, shadowing the base-class version) is a plain m_buffer read; that a pinned typed array is always WastefulTypedArray (because pin materialized the buffer via possiblySharedBuffer()), so the allocating fallthrough on line 3252 is genuinely unreachable during sweep; and that the butterfly indexing-header read is safe while the owning cell is being finalized. These all look right to me against upstream JSC, but they're exactly the kind of thing a JSC maintainer should confirm rather than a bot.
Other factors
- The mechgate evidence in the PR body demonstrates the new test fails on unfixed debug (
ASSERTION FAILED: ... MutatorState::Sweeping, exit 134) and passes with the fix, and passes on release either way — so the test is load-bearing on the lanes where the bug manifests. - CodeRabbit's two suggestions were handled: plain
Uint8Arraycases were added; thestderr: ""snapshot was intentionally kept with a reasonable justification (bunEnv quiets debug logs, the sibling test in the same file uses the identical shape and is CI-stable, and the assertion text lands on stderr so the snapshot diff surfaces the actual failure). - The comment-cop bot keeps re-flagging the two-line note at line 3237-3238. The author trimmed it twice and argued for keeping it; I agree — it names the one non-obvious constraint that would otherwise invite a "cleanup" back to
dynamicDowncast, and it's two lines, not a paragraph. But that's a maintainer call. - Deferring rather than approving because GC finalizer paths in JSC bindings warrant a human sign-off from someone with JSC internals context.
3ef0503 to
a117e49
Compare
|
Rebased onto current main (9fcdea8), no conflicts. The fuzzer has since minimized this fingerprint down to a deterministic repro that matches the root cause described above: Bun.file(Buffer.from("e"));
Bun.gc(true);On current main without this patch the debug build aborts on every run (10 of 10 plain, 3 of 3 with |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The JSType-dispatch rewrite of arrayBufferImpl looks correct and the fails-before/passes-after evidence is solid, but since this changes finalizer-reachable JSC code and relies on a non-obvious invariant (pinned views are always wasteful/DataView by unpin time, so the allocating fallthrough is unreachable during sweep), a human look would still be worthwhile.
What was reviewed:
- The JSType + static_cast dispatch mirrors what release builds already compiled the old
dynamicDowncastcalls to; debug-only behavior change. - DataView branch reads
JSDataView::possiblySharedBuffer()(own field accessor, not the parent'spossiblySharedBufferImplwith itsuncheckedDowncast); the DataView case is exercised in the test loop. - Wasteful-mode branch reads
butterfly()->indexingHeader()->arrayBuffer()— same asexistingBufferInButterfly(), no classInfo(). - Test covers Buffer, plain Uint8Array, DataView, and ArrayBuffer for both
Bun.fileandS3Client.file; the siblingstderr: ""snapshot pattern is already stable in this file.
Extended reasoning...
Overview
This PR rewrites arrayBufferImpl() in src/jsc/bindings/bindings.cpp (~17 net lines) to resolve a JSC::ArrayBuffer* from a JSValue without touching JSCell::classInfo(), and adds a subprocess regression test in test/js/bun/s3/s3-stream-error-gc.test.ts. The old code used dynamicDowncast<JSArrayBuffer> / dynamicDowncast<JSArrayBufferView>, which in debug builds cross-checks the JSType against classInfo() — forbidden while mutatorState() == Sweeping. Since unpinArrayBuffer is called from blob-store finalizers during the sweep, this tripped validateIsNotSweeping in debug/ASAN builds.
The new code branches on cell->type() and uses static_cast: JSArrayBuffer::impl() for ArrayBufferType, JSDataView::possiblySharedBuffer() (a plain m_buffer field read on the DataView subclass) for DataViewType, and for typed arrays either a butterfly indexing-header read when the mode is wasteful, or the allocating possiblySharedBuffer() otherwise. The PR notes the allocating path is unreachable from sweep because pinning at borrow time already materialized the buffer (converting the view to wasteful mode).
Security risks
None. This is a debug-assertion fix in an internal helper; release builds already compiled the old downcasts to the same JSType checks. No user-controlled input reaches new parsing or allocation logic.
Level of scrutiny
High — this is GC-finalizer-reachable code in the JSC bindings layer, and correctness depends on JSC internals (TypedArray mode transitions, butterfly layout, which possiblySharedBuffer overload gets called for a static JSDataView*). The change is small and the PR body walks through the mechanism precisely, but the "unreachable during sweep since pinning materialized it" invariant on the fallthrough branch is the kind of claim a maintainer with JSC depth should confirm. If that invariant were ever violated (e.g., a future caller unpins a value that was never pinned, or a mode transition doesn't happen), the fallthrough would allocate during sweep.
Other factors
- The PR body includes fails-on-main / passes-on-PR evidence for both debug-ASAN and release, and the fuzzer's minimized repro matches the test fixture.
- CodeRabbit's Uint8Array-coverage suggestion was addressed; the
stderr: ""snapshot suggestion was declined with a reasonable justification (bunEnv setsBUN_DEBUG_QUIET_LOGS=1, the sibling test in the same file uses the identical shape, and the snapshot surfaces the actual assertion text on regression). - The comment-cop bot fired repeatedly on the inline comment; the author trimmed it to two lines stating only the constraint, which seems worth keeping to prevent a future "cleanup" back to
dynamicDowncast. - No prior claude[bot] reviews on this PR.
|
CI is green on the rebased head (build 91849, 190 of 190 jobs passed), so the earlier infra-timeout note is stale. Also re-checked against today's main (cd15fba): the debug build still aborts on |
…ring GC sweep Blob stores for Bun.file(bufferPath) and S3Client.file(bufferKey) keep the pinned path buffer until the store drops, which happens in the JS wrapper's finalizer during the sweep phase. JSC__JSValue__unpinArrayBuffer re-derived the JSC::ArrayBuffer* with dynamicDowncast and possiblySharedBuffer(), both of which call JSCell::classInfo() in debug builds and trip the validateIsNotSweeping assertion (JSCell.cpp:179). Dispatch on the cell's JSType with static_cast instead, reading a DataView's buffer field directly and a wasteful view's buffer out of the butterfly indexing header, like JSC::Weak<T>::get() does for finalizer contexts. A view whose mode has no ArrayBuffer (Fast/Oversize, which pinStorage reports as None/Held) stays a no-op, matching the previous hasArrayBuffer() check. Release builds already compiled the old downcasts to the same type checks, so behavior there is unchanged.
a117e49 to
4a0a28a
Compare
| // Reached from finalizers during GC sweep, where classInfo() (and so any | ||
| // dynamicDowncast) is forbidden; dispatch on JSType like JSC::Weak<T>::get(). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
robobun
changed the title
|
Rebased onto current main (07d38c1). The conflict was real: main has since split the pin side into |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewrites finalizer-reachable JSC buffer resolution against internal invariants (JSDataView's possiblySharedBuffer() being a plain field read, pinned typed arrays always being in a wasteful mode), a human familiar with the WebKit side would be worth having sign off.
What was reviewed:
- Confirmed the JSType dispatch covers everything the old
dynamicDowncastpath matched (ArrayBuffer, DataView, all typed array types) and that unpin's precondition ("only called forPinned") guarantees the wasteful-mode check is sufficient —pinStoragematerializes the buffer viapossiblySharedBuffer()before returningPinned. - Checked that
static_cast<JSDataView*>resolves to JSDataView's ownpossiblySharedBuffer()(them_bufferread), not the base-class path with theuncheckedDowncastassert. - Test follows harness conventions (subprocess, drains both pipes, snapshot on combined object) and demonstrably fails on unpatched debug builds per the gate evidence.
Extended reasoning...
Overview
The PR changes ~15 lines in JSC__JSValue__unpinArrayBuffer (src/jsc/bindings/bindings.cpp) to resolve the backing ArrayBuffer* via JSCell::type() + static_cast instead of dynamicDowncast, because the latter reaches classInfo() in debug builds and classInfo() asserts when called during a GC sweep. The unpin path is reached from Blob/S3File finalizers, which run inside the sweep. A subprocess regression test is added that exercises Buffer/Uint8Array/DataView/ArrayBuffer paths through both Bun.file and S3Client.file under forced GC.
Security risks
None. This is a debug-assertion fix; the PR description states (and the shape of the change is consistent with) release builds already compiling the old downcasts to the same JSType checks. No new user-controlled input reaches this code.
Level of scrutiny
High. This is finalizer-reachable C++ in the JSC bindings, and correctness depends on WebKit-internal invariants I could not verify from headers in this checkout: that JSDataView::possiblySharedBuffer() shadows the base-class method with a plain m_buffer read, that isWastefulTypedArray() covers every mode a pinned view can be in (including resizable/growable-shared wasteful variants), and that butterfly()->indexingHeader()->arrayBuffer() is a pure field-chain read. The reasoning in the PR description is sound and matches known JSC patterns (it cites JSC::Weak<T>::get() as precedent), and CI is green across 190 jobs, but a maintainer who works in the WebKit fork should confirm those invariants.
Other factors
- The gate evidence shows the new test aborting on unpatched debug (
JSCell.cpp(179)assertion) and passing with the fix, and passing on release either way — the test is not vacuous. - The comment-cop bot fired again on the current 2-line comment; the author already justified keeping it (without it, the natural refactor back to
dynamicDowncastreintroduces the crash). I don't consider that blocking. - One CodeRabbit note about not asserting empty stderr remains open; the author's response (bunEnv sets
BUN_DEBUG_QUIET_LOGS=1, sibling test in the same file uses the same shape, and stderr carries the assertion text on regression) is reasonable.
1 participant
Crash
Fuzzilli found a flaky abort in debug builds, fingerprint
JSCell.cpp(179):The fuzzer's minimized script did not reproduce on its own (the crash depends on what earlier programs in the same REPRL child left on the heap; every fuzzed program ends with
Bun.gc(true), which is what runs the sweep). Replaying the fuzzer's API surface with ill-typed arguments reproduced it, and this two-liner then crashes a debug build deterministically:Root cause
Bun.file(path)andS3Client.file(key)acceptBuffer, typed array, andArrayBufferpaths.PathLike::from_jspins the backingJSC::ArrayBufferso a transfer cannot move the bytes while native code borrows them, and the blobStorekeeps that pinnedPathLikefor its whole lifetime. The store is released from the JS wrapper's finalizer, i.e. inside the GC sweep (forJSS3File, a precise allocation, that isHeap::finalizeon every collection). Frame-pointer walk at the assertion:dynamicDowncastasserts the JSType check againstclassInfo()in debug builds, andpossiblySharedBufferImplhas anuncheckedDowncast<JSDataView>doing the same, so any unpin that runs during a sweep trips the assertion.Fix
JSC__JSValue__unpinArrayBuffernow resolves the buffer withoutclassInfo(): dispatch on the cell'sJSTypewithstatic_cast, read a DataView's buffer field directly, and read a wasteful view's buffer out of the butterfly indexing header. This is the same approachJSC::Weak<T>::get()uses for code reachable from finalizers. A view whose mode has no ArrayBuffer (fast or oversize, whichpinStoragereports asNone/Held) stays a no-op, exactly as the previoushasArrayBuffer()check did, since that mode bit is set precisely for the wasteful and DataView modes.Release builds already compiled the old downcasts to these exact type checks, so non-debug behavior is unchanged.
Rebase note: main has since split pinning into
pinStorage()/PinKindwith a separate unpin function, so the originalarrayBufferImpl()change no longer applied. The pin side runs inside host calls, wheredynamicDowncastis fine, so the fix is now confined to the unpin function, the only one reachable from a sweep. Re-verified on the rebased main: a debug build aborts onnew Bun.S3Client({}).file(Buffer.from("k")); Bun.file(Buffer.from("/tmp/x")); Bun.gc(true)without the patch and exits cleanly with it, and the test in this PR passes.Verification
Bun.fileandS3Client.file) crashed before and pass now, including withBUN_JSC_sweepSynchronously=1.bun bd testpasses for the new test plusbun-write.test.js,zlib.test.js,compression.test.ts, andfs.test.ts(the other users of the pin/unpin path).[review] gate passed · iteration 3 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 3
evidence per changed file