Extend ForeignRef<T> to the rest of the owned C/C++ FFI handles - #33887
Conversation
|
Updated 1:05 AM PT - Jul 10th, 2026
@Jarred-Sumner, your commit ee92c7f is building: |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// `archive` is a live handle from `read_new()`/`write_new()`. | ||
| #[inline] | ||
| pub fn new2(archive: &Archive) -> Self { | ||
| Self( | ||
| core::ptr::NonNull::new(Entry::new2(archive)) | ||
| .expect("archive_entry_new2 returned null"), | ||
| ) | ||
| } | ||
| #[inline] | ||
| pub fn as_ptr(&self) -> *mut Entry { | ||
| self.0.as_ptr() | ||
| // SAFETY: `archive_entry_new2` hands back the sole ownership unit. | ||
| unsafe { Self::adopt_ptr(Entry::new2(archive)) } | ||
| .expect("archive_entry_new2 returned null") | ||
| } |
There was a problem hiding this comment.
🟡 OwnedEntry::new2(archive: &Archive) kept its textual signature, but this PR repurposed lib::Archive from the opaque ZST to the read-owning ForeignRef handle — so a WriteArchive (which now derefs to &sys::Archive) can no longer be passed, contradicting the doc comment "live handle from read_new()/write_new()". The underlying Entry::new2 was correctly updated to take &sys::Archive; this wrapper should match. No callers today, so no runtime impact — just an incomplete refactor.
Extended reasoning...
What changed
Before this PR, lib::Archive was the opaque_ffi! ZST for libarchive's struct archive, and both ReadArchive and WriteArchive implemented Deref<Target = Archive>. So OwnedEntry::new2(archive: &Archive) accepted either kind of handle, matching its doc comment: "archive is a live handle from read_new()/write_new()".
This PR renamed the ZST to sys::Archive and repurposed lib::Archive as the read-owning ForeignRef handle (via foreign_handle! { pub struct Archive(sys::Archive) via archive_free_release; }). WriteArchive now derefs to &sys::Archive, not &Archive. The underlying Entry::new2 was correctly updated at line 491 to take archive: &sys::Archive — but OwnedEntry::new2 at line 579 kept the textual spelling &Archive, whose meaning silently narrowed to "read-owning handle only".
Step-by-step
WriteArchive::new()returns aWriteArchive, which isForeignRef<sys::Archive, WriteFree>.WriteArchiveimplementsDeref<Target = sys::Archive>(line 555), so&*write_archiveis a&sys::Archive.OwnedEntry::new2requires&Archive— the read-owningForeignRef<sys::Archive, DefaultRelease>— which&sys::Archivedoes not coerce to.- A caller with a
WriteArchivetherefore gets a type error and must fall back to rawEntry::new2+ manualOwnedEntry::adopt_ptr. - Meanwhile the body
Entry::new2(archive)still compiles becauseArchiveimplementsDeref<Target = sys::Archive>(line 186), so auto-deref hides the over-constraint.
Why this matters
archive_entry_new2 exists specifically to inherit the archive's charset-conversion context, and it's most commonly used on the write side (setting entry pathnames before archive_write_header). So the over-constraint is backwards from the likely use case. The doc comment on line 577 is now false: a write_new() handle cannot be passed.
Impact and fix
Grep confirms zero callers of OwnedEntry::new2 in tree (only OwnedEntry::new() is used, at Archive.rs:329), so there's no compile break or runtime impact — this is dead code with a signature that contradicts its own doc, introduced by the type rename.
The fix is a one-token change: pub fn new2(archive: &sys::Archive) -> Self, matching Entry::new2. Both Archive and WriteArchive deref to &sys::Archive, so both would then be accepted.
| .map(|p| unsafe { Self::adopt_ptr(p) }) | ||
| } | ||
|
|
||
| pub fn from_utf8(input: &[u8]) -> Option<NonNull<URL>> { | ||
| pub fn from_utf8(input: &[u8]) -> Option<Self> { | ||
| Self::from_string(String::borrow_utf8(input)) | ||
| } | ||
|
|
||
| pub fn from_string(str: String) -> Option<NonNull<URL>> { | ||
| pub fn from_string(str: String) -> Option<Self> { | ||
| let mut input = str; | ||
| NonNull::new(URL__fromString(&mut input)) | ||
| // SAFETY: `URL__fromString` transfers a fresh `new WTF::URL` (or null) to us. |
There was a problem hiding this comment.
🟡 The URL Parsing (bun_jsc::URL) section in src/CLAUDE.md (lines ~151-160) still documents URL::from_utf8 returning Option<NonNull<URL>> and instructs callers to unsafe { URL::destroy(url.as_ptr()) }, but this PR changes those constructors to return an owning Option<Self> handle and deletes URL::destroy() entirely. Please update that snippet to reflect the new Drop-backed handle so future contributors don't go looking for a nonexistent destroy().
Extended reasoning...
What changed vs. what the docs say
This PR converts bun_jsc::URL to the foreign_handle! pattern: URL::from_utf8, URL::from_string, and URL::from_js now return Option<Self> — a #[repr(transparent)] newtype over ForeignRef<sys::URL> whose Drop calls URL__deinit. The explicit URL::destroy(this: *mut Self) method is deleted (see the src/jsc/URL.rs hunk removing pub unsafe fn destroy).
However, src/CLAUDE.md at lines 151-160 still shows the old API:
let url = URL::from_utf8(href)?; // Option<NonNull<URL>>
// caller owns the C++ object — destroy it when done:
// unsafe { URL::destroy(url.as_ptr()) }The return-type annotation is wrong (it's now Option<URL>, an owning handle), and the destroy instruction references a method that no longer exists.
Step-by-step proof
src/jsc/URL.rsin this PR:pub fn from_utf8(input: &[u8]) -> Option<Self>(wasOption<NonNull<URL>>).src/jsc/URL.rsin this PR: thepub unsafe fn destroy(this: *mut Self)block is removed;URL__deinitis now reached only throughforeign_handle!'s generatedDrop.src/CLAUDE.mdlines 158-160 (unchanged in this PR — not in the changed-files list): still saysOption<NonNull<URL>>andunsafe { URL::destroy(url.as_ptr()) }.- The PR's own
SocketAddress.rshunk demonstrates the new usage (let Some(url) = URL::from_string(...)with no explicit destroy — Drop handles it), which is exactly what the doc example should now show.
Why this matters
src/CLAUDE.md is contributor-facing guidance loaded into agent context. The repo's own review guidelines (root CLAUDE.md → .claude/docs/landing-prs.md §Docs) call for sweeping the same PR for READMEs/CLAUDE.md that describe the old state and updating them. A contributor following the current example would try to call a nonexistent URL::destroy and get a compile error, or worse, assume they need to hand-roll a scopeguard as SocketAddress.rs used to do — the exact pattern this PR is eliminating.
Impact / severity
This is documentation staleness, not a runtime defect — nothing breaks if merged as-is, and the compiler will catch anyone who copies the stale snippet. Marked nit: worth fixing in the same PR per the repo's docs-sweep guidance, but not merge-blocking.
Suggested fix
Update src/CLAUDE.md lines ~158-160 to something like:
let url = URL::from_utf8(href)?; // Option<URL> — owning handle, Drop frees the C++ objectand drop the URL::destroy line.
| Put driver scripts under `~/code/tmp/**` — Santa blocks unsigned executables elsewhere. | ||
|
|
||
| ## Two ways to invoke the debug build | ||
|
|
||
| | Need | Use | | ||
| |---|---| | ||
| | run a script, stay in the repo | `bun bd run /path/to/drive.js` | | ||
| | any command, from another cwd | `/Users/jarred/code/bun/build/debug/bun-debug <cmd>` | |
There was a problem hiding this comment.
🟡 This skill file hardcodes contributor-specific absolute paths (/Users/jarred/code/bun/build/debug/bun-debug on line 18, ~/code/bun-3 on line 58) and a macOS-only Santa MDM constraint (line 11). Since .claude/skills/ is checked in and loaded by every contributor's Claude Code session, agents on other machines will follow these instructions verbatim and hit ENOENT — consider using repo-relative ./build/debug/bun-debug or marking these as environment-specific placeholders.
Extended reasoning...
What the issue is
The new .claude/skills/verify/SKILL.md contains three environment-specific references:
- Line 11:
Put driver scripts under ~/code/tmp/** — Santa blocks unsigned executables elsewhere.— Santa is a macOS-only binary-authorization MDM tool. Contributors on Linux (or macOS without corporate MDM) have no such restriction, and~/code/tmp/may not exist. - Line 18:
| any command, from another cwd | /Users/jarred/code/bun/build/debug/bun-debug <cmd> |— an absolute path rooted at one specific contributor's home directory. - Line 58:
~/code/bun-3 tracks main and usually has a built build/debug/bun-debug— references a second personal checkout that won't exist on other machines.
Why it matters here
Unlike a personal note or a scratch script, .claude/skills/verify/SKILL.md is checked into the repository. Per Claude Code's skill mechanism, every contributor's agent will load this file and follow its instructions verbatim when asked to verify a change. The PR description explicitly says "Also adds a verify skill capturing the build-and-drive recipe", so the intent is for this to be shared tooling.
Concrete failure
A contributor on Linux with the repo cloned at /home/alice/bun invokes the verify skill. The agent, following line 18's instruction for "any command, from another cwd", runs:
/Users/jarred/code/bun/build/debug/bun-debug pm pack
This fails with No such file or directory. Similarly, an agent trying to "compare against a baseline binary" per line 58 will cd ~/code/bun-3 and fail. The Santa reference on line 11 will lead a Linux agent to needlessly restrict where it writes driver scripts.
Why nothing prevents it
There is no indirection or placeholder marker — the paths are presented as literal instructions in a table and prose. The skill's own header says "Build Bun and drive the changed code", positioning it as general-purpose. Nothing in the file signals "adjust these paths for your machine."
Suggested fix
Replace the absolute path with the repo-relative form the rest of the repo already uses (CLAUDE.md documents ./build/debug/bun-debug), and either drop the Santa/~/code/bun-3 references or annotate them as environment-specific. For line 18 specifically, something like $PWD/build/debug/bun-debug (run from the repo root) or <repo>/build/debug/bun-debug would be portable.
This is documentation rather than runtime code — nothing crashes at build or test time — so it's a nit, but checked-in agent instructions should work for everyone who clones the repo.
7fd7042 to
c6d453b
Compare
64ec3b5 to
ac32919
Compare
c6d453b to
4f14bfa
Compare
ac32919 to
b78068c
Compare
Extends the ForeignRef<T> pattern from FetchHeaders to 16 more opaque FFI types. Each moves its opaque_ffi! ZST into a `sys` module and exposes a release function, replacing hand-rolled owners (OwnedSslCtx, OwnedDecompressor) and bare raw pointers. Also flips 314 `&mut self` receivers to `&self` on opaque ZSTs. Those types are UnsafeCell-backed and !Freeze, so `&T` carries no noalias and C mutates through it; `&mut self` asserted an exclusivity that was never true and never needed. The receiver flip is only sound when a method hands back nothing aliasing real memory. Four do, and keep `&mut self`: ConnectingSocket::ext, Timer::ext, ListenSocket::ext, and JSUint8Array::slice all return `&mut T` into storage the ZST receiver does not cover. Two fixes fall out of the rename: - cppbind mapped JSC::SourceProvider to the owning handle, so the generated safe wrapper passed the address of a Rust stack slot to C++ `->deref()`. It now names the sys:: ZST, matching the WebCore::EventLoopTask entry. - Flipping Response::upgrade to `&self` moved method resolution to ResponseLike::upgrade, which boxes its argument a second time. The DevServer call site now names the inherent method explicitly. Restores the debug-only corrupted-HandleSlot assert that Strong::destroy carried before it became a ForeignRef; a bad slot otherwise faults inside JSC with no Rust frame. Adds a verify skill capturing the build-and-drive recipe.
Two changes to bun_opaque, then 18 types stop hand-rolling their ownership boilerplate. ForeignRef<T> gains a release-marker parameter, ForeignRef<T, R = DefaultRelease>. ForeignOwned admits exactly one release per type, so a foreign object with two ownership disciplines could not use it twice: libarchive's `struct archive` is freed by archive_read_free when opened for reading and archive_write_free when opened for writing, and the write side had degraded into a bespoke owner with a hand-written Drop. It is now ForeignRef<sys::Archive, WriteFree>. The parameter defaults, so every existing ForeignRef<T> is unchanged. foreign_handle! emits the newtype, its ForeignOwned impl, and adopt / adopt_ptr / as_ptr / leak / raw. That block had been copied onto 17 types by hand; a missing mem::forget in one copy is a double-free the other sixteen would not reveal. Net -437 lines. adopt and adopt_ptr are now unsafe. Most hand-written copies were safe private fns, but adopting a pointer whose ownership unit you were not given is UB, so the obligation belongs at the call site. All 29 call sites now carry a SAFETY comment naming the producer that hands over the unit. WriteArchive and OwnedEntry lose their hand-written Drop impls, leaving two hand-rolled owners in the tree (CookieMapRef, CppWebSocketRef).
Nine file-disjoint shards classified 17 opaque types by reading the C and C++,
then converted the ones Rust actually owns a unit of.
Converted:
ENGINE, X509, X509_STORE, X509_STORE_CTX (boringssl)
SSL_SESSION (tls_socket_functions)
spng_ctx (PNG codec)
WebPDemuxer, WebPMux (WebP codec)
CookieMapRef (folded onto ForeignRef, name kept)
Each replaces a hand-written scopeguard or Drop. The certificate handles are
careful about where the ref comes from: SSL_get_peer_certificate, X509_up_ref,
X509_STORE_CTX_get1_issuer and d2i_X509 hand over a +1 and are adopted;
SSL_get_certificate, sk_X509_value and SSL_CTX_get_cert_store return borrows and
stay raw pointers. SSL_set0_verify_cert_store takes ownership, so that path
leaks the handle rather than dropping it.
Not converted, each for a reason found in the C++:
AbortSignal, NapiEnv, JSCArrayBuffer already owned by bun_ptr::ExternalShared,
which models ref/deref, not one unit
Blob the opaque decl is an erased stand-in for
a type declared a tier up
JSPropertyIteratorImpl freed by its enclosing struct's Drop
Channel (c-ares) ares_destroy() fires ARES_EDESTRUCTION
callbacks that re-enter RefPtr::deref;
Resolver declares ref_count before
channel, so a Drop-based owner would free
the refcount's debug tables first and
then read them. The existing open-coded
teardown in Drop for GlobalData is
load-bearing.
Loop, Heap, App thread/process-lifetime singletons
spng_ctx_free, WebPDemuxDelete and WebPMuxDelete stay unsafe externs behind a
plain wrapper: a safe fn taking &sys::T would let safe code free a context the
handle still owns.
4f14bfa to
ee92c7f
Compare
b78068c to
32f1300
Compare
59f77ea
into
claude/foreign-owned-fetch-headers
2 participants
Stacked on #33820, which introduces
ForeignRef<T>and convertsFetchHeaders.What this does
Extends the owned-handle pattern to every remaining opaque FFI type that Rust holds an
ownership unit of, and generates the boilerplate instead of copying it.
1. Owned handles for 16 more types — each replaces a hand-rolled owner, a
scopeguard,or a bare raw pointer with a
#[repr(transparent)]newtype whoseDropcalls the Crelease function. Also flips 314
&mut selfreceivers to&selfonopaque_ffi!ZSTs:those types are
UnsafeCell-backed, so&Tcarries nonoaliasand C mutates throughit — the
&mutasserted an exclusivity that was never true and never needed.2.
ForeignRef<T, R = DefaultRelease>— a release-marker parameter.ForeignOwnedadmits one release per type, so an object with two ownership disciplines could not use it
twice: libarchive's
struct archiveis freed byarchive_read_freewhen opened forreading and
archive_write_freewhen opened for writing, and the write side had degradedinto a bespoke owner with a hand-written
Drop. It is nowForeignRef<sys::Archive, WriteFree>. The parameter defaults, so no existingForeignRef<T>changed.3.
foreign_handle!emits the newtype plusadopt/adopt_ptr/as_ptr/leak/raw. That block had been hand-copied onto 17 types; a missingmem::forgetin one copyis a double-free the other sixteen would not reveal. Net -437 lines.
adoptandadopt_ptrare nowunsafe. Most hand-written copies were safe private fns,but adopting a pointer whose ownership unit you were not given is UB, so the obligation
belongs at the call site — all 29 now carry a
SAFETY:comment naming the producer.4. The remaining C handles, converted after reading the C and C++ rather than the
names:
ENGINE,X509,X509_STORE,X509_STORE_CTX,SSL_SESSION,spng_ctx,WebPDemuxer,WebPMux, andCookieMapReffolded ontoForeignRef.The certificate handles are careful about where the ref comes from.
SSL_get_peer_certificate,X509_up_ref,X509_STORE_CTX_get1_issuerandd2i_X509hand over a
+1and are adopted;SSL_get_certificate,sk_X509_valueandSSL_CTX_get_cert_storereturn borrows and stay raw pointers.SSL_set0_verify_cert_storetakes ownership, so that path leaks the handle rather thandropping it.
Two bugs this surfaced
cppbindmappedJSC::SourceProviderto the owning handle, so the generated safewrapper passed the address of a Rust stack slot to C++
->deref(). No caller today, butit also produced two conflicting
extern "C"declarations of one symbol.Response::upgradeto&selfsilently moved method resolution toResponseLike::upgrade, which boxes its argument a second time. Rust probes the receiverby-value first, so an inherent
&mut selfmethod beats a trait method there; onceflipped it no longer matches. Only an arity mismatch made it visible.
Restores the debug-only corrupted-
HandleSlotassert thatStrong::destroycarried beforeit became a
ForeignRef; a bad slot otherwise faults inside JSC with no Rust frame.Deliberately not converted
AbortSignal,NapiEnv,JSCArrayBufferbun_ptr::ExternalShared, which models ref/deref, not a single unitBlob(standalone_graph)JSPropertyIteratorImplDropChannel(c-ares)Loop,Heap,Appares_destroy()invokesquery->callback(query->arg, ARES_EDESTRUCTION, 0, NULL)for everypending query, and those callbacks re-enter
RefPtr::deref.Resolverdeclaresref_countbefore
channel, and Rust drops fields in declaration order, so aDrop-based owner wouldfree the refcount's debug tables and then let the callbacks read them. The open-coded
teardown in
Drop for GlobalDatais load-bearing. Converting it safely means declaringchannelfirst, or keeping an explicitdrop(self.channel.take()); left for a follow-up.spng_ctx_free,WebPDemuxDeleteandWebPMuxDeletestayunsafeexterns behind a plainwrapper: a
safe fntaking&sys::Twould let safe code free a context the handle owns.Verification
cargo build -p bun_binclean.cargo checkis not sufficient here: it stops beforecodegen, so it never evaluates the
const { assert!(size_of::<T>() == 0) }guards insideopaque_deref*, and a rename that turnsNonNull<X>from "the C object" into "an 8-byteRust struct" typechecks fine.
bun bdclean, no warnings.the
rejectUnauthorizedreject path, PNG and WebP encode/decode round-trips (outputbyte-identical to before), cookie get/set through
Bun.serveroutes,bun pm packandtarball extraction,
bun:ffi cc,--bytecode,.npmrcregex, HMR WebSocket upgrade,Bun.connectfailure path. Each hammered a few hundred iterations underBun.gc(true)to check refcount balance.
verifyskill capturing the build-and-drive recipe.