Conversation
An upload's `process_multi_part` took the buffered bytes out of the upload and handed the new `UploadPart` a raw pointer to them, then transferred the allocation with `ManuallyDrop` only after `enqueue_part` returned `Ok`. An `Err` skipped that transfer, so the part and the local `StreamBuffer` both owned the bytes: the upload's failure freed them through the part, and the buffer freed them again as the error unwound. The part now holds a `Vec<u8>` it takes when it is created, so the buffer gives the bytes up at the same point the part receives them. The queue's `Drop` frees what a part still owns, which the raw pointer never did.
|
Status: the fix and its test are pushed. This PR is ready for review. How I reproduced it. A host process runs a loopback S3 stand-in and 4 workers. Each worker starts 8 uploads with
The test is |
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughMultipart upload parts now own payloads through reference-counted vectors. Enqueueing selects a queue slot before moving or copying data. Request and cleanup paths release payload references. A worker-termination regression test verifies clean process completion. ChangesMultipart payload ownership
Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the removed queue-full write-back of owned into buffered in process_multi_part — it is behavior-preserving, since take_data never runs when get_create_part finds no free slot, so the buffer is left untouched (and cursor == 0 && size() == len means take().list is exactly the part's bytes). The retry-recursion and queueSize > 1 post-failure allocation paths were examined and ruled out as regressions introduced here.
Extended reasoning...
The change replaces UploadPart's raw-pointer/allocated_size ownership with a Vec moved in via a closure that only runs once a queue slot exists, removing the ManuallyDrop and Vec::from_raw_parts free in src/runtime/webcore/s3/multipart.rs, plus a new worker-terminate ASAN repro test. It touches no auth/crypto surface. Two inline findings (a dangling body slice inside the S3 client on the stopping-VM path and an incomplete proxy env scrub in the test) remain for a human to weigh, which is why this is not an approval.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/webcore/s3/multipart.rs— A part's request body slice is left dangling inside the S3 client whenever a worker terminates mid-upload, the exact case this PR targets. perform() at multipart.rs:357 passesself.data()(a&[u8]into the JsCell's Vec) into execute_simple_s3_request; on the stopping-VM branch that call runs on_part_response synchronously, which calls free_data() at multipart.rs:303 and drops the Vec through with_mut while the caller'sbodyborrow is still live. REVIEW.md forbids a slice outliving its memory and comment-enforced safety; the only guard is the doc comment at multipart.rs:268. Fix: perform() must not lend the Vec across a call that can free it: take the Vec out (or clone into the request's Box body first) and only free on the non-reentrant path.Why this was flagged
Trigger: any streaming multipart upload whose part request starts while the VM is stopping (worker.terminate(), the PR's own test) or whose sign_request fails. Entry: process_multi_part -> enqueue_part -> part.start() -> perform() (multipart.rs:335-365). perform() builds S3RequestOptions with
body: self.data()at multipart.rs:357; data() at :271 returns&[u8]derived from JsCell::get(), a shared borrow of the Vec. execute_simple_s3_request (simple_request.rs:601-608) calls callback.fail synchronously; on_part_response then calls free_data() at multipart.rs:303 (or :285), which is JsCell::set -> with_mut -> mem::replace (js_cell.rs:83-103), forming&mut Vecand deallocating the buffer whileoptions.bodyin the outer frame still points at it. On the base branch the same shape existed with a raw pointer; the PR turned it into a Rust reference, so it is now an aliasing violation and a dangling reference, not just a stale raw pointer. Nothing reads options.body after the fail today, so the guard is only the comment at :268-269; any future read (logging, verbose fetch, a body-length…Verification: pre-existing. acknowledged in diff: multipart.rs:268-269 ("The request
performhands these bytes to copies them, and can fail the upload before it returns, which releases them") and the PR description ("Left alone:performstill lends the part's bytes to a request that can free them. Nothing reads them after") — the stated bound is accurate. Trigger: a part's request starts while the VM…
`UploadPart::perform` lent the part's bytes to `execute_simple_s3_request`, which reports a request that fails before it leaves inside that call. The report frees the part, so the lent slice pointed at freed memory until the call returned. Nothing read it, and only a comment said so. The bytes are now an `Rc<Vec<u8>>`. `perform` holds a count for the length of the call, so the report drops the part's count and the bytes go when the call returns. The test clears `ALL_PROXY` as well, which the S3 client falls back to, and its comment no longer says that the client ignores `NO_PROXY`.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I re-reviewed after the second commit and found no bugs; the earlier ALL_PROXY/comment point is addressed. Because this reworks ownership of a part's bytes in a native path with synchronous re-entrant failure callbacks (and overlaps a pending rewrite of process_multi_part), a human look would still be worthwhile.
What was reviewed:
- Every former
free_allocated_slicesite now callsfree_data; the queueDropreleases whatever a part still holds, andcancelonly frees in thePendingstate as before. perform's localRcclone:execute_simple_s3_requestcopies the body intoBox<[u8]>before returning, so the borrowed slice is never stored past the call, and the clone keeps it valid ifcallback.failrunson_part_responsesynchronously.take_dataruns only after the queue slot is reserved, so theOk(false)path leavesbuffereduntouched;buffered.take().listis exact because the branch requirescursor == 0 && size() == len; the slice closure'sbuffered.get()borrow ends before anybufferedmutation.- Test: hermetic loopback stand-in, proxy vars scrubbed including
ALL_PROXY, pipes drained concurrently, output asserted before the exit code.
Extended reasoning...
The change replaces a raw-pointer-plus-length ownership scheme for S3 multipart part bytes in src/runtime/webcore/s3/multipart.rs with JsCell<Option<Rc<Vec>>>, moves the buffer-to-part handoff into a closure that runs only once a queue slot exists, and adds a worker-terminate regression test. It touches no auth, injection, or data-exposure surface; the S3 signing path is unchanged. The diff is small and I traced every free/acquire path and the request layer's body copy, but it is native lifecycle code with synchronous re-entrant failure callbacks, the most-blocked category in this repo's review history, and the description notes another open PR rewrites the same function, so coordination is a maintainer call rather than an approval I can make alone.
One line each. The field's comment says why the bytes are counted, so `perform` does not repeat it.
|
The review findings are addressed.
I ran |
There was a problem hiding this comment.
I re-reviewed the current head (through 0c3bdf3) and didn't find any bugs; the proxy-env point from my earlier inline comment is addressed in 77c5c0a. Because this rewrites native memory ownership in the S3 multipart path and the PR notes #31733 rewrites the same function, a human look would still be worthwhile.
What was reviewed:
- Traced both
process_multi_partbranches:take_dataruns only afterget_create_parthas reserved a slot, so onOk(false)the buffer is untouched and onErrthe part is the sole owner of the takenVec(the old re-append dance is correctly gone). - Checked the
performkeepalive:execute_simple_s3_requestcopiesoptions.bodyintotask.bodybefore the HTTP thread sees it, so theRcclone only needs to cover the synchronouscallback.failpath, which it does. - Confirmed no remaining references to
allocated_size/free_allocated_slice/needs_cloneinsrc/runtime/webcore/s3/, and thatdetect_leaks=0in a spawned fixture with a stated reason matches existing tests (bun-write, compression, websocket). - Not done here: I did not build or run the new test, so the "aborts on the unfixed build" claim rests on the PR description and CI.
Extended reasoning...
The change touches one native file, src/runtime/webcore/s3/multipart.rs, replacing UploadPart's raw-pointer plus allocated_size ownership (freed via unsafe Vec::from_raw_parts and ManuallyDrop) with JsCell<Option<Rc<Vec>>> and a take_data closure that only runs once a queue slot exists, plus a new spawned ASAN fixture test that terminates workers mid-enqueue. It touches no auth, injection, or data-exposure surface; the S3 signing and request code is unchanged. The bug-hunt ran dry with no findings, the diff removes unsafe code rather than adding it, and my earlier inline finding was addressed by a follow-up commit. I deferred rather than approved because this is a memory-ownership rewrite in native code (the most-blocked category per REVIEW.md), I could not compile or run the test in this environment, and the author notes an in-flight PR (#31733) rewriting the same function that a maintainer needs to sequence.
`get_create_part` and `enqueue_part` were generic over the closure that produced the part's bytes, so each was compiled once per call site although almost none of their code used the closure. The bytes always come from the upload's buffer, either all of them or a copy of the first part of them, so `PartBytes` says which and `get_create_part` takes them itself, still only once the queue has a slot.
|
Updated 2:37 PM PT - Sep 21st, 2026
✅ @robobun, your commit 0765a97af0635b5911037ac761dfae8dee830bb5 passed in 🧪 To try this PR locally: bunx bun-pr 43709That installs a local version of the PR into your bun-43709 --bun |
There was a problem hiding this comment.
I reviewed the latest revision and didn't find any bugs; since this rewrites who owns a multipart part's bytes in native S3 code, a human look at the ownership paths would still be worthwhile.
What was reviewed:
- Every exit of
enqueue_part:Wholetakesbufferedonly after a queue slot is found (soOk(false)leaves the buffer intact andErrleaves the part as the single owner);First(len)copies and advances the cursor only onOk(true). performclones theRcacrossexecute_simple_s3_request, which copies the body into its ownBox<[u8]>(simple_request.rs:677), so an in-place failure that frees the part cannot leave the request reading freed memory;sentis read beforefree_dataon the ETag path.- The test's proxy scrub now covers ALL_PROXY/all_proxy, which resolves the earlier inline note. Only remaining nit: the host awaits the worker's "started" message without wiring the worker
error/exitevents, so a worker that throws before posting hangs to the runner timeout instead of failing clearly.
Extended reasoning...
The change replaces UploadPart's raw slice pointer plus manual Vec::from_raw_parts free with an Rc<Vec> and moves the take-from-buffer step inside get_create_part behind the queue-slot check, removing the ManuallyDrop juggling that left two owners when the request failed in place; it adds one spawned test that terminates workers mid-enqueue. It touches no auth, injection, or data-exposure surface; the sensitive surface is native memory ownership in src/runtime/webcore/s3/multipart.rs. I traced the Ok(false)/Ok(true)/Err paths for both PartBytes variants and the in-place-failure path through perform and found the ownership balanced, and the prior ALL_PROXY finding is addressed in the current test. Deferring rather than approving because this is a memory-safety fix in native runtime code that shipped with a double free before, and the unfixed-build failure claim for the test was not independently reproduced here.
… starts The host waited only for each worker's message, so a worker that threw or exited first left the run to the runner's timeout. Its `error` and `exit` events now reject the wait, and the host prints the error and exits 1.
|
The remaining test nit is addressed in 0765a97. The host now rejects its wait on a worker's |
There was a problem hiding this comment.
I reviewed the latest push and found no bugs; since this reworks who owns a part's bytes across a failure callback that can re-enter the upload, a human look at the ownership model would still be worthwhile.
What was reviewed:
get_create_partmoves bytes out ofbufferedonly after a queue slot is found, soOk(false)leaves the buffer untouched and the removed re-append fallback had no remaining purpose; onErrthe part is the single owner.performclones theRcbeforeexecute_simple_s3_request; the request copies the body (Box::from(options.body)in simple_request.rs), so nothing references the part's bytes after the call returns and a synchronouscancelinside it cannot invalidate the slice.MultiPartUpload::dropnow frees data parts still hold; in-flight part requests hold a ctx ref, so that cannot race a request.- The test now clears every proxy variable the client consults and rejects on early worker exit, addressing the earlier inline thread.
Extended reasoning...
The change touches src/runtime/webcore/s3/multipart.rs, replacing a raw Cell<*const [u8]> plus manual Vec::from_raw_parts free with JsCell<Option<Rc<Vec<u8>>>> and a PartBytes enum, and adds test/js/bun/s3/s3-upload-terminate.test.ts. No security-sensitive surface (no auth, injection, or data-exposure paths); it is a memory-ownership fix in native runtime code. Deferring rather than approving because the correctness depends on the reentrant failure path inside execute_simple_s3_request freeing the part while the caller is mid-call, and the regression test is a probabilistic race reproduction that relies on ASAN to observe the old double free. No CODEOWNERS entry covers the changed files, and the bug hunt ran dry with no findings.
This branch has not been deployed
2 participants
Problem
attempting double-free ... in thread T.. (Worker), reported inMultiPartUpload::process_multi_part(src/runtime/webcore/s3/multipart.rs:991), freed first byUploadPart::cancel(multipart.rs:273).process_multi_partgives the new part a raw pointer to the buffer's bytes. It gives the allocation up withManuallyDroponly afterenqueue_partreturnsOk. AnErrskips that and leaves two owners.terminate()produces thatErr. The client refuses the request for a stopping VM, the failure frees the part's bytes, and the error unwinds through the buffer'sDrop.Fix
UploadPart::datais anRc<Vec<u8>>.get_create_parttakes the bytes out of the buffer itself (all of them, or a copy of the first part), and only when the queue has a slot. The raw pointer,allocated_size,needs_cloneand theManuallyDropare gone.performholds a count while it lends the bytes to a request, because a request that fails before it leaves frees the part inside that call.test/js/bun/s3/s3-upload-terminate.test.ts(20 of 20 runs abort without this change, clean with it). Alsotest/js/bun/s3/and theworker_threads.test.tsteardown rows.Background
UploadPartand sends them as one PUT.failandDropin the same file. Whichever lands second rebases.Notes
Repro. A host process runs a loopback S3 stand-in and 4 workers. Each worker starts 8 uploads with
client.write(key, new Response(stream), { partSize: 5 MiB, queueSize: 1, retry: 0 }), where the stream's first chunk is exactly one part, then posts a message. The host terminates each worker as its message arrives. The first write of an upload is the only one that sends the request that creates the upload, so the terminate has to land before it. With several uploads per worker and several workers that happens on almost every run: 20 of 20 runs of the unfixed debug build aborted (19 of 20 with 4 uploads per worker). Every run with the change was clean. The test takes 1.8 s on the debug build.The failing path, from the ASAN report on the unfixed build. Second free:
process_multi_part(multipart.rs:991, the end of the one-big-chunk block, where the localStreamBufferdrops) fromprocess_bufferedfromwritefromNetworkSink::writefrom the stream pump (rsisSinkWrite), in the worker's ordinary microtask drain. First free:free_allocated_slice(:273) fromUploadPart::cancel(:393) fromfail(:594) fromstart_multi_part_request_result(:689) fromCallback::failfromexecute_simple_s3_request(simple_request.rs:603, the branch for a VM that sends nothing new) fromenqueue_part(:905) fromprocess_multi_part(:967). So the refused request is the one that creates the upload, for the upload's first part.Why only that block. The other block of
process_multi_partcopies a slice of the buffer for the part, so the part and the buffer never share an allocation. AnErrthere leaves the buffer's cursor where it was, which does not matter: everyErrout ofenqueue_partcomes fromfail, which finishes the upload, and a finished upload sends nothing more.Release builds. A release build has no such check. The second free hands mimalloc a block it already has back, and what follows is undefined. Rare segfaults of a release build under this workload are consistent with that, but this change does not prove the link.
The bytes
performlends. The first revision of this PR kept aVec<u8>in the part, andperformpassed a slice of it toexecute_simple_s3_request. That call reports a request that fails before it leaves from inside itself, and the report frees the part, so the slice pointed at freed memory until the call returned. Nothing read it, and the previous raw pointer had the same shape, but only a comment said so. The bytes are counted now andperformholds a count for the length of the call. The queue'sDropfrees what a part still owns, which the raw pointer never did.What this does not change. The leak of an upload that is still open when its worker goes (#39692) is separate, which is why the test's child process runs with
detect_leaks=0. Five sibling S3 tests clearHTTP_PROXYandHTTPS_PROXYbut notALL_PROXY, which the client falls back to. This PR's test clears all of them, and the siblings are a separate change.Other suites.
test/js/bun/s3/: 160 pass, ands3-list-objects.test.ts"Should fall back to NoSuchKey ..." times out on the debug build when the whole file runs. It passes alone, it exercisesclient.list()only, and #38353 recorded the same timeout on an unmodified debug build.worker_threads.test.ts -t "VM teardown": 3 pass.test/internal/source-lints/: 188 pass.cargo clippy -p bun_runtime --no-deps,cargo fmt --checkand prettier are clean.[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file