From 4719c5253c587002529ac98a243b15df3b2c46f9 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:27:41 +0000 Subject: [PATCH] blob: make the Blob__ref/Blob__deref refcount exports unsafe fns Blob__ref and Blob__deref were safe pub extern "C" fns taking &mut Blob, and bun_runtime::webcore re-exported them. Blob is an ordinary struct that also lives by value (AnyBlob payloads, stack locals), so safe code could release a count it did not own (double free against the ExternalShared or JS wrapper that does own it) or bump a by-value Blob's count and make its normal deinit() free a non-heap address. Declare both as private unsafe extern "C" fns over *mut Blob with the ownership contract documented, matching ExternalSharedDescriptor and the other teardown exports in this crate. The exported symbols are unchanged; the only callers are BlobImplRefDerefTraits in blob.h, the ExternalSharedDescriptor impl and Blob::finalize. Drop the dead re-export and fix blob.h, which declared both as returning void* while the definitions return nothing. Add a source lint requiring no_mangle *__ref/__deref/__unref/__release exports to be declared unsafe. --- src/jsc/bindings/blob.h | 4 +- src/jsc/webcore_types.rs | 72 ++++++++++----- src/runtime/webcore/Blob.rs | 4 +- .../unsafe-refcount-exports.test.ts | 92 +++++++++++++++++++ 4 files changed, 144 insertions(+), 28 deletions(-) create mode 100644 test/internal/source-lints/unsafe-refcount-exports.test.ts diff --git a/src/jsc/bindings/blob.h b/src/jsc/bindings/blob.h index 9612ac6942d0..631861a37ae6 100644 --- a/src/jsc/bindings/blob.h +++ b/src/jsc/bindings/blob.h @@ -11,8 +11,8 @@ extern "C" void* Blob__dupe(void* impl); extern "C" void* Blob__getDataPtr(JSC::EncodedJSValue blob); extern "C" size_t Blob__getSize(JSC::EncodedJSValue blob); extern "C" void* Blob__fromBytes(JSC::JSGlobalObject* globalThis, const void* ptr, size_t len); -extern "C" void* Blob__ref(void* impl); -extern "C" void* Blob__deref(void* impl); +extern "C" void Blob__ref(void* impl); +extern "C" void Blob__deref(void* impl); // Opaque type corresponding to `bun.webcore.Blob`. class BlobImpl; diff --git a/src/jsc/webcore_types.rs b/src/jsc/webcore_types.rs index cd202fc83753..b3a6a52daae4 100644 --- a/src/jsc/webcore_types.rs +++ b/src/jsc/webcore_types.rs @@ -232,10 +232,11 @@ impl Blob { self.is_heap_allocated(), "`finalize` may only be called on a heap-allocated Blob" ); - // `release` returns the raw `m_ctx` pointer without dropping; - // `Blob__deref` runs `deinit()` (which `drop(heap::take)`s) when the - // count reaches zero. - Blob__deref(bun_core::heap::release(self)); + // SAFETY: `self` is the allocation `Blob::new` produced and the JS + // wrapper's `+1` is the count released here. `into_raw` hands the box + // over without dropping it; `Blob__deref` runs `deinit()` (which + // `drop(heap::take)`s) when the count reaches zero. + unsafe { Blob__deref(bun_core::heap::into_raw(self)) } } #[inline] @@ -471,34 +472,59 @@ impl Blob { unsafe impl bun_ptr::ExternalSharedDescriptor for Blob { unsafe fn ext_ref(this: *mut Self) { // SAFETY: caller guarantees `this` points to a live heap-allocated Blob. - unsafe { Blob__ref(&mut *this) } + unsafe { Blob__ref(this) } } unsafe fn ext_deref(this: *mut Self) { - // SAFETY: caller guarantees `this` points to a live heap-allocated Blob. - unsafe { Blob__deref(&mut *this) } + // SAFETY: caller guarantees `this` points to a live heap-allocated Blob + // and is releasing a count it owns. + unsafe { Blob__deref(this) } } } +/// Retain half of the refcount protocol behind `BlobImplRefDerefTraits` +/// (`src/jsc/bindings/blob.h`) and [`bun_ptr::ExternalShared`]. +/// +/// # Safety +/// `this` must point to a live `Blob` produced by [`Blob::new`], and the call +/// must happen on the thread that owns it (the count is not atomic). A +/// by-value `Blob` has a count of zero; bumping it would make a later +/// [`Blob::deinit`] free an address that was never heap-allocated. #[unsafe(no_mangle)] -pub extern "C" fn Blob__ref(self_: &mut Blob) { - debug_assert!( - self_.is_heap_allocated(), - "cannot ref: this Blob is not heap-allocated" - ); - self_.ref_count.increment(); +unsafe extern "C" fn Blob__ref(this: *mut Blob) { + // SAFETY: caller contract above. + unsafe { + debug_assert!( + (*this).is_heap_allocated(), + "cannot ref: this Blob is not heap-allocated" + ); + (*this).ref_count.increment(); + } } +/// Release half of the refcount protocol behind `BlobImplRefDerefTraits` +/// (`src/jsc/bindings/blob.h`), [`bun_ptr::ExternalShared`] and the JS +/// wrapper's [`Blob::finalize`]. +/// +/// # Safety +/// `this` must point to a live `Blob` produced by [`Blob::new`], the caller +/// must own one of its counts (which this call consumes), and the call must +/// happen on the thread that owns it (the count is not atomic). Releasing the +/// last count frees the `Blob`, so `this` is dangling once this returns. #[unsafe(no_mangle)] -pub extern "C" fn Blob__deref(self_: &mut Blob) { - debug_assert!( - self_.is_heap_allocated(), - "cannot deref: this Blob is not heap-allocated" - ); - if self_.ref_count.decrement() == bun_ptr::raw_ref_count::DecrementResult::ShouldDestroy { - // `deinit` has its own `is_heap_allocated()` guard around the - // `drop(heap::take)`, so re-arm so it returns true. - self_.ref_count.increment(); - self_.deinit(); +unsafe extern "C" fn Blob__deref(this: *mut Blob) { + // SAFETY: caller contract above. `deinit` frees the allocation, so `this` + // is not touched after it. + unsafe { + debug_assert!( + (*this).is_heap_allocated(), + "cannot deref: this Blob is not heap-allocated" + ); + if (*this).ref_count.decrement() == bun_ptr::raw_ref_count::DecrementResult::ShouldDestroy { + // `deinit` has its own `is_heap_allocated()` guard around the + // `drop(heap::take)`, so re-arm so it returns true. + (*this).ref_count.increment(); + (*this).deinit(); + } } } diff --git a/src/runtime/webcore/Blob.rs b/src/runtime/webcore/Blob.rs index 3e9b5efb447a..514364a889ca 100644 --- a/src/runtime/webcore/Blob.rs +++ b/src/runtime/webcore/Blob.rs @@ -107,9 +107,7 @@ pub trait ReadBytesHandler { // This crate layers behaviour via the `BlobExt` extension trait below. // ────────────────────────────────────────────────────────────────────────── -pub use bun_jsc::webcore_types::{ - Blob, Blob__deref, Blob__ref, BlobContentType, ClosingState, MAX_SIZE, SizeType, -}; +pub use bun_jsc::webcore_types::{Blob, BlobContentType, ClosingState, MAX_SIZE, SizeType}; /// 1: Initial /// 2: Added byte for whether it's a dom file, length and bytes for `stored_name`, diff --git a/test/internal/source-lints/unsafe-refcount-exports.test.ts b/test/internal/source-lints/unsafe-refcount-exports.test.ts new file mode 100644 index 000000000000..85bb8fcc90ed --- /dev/null +++ b/test/internal/source-lints/unsafe-refcount-exports.test.ts @@ -0,0 +1,92 @@ +import { file } from "bun"; +import { expect, test } from "bun:test"; +import { realpathSync } from "fs"; +import path from "path"; +import { globAllSources } from "../../../scripts/glob-sources.ts"; + +// Rust-implemented refcount entry points exported to C++ (`#[unsafe(no_mangle)]` +// fns named `*__ref`, `*__deref`, `*__unref`, `*__release`) must be `unsafe fn`. +// +// Adjusting an intrusive count is only sound when the caller holds a count on +// an object that is actually refcounted, and no signature can prove that: +// releasing a count nobody owns frees the object out from under its owner, and +// bumping the count of a by-value instance turns its ordinary teardown into a +// free of a non-heap address. That obligation has to be an `unsafe` contract on +// the export itself, because the same symbol is callable from Rust (the +// `ExternalSharedDescriptor` impl, finalizers) as well as from the C++ +// `RefDerefTraits` it exists for. +// +// Motivating instance: `Blob__ref` / `Blob__deref` in src/jsc/webcore_types.rs +// were safe `pub extern "C" fn`s over `&mut Blob`, re-exported from +// `bun_runtime::webcore`, so any safe code holding a `Blob` (stack local, +// `AnyBlob` payload) could release the count owned by the JS wrapper or an +// `ExternalShared`. `Bun__VmHandle__release` in src/jsc/VmHandle.rs is the +// shape this lint requires. +// +// Scope: definitions only. `safe fn X__deref(..)` declarations of C++-implemented +// functions inside `unsafe extern "C" { .. }` blocks are a separate population, +// as are `*__destroy` shims over raw pointers. The suffix list is the +// enforcement boundary: a new refcount export under another name goes here too. +// +// Sibling guards: unsound-erased-box.test.ts, frozen-nonnull-reborrow.test.ts. + +const root = path.resolve(import.meta.dir, "..", "..", ".."); +const rustSources = globAllSources().rust.filter(p => p.endsWith(".rs")); + +// Only scan files tracked in HEAD (a `git stash` round-trip can leave stray +// `.rs` files in the working tree; CI runs on a clean checkout). Same guard as +// dead-code-escapes.test.ts. +const tracked: Set | null = (() => { + const r = Bun.spawnSync({ + cmd: ["git", "-C", root, "ls-tree", "-r", "--name-only", "-z", "HEAD"], + stdout: "pipe", + stderr: "ignore", + }); + if (!r.success) return null; + return new Set(r.stdout.toString().split("\0").filter(Boolean)); +})(); + +// `#[unsafe(no_mangle)]`, any further attributes, then the fn header. Group 1 is +// the `unsafe` qualifier (absent on an offender), group 2 the exported name. +// Doc comments between the attribute and the header are stripped below. +const REFCOUNT_EXPORT = + /#\[unsafe\(no_mangle\)\]\s*(?:#\[[^\]]*\]\s*)*(?:pub(?:\([^)]*\))?\s+)?(unsafe\s+)?extern\s+"C(?:-unwind)?"\s+fn\s+(\w+__(?:ref|deref|unref|release))\s*\(/g; + +const found: string[] = []; +const offenders: string[] = []; +let scanned = 0; +for (const abs of rustSources) { + const source = path.relative(root, abs).replaceAll(path.sep, "/"); + // `src/cli` is a symlink into `src/runtime/cli`; count each file once under + // its canonical path. + if (path.relative(root, realpathSync(abs)).replaceAll(path.sep, "/") !== source) continue; + if (tracked !== null && !tracked.has(source)) continue; + scanned++; + const content = await file(abs).text(); + // Strip full-line comments (including `///` docs) so prose mentions don't + // count and the attribute -> header match is not interrupted by a doc block. + // `[ \t]*`, not `\s*`, so blank lines survive and line numbers stay right. + const stripped = content.replace(/^[ \t]*\/\/.*$/gm, ""); + for (const m of stripped.matchAll(REFCOUNT_EXPORT)) { + const line = stripped.slice(0, m.index + m[0].lastIndexOf(m[2])).split("\n").length; + const entry = `${source}:${line}: ${m[2]}`; + found.push(entry); + if (m[1] === undefined) offenders.push(entry); + } +} + +test("scans a non-empty set of tracked Rust sources", () => { + // Guards against the tracked/realpath filters above over-firing and leaving + // nothing to scan, which would make the assertions below pass vacuously. + expect(scanned).toBeGreaterThan(0); +}); + +test("the pattern still recognizes the tree's refcount exports", () => { + // If this goes empty, the exports were renamed or restructured and the + // suffix list / regex above needs updating, not the assertion below. + expect(found).not.toBeEmpty(); +}); + +test('exported refcount entry points are declared `unsafe extern "C" fn`', () => { + expect(offenders).toEqual([]); +});