blob: make the Blob__ref/Blob__deref refcount exports unsafe fns

[robobun]

Problem

Blob__ref and Blob__deref in src/jsc/webcore_types.rs are the intrusive refcount entry points behind BlobImplRefDerefTraits (src/jsc/bindings/blob.h) and the ExternalSharedDescriptor for Blob impl. They were declared as safe functions:

#[unsafe(no_mangle)]
pub extern "C" fn Blob__ref(self_: &mut Blob)
#[unsafe(no_mangle)]
pub extern "C" fn Blob__deref(self_: &mut Blob)

and bun_runtime::webcore re-exported them. Blob is an ordinary struct that also lives by value (AnyBlob payloads, stack locals, Blob::dupe() results), so a &mut Blob is easy to come by in safe code, and both functions have preconditions that nothing in the signature proves:

• Blob__deref releases a count the caller must own. On a heap Blob it double frees against the ExternalShared<Blob> or JS wrapper that actually owns the count (deref runs deinit(), which heap::takes the allocation). On a by-value Blob it underflows the count (debug_assert!(is_heap_allocated()) in debug builds).
• Blob__ref requires a heap Blob. is_heap_allocated() is encoded as ref_count != 0, so bumping a by-value Blob from 0 to 1 makes its ordinary deinit() run heap::take on an address that was never boxed. This is why the retain side needs the contract too, unlike e.g. Bun__VmHandle__retain(&VirtualMachine), where the reference already proves everything the function needs.

No in-tree Rust caller misuses them today; this is a contract fix. Same class as the other "safe fn that releases something it cannot prove the caller owns" fixes (#31981, #30812).

Fix

• Both are now private unsafe extern "C" fns over *mut Blob with the ownership and threading contract in a # Safety section. This is the shape of ExternalSharedDescriptor::ext_ref/ext_deref (so the impl forwards the pointer as is) and of the other teardown exports in the crate (AbortSignal__Timeout__deinit, Bun__VmHandle__release). Taking a raw pointer rather than materializing &mut Blob at the FFI boundary also follows the provenance guidance in src/CLAUDE.md for entry points that may free their argument; the bodies are unchanged otherwise. #[unsafe(no_mangle)] keeps the symbols exported, so blob.h and the C++ callers are unaffected; per the workspace pub convention the items no longer need to be pub since no other crate imports them.
• The two Rust callers (ExternalSharedDescriptor impl, Blob::finalize) now acknowledge the contract with unsafe blocks. finalize hands its Box over with heap::into_raw, which is what Blob__deref -> deinit -> heap::take pairs with.
• The dead re-export of both names from src/runtime/webcore/Blob.rs is removed.
• blob.h declared both as returning void*; the definitions return nothing. C++ ignores the value, so this was harmless, but the declarations now match.

Intentionally not touched here, same area, different fixes: Blob::deinit(&mut self) still frees the allocation itself when ref_count != 0 (it is the teardown Blob__deref and the structured-clone error path rely on, so changing it means auditing its callers), and the safe fn X__deref(..) declarations of C++-implemented refcount functions in unsafe extern "C" blocks are a separate population.

Test

test/internal/source-lints/unsafe-refcount-exports.test.ts scans the tracked Rust sources for #[unsafe(no_mangle)] exports named *__ref, *__deref, *__unref or *__release and requires them to be declared unsafe. It also asserts the pattern still finds the tree's refcount exports (currently Blob__ref, Blob__deref, Bun__VmHandle__release), so it cannot pass vacuously after a rename. On main it fails with:

+   "src/jsc/webcore_types.rs:483: Blob__ref",
+   "src/jsc/webcore_types.rs:492: Blob__deref",

The compiler enforces the rest: reverting either signature to safe trips unused_unsafe (warnings are denied workspace-wide) at the two Rust call sites.

Verification

• bun bd builds clean; bun bd test test/internal/source-lints/unsafe-refcount-exports.test.ts passes, and fails with the output above with src/ stashed.
• bun test test/internal/source-lints/ (all 17 lints) passes; cargo fmt -p bun_jsc -p bun_runtime -- --check clean.
• Suites that drive the C++ side of the protocol (RefPtr<Blob> in FormData, File and WebSocket) on the debug build: test/js/web/html/FormData.test.ts, FormData-file-error-leak.test.ts, test/js/web/fetch/blob.test.ts, test/js/web/websocket/websocket-blob.test.ts pass. test/js/web/fetch/blob-file-name-ownership.test.ts (4000 structuredClone round-trips, each finalized through Blob__deref) takes about 4s under the debug build on this machine and so sits at its default 5s timeout; it passes with a longer timeout, and its subprocess prints the expected output with no ASAN report.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 5 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f0b4601d-7906-4c13-a441-2c3f5462ccd9

📥 Commits

Reviewing files that changed from the base of the PR and between 97e21e5 and 4719c52.

📒 Files selected for processing (4)

• src/jsc/bindings/blob.h
• src/jsc/webcore_types.rs
• src/runtime/webcore/Blob.rs
• test/internal/source-lints/unsafe-refcount-exports.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: merged as 2d6bc9b.

Reproduced on main with the new lint: bun test test/internal/source-lints/unsafe-refcount-exports.test.ts reported Blob__ref and Blob__deref (src/jsc/webcore_types.rs) as safe extern "C" refcount entry points; with this change it passes. Contract-only change, the exported symbols and the C++ callers are unchanged.

The two related items noted in the description (Blob::deinit(&mut self) freeing the allocation itself, and BlobExt::to_js(&self) deriving the wrapper's owning pointer from a shared reference) are being tracked separately.

[claude]

LGTM — pure contract tightening with no runtime behavior change.

What was reviewed:

• Blob__ref/Blob__deref bodies are byte-identical modulo self_. → (*this).; &mut Blob → *mut Blob matches ExternalSharedDescriptor::ext_ref/ext_deref and the Bun__VmHandle__release precedent.
• heap::release → heap::into_raw in finalize: both hand off the Box without dropping; only the return type differs (&mut T vs *mut T) to match the new parameter.
• void* → void in blob.h: definitions return nothing and the three C++ call sites discard the value.
• Dropping pub and the bun_runtime::webcore re-export: grepped, no other Rust callers; #[unsafe(no_mangle)] keeps the symbol linkable for C++.
• The new source-lint follows frozen-nonnull-reborrow.test.ts exactly (same tracked-file guard, realpath dedup, non-vacuous found assertion) and its regex matches the existing pub unsafe extern "C" fn Bun__VmHandle__release.

Extended reasoning...

Overview

This PR converts Blob__ref and Blob__deref in src/jsc/webcore_types.rs from safe pub extern "C" fn(&mut Blob) to private unsafe extern "C" fn(*mut Blob), adds # Safety doc contracts, updates the two Rust callers (ExternalSharedDescriptor impl and Blob::finalize) to use unsafe blocks, removes a dead re-export from src/runtime/webcore/Blob.rs, corrects the void* → void return type in blob.h, and adds a source-lint test enforcing the pattern tree-wide.

Security risks

None. This is a compile-time contract change only. The function bodies are unchanged except for the mechanical self_. → (*this). rewrite required by the parameter-type change. No new unsafe operations are introduced; existing operations that were already unsafe-in-practice are now correctly labeled. The *mut parameter is arguably safer than &mut here since Blob__deref may free its argument, and materializing a &mut to memory that the function body then frees is questionable provenance-wise.

Level of scrutiny

Low-to-medium. While this touches refcounting FFI (memory-safety-critical territory per REVIEW.md), the change is purely a signature/contract adjustment with zero behavioral delta:

• heap::release(self) (= Box::leak → &mut T) vs heap::into_raw(self) (= Box::into_raw → *mut T) both relinquish the Box without dropping and yield a pointer to the same allocation; only the return type changed to match the callee's new parameter type.
• ext_ref/ext_deref now forward this directly instead of &mut *this — same pointer value, one fewer reborrow.
• The blob.h return-type fix is trivially verifiable: the Rust definitions return (), and all three C++ call sites in BlobImplRefDerefTraits discard the return.
• Dropping pub: #[unsafe(no_mangle)] exports the symbol regardless of Rust visibility, so C++ linkage is unaffected; grep confirms no other Rust importers of the removed re-export.

Other factors

• The PR follows the exact shape of prior contract fixes it cites (#31981, #30812) and matches the in-tree Bun__VmHandle__release precedent (src/jsc/VmHandle.rs:511).
• The new source-lint test is a near-verbatim copy of frozen-nonnull-reborrow.test.ts (same imports, same git ls-tree tracked-file guard, same realpath symlink dedup) with a well-commented regex. It guards against vacuous passes by asserting found is non-empty and scanned > 0.
• The PR description documents extensive verification: debug build, the new lint failing on stashed src/, and the FormData/WebSocket/structuredClone suites that exercise the C++ RefPtr<Blob> path.
• The bug hunting system found no issues.