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

RO
3mo ago
+214-15

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).