bun_url: make freeing a whatwg::URL an unsafe raw-pointer operation

[robobun]

What

bun_url::whatwg::URL (src/url/lib.rs) is an opaque zero-sized handle to a C++ heap WTF::URL (URL__fromString does new, URL__deinit does delete). The extern was declared safe fn URL__deinit(url: &mut URL) and wrapped as a safe pub fn deinit(&mut self), so this compiles with no unsafe anywhere and is a double delete, and the commented-out line is a use after free:

let p = bun_url::whatwg::URL::from_utf8(b"https://example.com/").unwrap();
let url = unsafe { &mut *p.as_ptr() };   // a valid &mut to a ZST; proves nothing about ownership
url.deinit();
// url.href();
url.deinit();

Nothing in tree does this today. The one caller, OwnedJscUrl::drop in src/install/hosted_git_info.rs, is correct, but the unsafe block it did have only covered as_mut(), not the free. Surfaced by review on #32023, which re-exports this type as bun_jsc::URL and deliberately left the signature alone (zero behavior change PR).

Cause

The comment above the extern block argued "deinit takes &mut URL (consumes)". &mut self does not consume, and for a zero-sized handle a &mut is obtainable from any non-null pointer, so it also does not prove the allocation is owned or still alive. The safe fn declaration was claiming a contract the signature cannot enforce. The bun_jsc::URL copy of the same binding (src/jsc/URL.rs) already had the right shape: fn URL__deinit(*mut URL) plus pub unsafe fn destroy(this: *mut Self).

Fix

• URL__deinit is declared as an unsafe fn taking *mut URL.
• deinit(&mut self) is replaced by pub unsafe fn destroy(this: *mut Self) with a Safety contract; from_string / from_utf8 say the result is owned and freed with it. Same name and signature as bun_jsc::URL::destroy.
• OwnedJscUrl::drop calls JscUrl::destroy and says why it is the single free.

Why this layer and not a Drop on URL: the type is a ZST token, not the allocation, so Rust never holds a URL by value and Drop cannot apply. Ownership lives in whoever holds the pointer (OwnedJscUrl here), and unsafe fn destroy(*mut Self) is the shape the other 18 FFI-handle destructors in the workspace use for this situation. No runtime behavior changes.

Interaction with #32023, checked with a three-way merge of src/url/lib.rs: three small conflicts (struct docs, the URL__deinit line, the constructor docs), the merge drops deinit by itself, and #32023 then has to delete its own destroy (whose body calls deinit) and two doc references to it. Its opaque_ffi! conversion and by-value from_string are its own dedup scope and are intentionally not pulled in here.

Test

test/internal/source-lints/safe-ffi-release-method.test.ts flags any safe pub fn taking &self / &mut self whose first statement, optionally inside unsafe { .. }, forwards self to a *_deinit / *_destroy / *_delete / *_free / *_dealloc / *_deref / *_unref / *_release shim. The unsafe { .. } form matters: once a shim is declared the recommended way (unsafe fn over a raw pointer), that is the only form the unsound wrapper can take, so without it the lint would only ever see shims still declared safe fn and would not catch deinit being re-added on top of this fix. The regex is checked against inline fixtures in both directions (pub unsafe fn destroy(*mut Self), Drop-only private shims, opaque_deref, releaseWeakRefs, by-value receivers and non-release calls inside unsafe blocks must not match). Against main's src/ it reports exactly src/url/lib.rs:148: pub fn deinit(&mut self, ..) forwards self to URL__deinit; with this branch it passes.

The same scan finds six pre-existing instances of the shape, which are allowlisted with a ratchet (a stale entry fails the test) rather than fixed here: FetchHeaders::deref and SourceProvider::deref are removed by #33820, which turns both into Drop-owned handles; AbortSignal::unref / detach and libarchive's Archive::read_free / write_free / Entry::free (each has a Drop-owning wrapper that derefs to the handle, so owner.read_free() plus the owner's drop is a double free; their callers sit in files #33820 and #37549 are changing) are tracked as their own fixes.

Verification

• cargo check, cargo clippy and cargo fmt --check on bun_url and bun_install; debug (ASAN) build links.
• bun bd test test/cli/install/hosted-git-info/ (655 pass) creates and drops a WTF::URL through OwnedJscUrl on every parse, so the new destroy path runs under ASAN.
• The new lint fails with main's two source files checked out in place of this branch's and passes with them restored (above).

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 8 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: 6d394dd9-e613-4cf1-b1cd-b2ff839c4694

📥 Commits

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

📒 Files selected for processing (3)

• src/install/hosted_git_info.rs
• src/url/lib.rs
• test/internal/source-lints/safe-ffi-release-method.test.ts

---

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

[robobun]

^{Updated 6:24 PM PT - Aug 11th, 2026}

✅ @robobun, your commit f699dde92eb3257dbae2f5dbb8347939222fd22f passed in Build #92695! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37577

That installs a local version of the PR into your bun-37577 executable, so you can run:

bun-37577 --bun

[robobun]

Status: ready for review at f699dde.

Reproduced how: the new lint in test/internal/source-lints/safe-ffi-release-method.test.ts reports src/url/lib.rs:148: pub fn deinit(&mut self, ..) forwards self to URL__deinit against main's src/ and passes with this branch; bun bd test test/cli/install/hosted-git-info/ (655 pass) exercises the new destroy path under ASAN.

Since the first push: the lint also matches the unsafe { shim(self..) } body shape (the only shape possible once a shim is declared unsafe), which added libarchive's three free methods to the allowlist; the binding comments were trimmed; the PR description's note on rebasing #32023 was corrected after checking with a three-way merge. The two other instances the lint allowlists are removed by #33820; AbortSignal::unref and the libarchive methods are tracked separately.

[claude]

LGTM — tightens an FFI free from a safe &mut self method to unsafe fn destroy(*mut Self), matching the existing bun_jsc::URL shape; no runtime behavior change.

What was reviewed

• Confirmed OwnedJscUrl::drop is the only caller of the removed deinit; src/jsc/URL.rs already uses destroy(*mut Self) and is unaffected.
• Verified the three ALLOW entries (AbortSignal::unref, FetchHeaders::deref, SourceProvider::deref) exist and match the lint regex, so the ratchet test won't spuriously fail.
• New lint follows the sibling source-lints pattern (globAllSources, tracked-files guard, realpath dedup, non-empty-scan vacuity check) and self-tests its regex against positive/negative fixtures.

Extended reasoning...

Overview

This PR converts bun_url::whatwg::URL::deinit(&mut self) (a safe method that deletes a C++ heap allocation) into pub unsafe fn destroy(this: *mut Self), and re-declares the URL__deinit extern as unsafe fn(*mut URL) instead of safe fn(&mut URL). The single caller, OwnedJscUrl::drop in src/install/hosted_git_info.rs, is updated to call JscUrl::destroy(self.0.as_ptr()). A new source-lint test scans for the unsound pattern (safe &self/&mut self method forwarding to a *_deinit/*_free/*_deref/etc. FFI shim) with a ratcheted allowlist for three pre-existing instances tracked in other PRs.

Security risks

None. This is a type-system soundness tightening on an internal FFI binding — the same URL__deinit C++ function is called with the same pointer value; only the Rust signature moves from safe to unsafe. No user input, no auth, no data exposure.

Level of scrutiny

Low-to-medium. The Rust change is mechanical (signature swap plus one call-site update) and aligns byte-for-byte with the existing src/jsc/URL.rs::destroy shape. I grepped for other users of whatwg::URL / JscUrl and confirmed hosted_git_info.rs is the only consumer of the changed method. The new lint test is the larger piece; it copies the harness scaffolding (git ls-tree filter, realpath symlink dedup, vacuity guard) verbatim from unsound-erased-box.test.ts / dead-code-escapes.test.ts, and I confirmed each of the three allowlisted entries exists in src/jsc/{AbortSignal,FetchHeaders,SourceProvider}.rs with a shape the regex matches, so the ratchet assertion holds.

Other factors

The PR description states cargo check/clippy/fmt pass and the 655 hosted-git-info tests (which exercise OwnedJscUrl::drop under ASAN) pass. The regex has an inline positive/negative self-test guarding against silent breakage. The stripLineComments helper uses [ \t]* (not \s*) so line numbers are preserved for the reported location. No prior reviewer comments to address.

[robobun]

Heads up: #37609 removes AbortSignal::unref / detach (the instance this lint allowlists as src/jsc/AbortSignal.rs: unref -> WebCore__AbortSignal__unref). Whichever of the two lands second needs to drop that allowlist line, since the ratchet here fails on a stale entry. #37609 also adds a narrower lint (refcount-release-owner.test.ts) that pins specific release shims to their owner's release hook; the two do not overlap in files.

[claude]

LGTM — tightens an unsound safe FFI wrapper to pub unsafe fn destroy(*mut Self) with no runtime behavior change, and adds a source lint to keep it that way.

What was reviewed:

• Confirmed OwnedJscUrl::drop is the sole caller of the old deinit; the new JscUrl::destroy(self.0.as_ptr()) reaches the same URL__deinit on the same pointer, and the shape now matches src/jsc/URL.rs.
• Verified all six ALLOW entries in the lint exist and match the regex against current src/; the other pub fn deref/unref methods in the tree do not match (path-qualified or non-shim first statements).
• Checked the lint follows the dead-code-escapes.test.ts harness pattern (tracked-file guard, realpath dedup, non-empty-scan guard) and that stripLineComments preserves line numbers.

Extended reasoning...

Overview

This PR changes bun_url::whatwg::URL::deinit(&mut self) (a safe method that deletes a C++ heap allocation behind a ZST handle) into pub unsafe fn destroy(this: *mut Self), and re-declares the extern URL__deinit as an unsafe fn over *mut URL. The single caller, OwnedJscUrl::drop in src/install/hosted_git_info.rs, is updated to call JscUrl::destroy(self.0.as_ptr()) inside an unsafe block. A new source-lint test (test/internal/source-lints/safe-ffi-release-method.test.ts) enforces that no safe &self/&mut self method forwards self to a *_deinit/*_free/*_deref/etc. FFI shim, with a ratcheted allowlist for six pre-existing instances tracked by other PRs.

Security risks

None. This is a type-system-level soundness fix (making an unsound safe API require unsafe). The generated machine code is identical: URL__deinit is still called with the same pointer from the same Drop impl. No user input, auth, or data-exposure surface is touched.

Level of scrutiny

Low-to-medium. The Rust change is mechanical (~15 lines net in two files) and matches the existing src/jsc/URL.rs binding shape line-for-line. I grepped for other callers of deinit/URL__deinit on this type — there are none beyond the updated OwnedJscUrl::drop. The lint test is the bulk of the diff; it copies the harness pattern from the sibling dead-code-escapes.test.ts (globAllSources().rust, git-tracked filter, realpath symlink dedup), includes a fixture self-test in both directions, guards against vacuous pass with expect(scanned).toBeGreaterThan(0), and ratchets the allowlist so stale entries fail. I spot-checked the regex against all six allowlisted sites and against the other pub fn deref/unref methods in the tree that do not forward to a bare shim — the pattern's \w+_verb(self requirement correctly excludes them.

Other factors

The comment-cop bot flagged verbose comments several times; the author trimmed them across a2812f5 → 9d783ef → f699dde and all threads are resolved. The final destroy doc is a two-line # Safety contract matching other unsafe fn destroy(*mut Self) handles in the tree. The PR description documents ASAN-verified test coverage via test/cli/install/hosted-git-info/ (655 tests) which exercises OwnedJscUrl on every parse. The known merge-order interaction with #37609 (which removes one allowlisted entry) is called out in the thread and handled by the ratchet.