DO NOT MERGE: bisect s3.test.ts regression (commit 2/4) - #33908
Draft
Jarred-Sumner wants to merge 2 commits into
Draft
DO NOT MERGE: bisect s3.test.ts regression (commit 2/4)#33908Jarred-Sumner wants to merge 2 commits into
Jarred-Sumner wants to merge 2 commits into
Conversation
A C++-allocated object that Rust releases through an FFI destructor needs two
types: the borrowed opaque (the `opaque_ffi!` ZST) and an owner whose Drop
gives the ref back. Four such owners were hand-rolled and 27 more C++ types
release by hand at every call site. Add the generic pair to `bun_opaque`:
- `ForeignOwned`: this opaque type has a release extern
- `ForeignRef<T>`: `Deref` + `Drop`, `repr(transparent)` over `NonNull<T>`
- `foreign_owned!(T, release_fn)` emits the impl
`FetchHeaders` is the first user, and the owned handle takes the public name;
the raw ZST moves into an extern-only `sys` module.
- Every receiver is `&self`. The type is `UnsafeCell`-backed precisely so
that `&T` carries no `noalias`, and C++ mutates the header storage through
the same pointer, so `&mut self` asserted an exclusivity that was never
true and never needed.
- Constructors return `Self`; C++ always hands back a fresh +1.
- `cast()` returns `ManuallyDrop<FetchHeaders>`: it borrows the ref that the
JS `Headers` wrapper owns, so releasing it must not be expressible.
- The four `void*`-taking externs become `unsafe fn`. C++ dereferences the
pointer, and safe Rust can forge a `*mut c_void`, so `safe fn` was
unsound on all four. This matches the rule already written above the
extern block.
- `create()` and `copy_to()` take slices instead of `*mut StringPointer`
plus a separate length.
- `FetchHeaders::from` and `create_value` had no callers; deleted. The
`_`-suffixed raw variants had no external callers; now private.
Making `cast()` a `ManuallyDrop` stopped `server.fetch(url, { headers })`
from compiling, which is how it turned out to be a use-after-free. That path
adopted the ref owned by the JS `Headers` wrapper: `cast_` never bumps the
refcount, and the wrapper's `Ref<>` derefs on finalize, so the internal
`Request` and the wrapper each released the same single ref. ASan confirms
the heap-use-after-free, and the aliasing half is deterministic without it: a
header the handler sets on `req.headers` shows up on the caller's `Headers`
object. Copy with `clone_this` instead, which is the path
`new Response(_, { headers })` already takes. All six `cast`/`cast_` callers
were audited; this was the only one that adopted the borrow. Regression test
added.
The rest of the diff converts the `unsafe { &mut *ptr }` pattern this grew
out of: raw-pointer fields become the in-tree owners (`JsCell`, `Cell`,
`ParentRef`, `BackRef`, `Box<Self>` receivers) across the runtime, and the
receivers that only ever needed `&self` say so. No allocation, lock, `Rc`,
`Arc`, or refcount is added anywhere in the diff.
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.
Collaborator
|
Updated 4:18 AM PT - Jul 10th, 2026
❌ @Jarred-Sumner, your commit 4cd86af has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33908That installs a local version of the PR into your bun-33908 --bun |
Contributor
|
Found 5 issues this PR may fix:
🤖 Generated with Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Temporary, will be closed. Bisecting an
s3.test.tscrash that only reproduces in CI (needs R2 credentials).Known:
mainis green, and the stack's first commit is green (#33820). The crash enters somewhere in commits 2-4.Crash is
panic: calledOption::unwrap()on aNonevalueatBlob.rsWrapper::resolve->JSPromiseStrong::get(), reached viaon_response -> error_with_body -> Callback::fail: the upload callback settles an already-cleared promise.This PR is that stack's commit 2 in isolation. Only the
s3.test.tsresult matters.