ffi: don't free caller-owned memory in toBuffer without a finalizer - #36521
Conversation
`bun:ffi.toBuffer(ptr, offset, len)` without an explicit finalizer fell into `JSValue::create_buffer`, which hard-codes `MarkedArrayBuffer_deallocator`. For a borrowed pointer (from `ptr(buffer)` or a dlopen'd symbol) that `mi_free`s storage this Buffer does not own when it is collected: an ASAN bad-free, and a SIGSEGV on release builds (reliably on Windows/macOS where mimalloc override is off). Install a no-op deallocator on the no-finalizer path so the Buffer borrows the pointer instead of freeing it on GC, matching `toArrayBuffer`'s existing behavior. The zero-copy view is preserved; an explicit finalizer still controls disposal and runs exactly once. Tests cover the bad-free (offset 0, interior offset, typed-array source) via subprocess, red on the unpatched build and asserting the caller's memory stays valid, plus a regression that an explicit finalizer is still called exactly once on GC. The `primitives` fixture test drops its no-op deallocator workaround and now exercises the real default path on static native storage. Fixes #35405 Closes #31753 Co-authored-by: Steven Zimmerman <15812269+EffortlessSteven@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 2 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 (1)
WalkthroughChangesFFI borrowed-buffer ownership
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced via All review threads addressed. |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Re the issue-finder suggestions:
|
|
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}
…r the finalizer guard
The subprocess tests now assert the full {stdout, stderr, exitCode} object so a
failure diff carries the child's crash output instead of a bare empty-string
mismatch, matching the rest of the file.
The explicit-finalizer regression guard switches from cc() (TinyCC) to the
compiled ffi-test fixture's getDeallocatorCallback/getDeallocatorBuffer/
getDeallocatorCalledCount helpers. Every in-process cc() invocation in the
repo is gated on isASAN, and the debian x64-asan lane reported a
generate_symbols leak from this one. The fixture-based test runs under ASAN
and the helpers already reset the counter, so no isolation is lost.
Co-authored-by: Steven Zimmerman <15812269+EffortlessSteven@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Co-authored-by: Steven Zimmerman <15812269+EffortlessSteven@users.noreply.github.com>
- make the three independent subprocess tests `it.concurrent` - the explicit-finalizer GC poll is now async with a yield between collections and a 100-iteration ceiling, matching the repo's gcUntil shape - drop the incorrect 'wedges in the crash handler' parenthetical; the unpatched child exits non-zero promptly (139 release / ASAN abort), so the toEqual mismatch is the red signal Co-authored-by: Steven Zimmerman <15812269+EffortlessSteven@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/runtime/ffi/FFIObject.rs`:
- Around line 739-748: Update the toBuffer API documentation associated with
JSBuffer__bufferFromPointerAndLengthAndDeinit to specify that omitting the
finalizer borrows caller-owned memory while a supplied finalizer releases it,
and explicitly document that zero-length inputs invoke the non-null finalizer
immediately. Also document that toArrayBuffer without a callback does not free
the supplied bytes.
In `@test/js/bun/ffi/ffi.test.js`:
- Around line 1375-1377: Measure the four GC subprocess tests under the debug
build, following test/CLAUDE.md. Remove GC_TIMEOUT and use the default test
timeout if the measured runtime fits; otherwise reduce the 20-iteration gcLoop
work until it does, without retaining a per-test timeout.
- Around line 1379-1385: Trim the paragraph-length comments in the affected test
blocks, including the block near the post-condition checks and the corresponding
comments near lines 1357 and 1452. Retain only the durable, non-obvious fact and
the issue URL; remove narrative explanations already conveyed by the assertion
messages.
- Around line 1436-1450: Update the test case around toBuffer and
originalSurvives to assert that adopted aliases the caller-owned typed array
before the GC survival check. Add assertions that verify the shared memory and
expected byte values, while preserving the existing “survived-gc” assertion and
cleanup behavior.
🪄 Autofix (Beta)
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: 2e3f9873-c0d4-46b8-a2e3-58bcc9795575
📒 Files selected for processing (3)
src/runtime/ffi/FFIObject.rstest/js/bun/ffi/ffi-test.ctest/js/bun/ffi/ffi.test.js
💤 Files with no reviewable changes (1)
- test/js/bun/ffi/ffi-test.c
… in the typed-array case Measured debug+ASAN runtime is ~1s per subprocess test (concurrent), well under the default budget. Co-authored-by: Steven Zimmerman <15812269+EffortlessSteven@users.noreply.github.com>
There was a problem hiding this comment.
I didn't find issues with the latest revision — the earlier review nits (ASAN/TinyCC guard, combined-object assertions, comment length, aliasing check, per-test timeout) are all addressed at 032d4b9. Deferring to a human because this changes memory-ownership semantics on a public bun:ffi path; the fix looks right, but that's the repo's most-scrutinized category.
What was reviewed:
callback.or(Some(noop_bytes_deallocator))preserves the explicit-finalizer path and only substitutes onNone;ctxhandling is unchanged.- Confirmed
JSBuffer__bufferFromPointerAndLengthAndDeinitasserts a non-null deallocator forlen > 0(JSBuffer.cpp:392), so a real no-op fn is required —Nonewould trip the debug assert. - Behavior now matches the sibling
to_array_buffer, which already borrows without freeing when no finalizer is given. - Removing
getNoopDeallocatorCallbackfrom the fixture is safe — its only consumer was theprimitivestest, which now exercises the real no-finalizer path.
Extended reasoning...
Overview
The PR changes to_buffer in src/runtime/ffi/FFIObject.rs so that when toBuffer(ptr, off, len) is called without a finalizer, the returned Buffer installs a no-op bytes deallocator instead of falling through to JSValue::create_buffer (which hard-codes MarkedArrayBuffer_deallocator → mi_free). The old path freed caller-owned FFI memory on GC, causing SIGSEGV/ASAN bad-free (#35405, #24160). The change is ~10 effective Rust lines plus a new no-op extern "C" fn; the rest is test coverage (3 subprocess crash-repro tests, 1 explicit-finalizer regression guard) and removing the fixture's now-unneeded getNoopDeallocatorCallback workaround.
Security risks
None introduced. FFI is inherently trust-the-caller; this change removes a case where Bun freed memory it didn't own. The only conceivable regression is a leak for a caller who was (undocumented, Linux-only) relying on the auto-mi_free — but that same call crashes on Windows/macOS today, and toArrayBuffer already borrows, so borrow is the correct contract.
Level of scrutiny
High. This is native memory-ownership code in a public API surface. Per the repo's review guide, memory safety is the most-blocked category and "every allocation has exactly one named owner, released exactly once" is the governing rule. The fix is small and I believe correct — .or(Some(noop)) is the minimal delta, the explicit-finalizer path is provably unchanged (Some(cb).or(...) == Some(cb)), and the C++ side's ASSERT(bytesDeallocator) at JSBuffer.cpp:392 confirms a real fn pointer is required rather than None. But an ownership-semantics change on bun:ffi should get a maintainer's eyes.
Other factors
All prior review feedback has been addressed at 032d4b9: my two inline nits (TinyCC/ASAN, combined-object subprocess assertions), the comment-cop paragraph-comment flags, and CodeRabbit's aliasing-assertion / comment-trim / GC_TIMEOUT remarks. The finalizer regression guard now uses the compiled fixture via dlopen (no in-process TinyCC), and the three subprocess tests are it.concurrent. This PR also overlaps with the larger omnibus #31449, which the author has already noted; a maintainer may want to coordinate landing order.
There was a problem hiding this comment.
My earlier feedback (TinyCC/ASAN gate, combined-object assertions) has been addressed, and I found nothing new to flag. Deferring to a human since this changes memory-ownership semantics of a public FFI API in native code — small and well-tested, but the kind of change that should get a maintainer sign-off.
What was reviewed:
- Confirmed
JSBuffer__bufferFromPointerAndLengthAndDeinitdoesASSERT(bytesDeallocator)forlength > 0(JSBuffer.cpp:392), so the real no-op fn is required rather thanNone. - Verified
getNoopDeallocatorCallbackhas no remaining references after removal from the fixture. - Traced arg parsing in
to_buffer:ctxis only ever set whencallbackis also set, so the removed|| ctx.is_some()branch was dead and collapsing to a singlecreate_buffer_with_ctxcall is behavior-preserving for the with-finalizer path. - The one CI failure (
worker-transfer-terminate-stress.test.tsSIGABRT on x64-asan) is unrelated to FFI.
Extended reasoning...
Overview
Three files touched: src/runtime/ffi/FFIObject.rs (the fix — ~10 production lines), test/js/bun/ffi/ffi-test.c (removes the now-unnecessary getNoopDeallocatorCallback workaround), and test/js/bun/ffi/ffi.test.js (four new tests plus dropping the workaround from the existing primitives test). The production change replaces the no-finalizer fallback JSValue::create_buffer(global_this, slice) — which hard-codes MarkedArrayBuffer_deallocator / mi_free — with create_buffer_with_ctx(..., callback.or(Some(noop_bytes_deallocator))), so a toBuffer view without a finalizer borrows caller-owned memory instead of freeing it on GC.
Security risks
None in the traditional sense. bun:ffi is an inherently trusted API surface (raw pointers from JS). This fixes a use-after-free / bad-free that crashes user programs; it does not add any new attack surface. The only theoretical downside is that a caller who was (incorrectly) relying on Linux's mimalloc-override to free their pointer via the old bug now leaks — but that behavior was never documented, never worked on Windows/macOS, and doesn't match the sibling toArrayBuffer.
Level of scrutiny
Medium-high. The diff is tiny and mechanical, but it changes ownership semantics of a public API (toBuffer without a finalizer: was "free on GC", now "borrow"). It touches native memory-lifetime code, which REVIEW.md flags as the most-blocked category. I verified the C++ assertion the PR description cites, confirmed the removed ctx.is_some() branch was unreachable, and checked that the explicit-finalizer path is unchanged (callback.or(Some(noop)) preserves any user-supplied callback). The fix mirrors what to_array_buffer already does in the same file.
Other factors
All prior review feedback is resolved: my two inline comments (TinyCC-under-ASAN, combined-object subprocess assertions) were addressed in a475c24; the comment-cop paragraph-comment flags and CodeRabbit's three nits (per-test timeout, comment length, typed-array aliasing assertion) were addressed in aa056c9/032d4b9. Test coverage is solid — three subprocess crash-repro variants plus an explicit-finalizer regression guard using the compiled fixture. The one CI failure so far is in an unrelated worker_threads stress test. Given the memory-ownership semantics change and REVIEW.md's emphasis on native lifetime code, I'm deferring rather than approving so a maintainer can confirm the intended contract.
2 participants
Adopts #31753 by @EffortlessSteven. Fixes #35405. Fixes #24160. Closes #31753.
Repro
On Windows/macOS this reproduces with any
dlopen'd symbol returning amalloc'd pointer (#35405), because mimalloc override is off there somi_freewalks a CRT/libc allocation. On Linux it reproduces via double-free/UAF when the pointer comes fromptr(Buffer).Cause
to_bufferinsrc/runtime/ffi/FFIObject.rsfalls back toJSValue::create_buffer(global_this, slice)when no finalizer is supplied.create_bufferhard-codesMarkedArrayBuffer_deallocator(i.e.mi_free), so collecting the returned Buffer frees storage it never owned.toArrayBufferalready gets this right: it passes the caller's optional finalizer through and never frees on its own.Fix
The no-finalizer path installs a no-op bytes deallocator, so the Buffer borrows the pointer and collecting it frees nothing. The zero-copy view is unchanged; an explicit finalizer still controls disposal and runs exactly once.
JSBuffer__bufferFromPointerAndLengthAndDeinitasserts a non-null deallocator for non-empty storage, so a real no-op function is required rather thanNone.Verification
New
describe("toBuffer borrowed-pointer ownership ...")block intest/js/bun/ffi/ffi.test.js:ptr(Buffer)at offset 0, interior offset,ptr(Uint8Array)): unpatched child crashes (empty stdout), patched printssurvived-gcand the caller's bytes remain readable/writable after GCcc()is still called exactly once on GC with the buffer's own pointerThe
primitivesfixture test drops itsgetNoopDeallocatorCallback()workaround and now exercises the real no-finalizer path on static native storage (also red on unpatched builds).Fail-before (3 fail under both release and ASAN), pass-after (4/4 under ASAN).
cargo clippy -p bun_runtimeclean.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/ffi/ffi.test.js