ffi: don't free caller-owned memory in toBuffer without a finalizer - #31753
Closed
EffortlessSteven wants to merge 3 commits into
Closed
ffi: don't free caller-owned memory in toBuffer without a finalizer#31753EffortlessSteven wants to merge 3 commits into
EffortlessSteven wants to merge 3 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughThis PR fixes a memory safety bug in Bun's FFI layer where ChangesFFI toBuffer Borrowed Pointer Ownership
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
EffortlessSteven
force-pushed
the
claude/ffi-tobuffer-owned-pointer
branch
from
June 5, 2026 09:02
a935241 to
2eb2e03
Compare
Collaborator
|
Confirming this also fixes an intermittent segfault on Windows (both x64 and arm64) when On Linux, mimalloc is built with import { dlopen, toBuffer } from "bun:ffi";
// any DLL that returns a malloc()'d pointer; test/js/bun/ffi/ffi-test.c built with clang -shared
const { symbols } = dlopen("bun-ffi-test.dll", {
ptr_should_point_to_42_as_int32_t: { returns: "ptr", args: [] },
});
toBuffer(symbols.ptr_should_point_to_42_as_int32_t(), 0, 4);
Bun.gc(true); // segfault ~50% of runs on Windows arm64/x64 canary 50bb3bd8eMeasured on canary
The This is also why The |
Jarred-Sumner
added a commit
that referenced
this pull request
Jul 25, 2026
…test.js Review fixes: - Engine failures were silently swallowed: take_exception() returned the raw JSC::Exception wrapper cell, which the glue's Error.isError() does not recognize, so a thrown TypeError (invalid signature) was treated as a successful symbol / a JSCallback with ptr === undefined. Use take_error() (unwraps to the ErrorInstance) at all three sites so the engine's error propagates, matching the TinyCC failure paths. - No fallback when the JIT is unavailable: with BUN_JSC_useJIT=0 or in a JIT-less environment the engine's create() throws "bun:ffi requires the JIT" and every dlopen failed. jsc_ffi_enabled() now queries the engine's own FFI::isAvailable() (via Bun__JSCFFIIsAvailable, cached per process) and routes to TinyCC when the engine machinery can't run. - test/harness.ts: add compileFixture(sourcePath), which builds a C fixture into a shared library with the host `cc` at test time (present on every CI test host), and gracefully skips fixture-dependent tests on a compiler-less machine instead of failing the file. - ffi.test.js was permanently skipped: its `make compile-ffi-test` prerequisite no longer exists and it hardcoded a .dylib path. Compile the fixture in-test so the suite runs on every platform. Neutralize the one pre-existing crash (toBuffer(cptr,0,4) frees a malloc'd pointer with mi_free on GC; #35405, real fix in #31753) by keeping that Buffer reachable, and mark the threadsafe i64/u64 callback cases todo (unrooted BigInt in the deferred task; #35406) -- both pre-existing on main, both reproduce on the stock canary, neither related to the backend. Result: 101 pass / 0 fail (previously never ran); the TinyCC kill-switch path still fails uint32_t identity at 2^32-1 (#7007 / #35407), which the engine path fixes. - Fix a stale doc comment on create_jsc_ffi_function and the .native comment in ffi.ts.
`bun:ffi.toBuffer(ptr, offset, len)` without an explicit finalizer fell into the owned-memory path (`JSValue::create_buffer`), which hard-codes `MarkedArrayBuffer_deallocator`. For a borrowed pointer (e.g. from `ptr(buffer)`) that `mi_free`s memory Bun never allocated when the Buffer is collected: an ASAN bad-free, and a SIGSEGV on release builds. Install a no-op deallocator on the no-finalizer path so the Buffer borrows the pointer instead of freeing it on GC, mirroring `toArrayBuffer` (which already passes its possibly-absent finalizer through). The zero-copy view is preserved; an explicit finalizer still takes ownership and runs exactly once. Tests cover the bad-free (offset 0, interior offset, typed-array source), red on system Bun / green patched and asserting the original caller memory stays valid, plus a regression that an explicit finalizer is still called exactly once on GC (self-compiled via cc() so it runs without an external fixture dylib).
EffortlessSteven
force-pushed
the
claude/ffi-tobuffer-owned-pointer
branch
from
July 29, 2026 23:24
d08f557 to
79f8000
Compare
The FFI runner's `primitives` test passed `getNoopDeallocatorCallback()` to `toBuffer`/`toArrayBuffer` so it would not hit the no-finalizer path, which adopted the caller's pointer as owned and freed it on GC. That path is fixed, so the workaround only hides the coverage: drop the callback and let the compiled fixture exercise the real default, wrapping static native storage, dropping the Buffer, forcing GC, then reading the pointer again. `getNoopDeallocatorCallback` has no other users, so remove it from the symbol descriptor, the destructuring, and the fixture. The real deallocator-counter helpers stay. Without the toBuffer fix this is red: the fixture segfaults the test runner (`panic(main thread): Segmentation fault`), which is what oven-sh#35405 reports and why `run ffi > primitives` flaked on Windows.
Comment-only. The comments around the `toBuffer` fix still described the no-finalizer path as transferring ownership to JSC and as freeing "memory Bun never allocated". Neither is accurate: `ptr(Buffer.alloc(...))` points at storage Bun did allocate but this Buffer does not own, and a caller-supplied finalizer controls disposal rather than taking ownership (it may free, return to an arena, drop a refcount, or do nothing). `create_buffer_with_ctx` also no longer receives a "possibly null" deallocator from `to_buffer`: it gets either the caller's finalizer or the internal no-op. In the tests, use "caller-owned" instead of "foreign" (a Buffer's backing store is not foreign to Bun), and correct the claim that dropping `original` makes an unpatched build hit "the same pointer a second time" — for the interior-offset case the invalidly adopted pointer is `base + 8` while the owner later disposes `base`; it is the same ownership path, not the same pointer.
Collaborator
|
Carried forward as #36521 (same diff, rebased on current main, fail-before/pass-after verified under ASAN). Co-author credit preserved in the commit. Thanks for the fix. |
Jarred-Sumner
pushed a commit
that referenced
this pull request
Jul 31, 2026
…36521) Adopts #31753 by @EffortlessSteven. Fixes #35405. Fixes #24160. Closes #31753. ## Repro ```js import { ptr, toBuffer } from "bun:ffi"; let original = Buffer.alloc(64, 0x41); let adopted = toBuffer(ptr(original), 0, 64); // zero-copy view adopted = null; Bun.gc(true); // SIGSEGV / ASAN bad-free: original's storage was mi_free'd ``` ``` panic(main thread): Segmentation fault at address 0x87D8 ``` On Windows/macOS this reproduces with any `dlopen`'d symbol returning a `malloc`'d pointer (#35405), because mimalloc override is off there so `mi_free` walks a CRT/libc allocation. On Linux it reproduces via double-free/UAF when the pointer comes from `ptr(Buffer)`. ## Cause `to_buffer` in `src/runtime/ffi/FFIObject.rs` falls back to `JSValue::create_buffer(global_this, slice)` when no finalizer is supplied. `create_buffer` hard-codes `MarkedArrayBuffer_deallocator` (i.e. `mi_free`), so collecting the returned Buffer frees storage it never owned. `toArrayBuffer` already 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__bufferFromPointerAndLengthAndDeinit` asserts a non-null deallocator for non-empty storage, so a real no-op function is required rather than `None`. ## Verification New `describe("toBuffer borrowed-pointer ownership ...")` block in `test/js/bun/ffi/ffi.test.js`: - three subprocess tests (`ptr(Buffer)` at offset 0, interior offset, `ptr(Uint8Array)`): unpatched child crashes (empty stdout), patched prints `survived-gc` and the caller's bytes remain readable/writable after GC - regression guard: explicit finalizer via `cc()` is still called exactly once on GC with the buffer's own pointer The `primitives` fixture test drops its `getNoopDeallocatorCallback()` 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_runtime` clean. <!-- robobun:evidence:begin --> --- **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 <!-- robobun:evidence:end --> --------- Co-authored-by: Steven Zimmerman <15812269+EffortlessSteven@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
bun:ffi'stoBuffer(ptr, offset, len)without a finalizer installed Bun's allocator deallocator on a pointer Bun does not own. Collecting the Buffer then frees storage owned by someone else, amallocfrom adlopen'd library or another Buffer's live backing store, and segfaults.Fix: the no-finalizer path installs a no-op 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 once.
Fixes #35405
Verification
Red on
main, green after: unpatched segfaults during GC, patched exits 0 with the caller's memory intact. #35405's DLL-allocation repro is platform-sensitive, but the same ownership bug reproduces deterministically on Linux with Buffer-backed and static storage once GC is forced, so the tests are not platform-gated.ptr(Buffer), interior offset,ptr(Uint8Array)SIGSEGV, patched reads/writes the original after GCcc()primitives)cargo clippy -p bun_runtime,prettierbun bd test test/js/bun/ffi/ffi.test.jsmainbaselineReview map
src/runtime/ffi/FFIObject.rs: no-finalizerto_bufferuses an internal no-op deallocator instead of theJSValue::create_bufferfallback, which hard-codesMarkedArrayBuffer_deallocator.test/js/bun/ffi/ffi.test.js: subprocess regressions for the three borrowed-pointer shapes, a finalizer-runs-once guard, and the compiledprimitivesfixture now exercisingtoBuffer/toArrayBufferon the default path instead of passing a workaround no-op.test/js/bun/ffi/ffi-test.c: drops the exported no-op deallocator that workaround needed; it has no other users.