Fix use-after-free of stream error objects parked in ByteStream#31963
Conversation
|
Updated 6:00 PM PT - Jun 21st, 2026
❌ @robobun, your commit c129560 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 31963That installs a local version of the PR into your bun-31963 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
WalkthroughStream errors are now protected and released with explicit GC pairing: a new StreamError::strong protects JS values, ByteStream/ResumableSink release parked errors safely, multiple call sites create strong stream errors, and an integration test verifies GC safety for parked S3 errors. ChangesStream error GC lifecycle fixes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/streams.rs (1)
339-362:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
Clonere-protect strong JS errors.
StreamError::strong()turnsStreamError::JSValueinto an owning handle, but#[derive(Clone)]still bit-copies that ownership. A cloned strong error can now reach multipleunprotect()paths off a singleprotect(), which reopens the same GC/UAF class this PR is fixing. Please either dropClonehere or implement it manually so cloning a strong variant callsprotect()again.♻️ Proposed fix
-#[derive(Clone)] pub enum StreamError { Error(SysError), AbortReason(CommonAbortReason), // TODO: use an explicit jsc.Strong.Optional here. JSValue(JSValue), WeakJSValue(JSValue), } + +impl Clone for StreamError { + fn clone(&self) -> Self { + match self { + Self::Error(err) => Self::Error(err.clone()), + Self::AbortReason(reason) => Self::AbortReason(reason.clone()), + Self::JSValue(value) => { + value.protect(); + Self::JSValue(*value) + } + Self::WeakJSValue(value) => Self::WeakJSValue(*value), + } + } +}As per coding guidelines, every acquisition must have exactly one owner/release, and JS values kept alive on the JS thread need explicit ownership semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/webcore/streams.rs` around lines 339 - 362, The derived Clone for StreamError bit-copies the protected JSValue so cloning a strong variant duplicates ownership and allows multiple unprotects; remove #[derive(Clone)] and implement Clone manually for StreamError: match each variant and clone its payload normally for Error(SysError) and AbortReason(CommonAbortReason) and for WeakJSValue(JSValue) simply clone the JSValue, but for the strong JSValue variant call value.protect() on the JSValue being cloned before returning StreamError::JSValue(cloned_value) so each clone acquires its own protect; leave WasStrong as-is.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/s3/s3-stream-error-gc.test.ts`:
- Around line 16-19: Replace the .then() chain used to capture the error from
stream.text() with async/await and a try/catch: await stream.text() inside a try
block and assign the caught error to the existing err variable in the catch
block so the test follows the repo rule for promises; update any surrounding
async function or test to ensure it is marked async if not already, and keep
references to err and stream.text() intact.
---
Outside diff comments:
In `@src/runtime/webcore/streams.rs`:
- Around line 339-362: The derived Clone for StreamError bit-copies the
protected JSValue so cloning a strong variant duplicates ownership and allows
multiple unprotects; remove #[derive(Clone)] and implement Clone manually for
StreamError: match each variant and clone its payload normally for
Error(SysError) and AbortReason(CommonAbortReason) and for WeakJSValue(JSValue)
simply clone the JSValue, but for the strong JSValue variant call
value.protect() on the JSValue being cloned before returning
StreamError::JSValue(cloned_value) so each clone acquires its own protect; leave
WasStrong as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d87c9cb-3063-4751-a7f9-95aca6ccc302
📒 Files selected for processing (8)
src/runtime/server/RequestContext.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/ByteStream.rssrc/runtime/webcore/ResumableSink.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/streams.rstest/js/bun/s3/s3-stream-error-gc.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Regarding the out-of-band finding on |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/Body.rs`:
- Around line 2312-2323: on_stream_pipe currently ignores streams::Result::Err
and always calls (self.on_finished_buffering) with the success path; change it
to detect when stream.is_err() (or pattern-match streams::Result) and propagate
that failure into the callback by calling (self.on_finished_buffering)(self.ctx,
bytes_or_partial, Some(ValueError::from(stream.error())) , true/false as
appropriate) instead of silently releasing only; ensure you still call
stream.release() to drop gcProtect but build and pass a proper ValueError (or
JSValue error) to self.on_finished_buffering so the error path is taken by
callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c36ee5e-9946-429b-bad6-93522ce919b7
📒 Files selected for processing (2)
src/runtime/server/RequestContext.rssrc/runtime/webcore/Body.rs
There was a problem hiding this comment.
Thanks for addressing both findings — the e92d3c1 change to call stream.release() in RequestContext::on_pipe and ValueBufferer::on_stream_pipe closes the piped-error leak. I didn't find anything further, but given this rebalances gcProtect/gcUnprotect across ~10 producer/consumer sites in core stream/fetch/server paths, it's worth a human pass to sanity-check the invariant.
Extended reasoning...
Overview
This PR fixes a use-after-free where StreamError::JSValue error objects were stored unrooted in ByteStream.pending.result across event-loop turns, allowing GC to collect them before consumption. The fix introduces StreamError::strong() (which takes a gcProtect) at all four producer sites (S3 download, fetch tasklet, request body size limit, Body::ValueError::to_stream_error), and audits every consumer path to release exactly once — BufferAction::reject, ByteStream::{on_data, append, finalize, to_buffered_value}, ResumableSink::init, and (after my earlier finding) the two remaining PipeHandler impls in RequestContext::on_pipe and ValueBufferer::on_stream_pipe. Clone is removed from StreamError so the protect count can't be aliased by a bitwise copy. A new regression test (s3-stream-error-gc.test.ts) reproduces the UAF deterministically.
Security risks
The original bug is a GC use-after-free in production code paths — promise rejection with a recycled cell is type confusion, and the debug build hits a StructureID assertion. The fix itself is the security improvement; the residual risk is an unbalanced protect count (leak if a consumer path is missed, potential over-unprotect if a path releases twice). I traced every StreamError::JSValue construction (now all via strong()) and every consumption site I could find, and the balance looks correct after e92d3c1, but this is exactly the class of invariant where a second pair of eyes is valuable.
Level of scrutiny
High. This touches the core streaming error lifecycle across ByteStream, ResumableSink, fetch, S3, the HTTP server's request body path, and Body buffering — every stream-error path in the runtime. The invariant ("one protect per JSValue variant, released exactly once") is enforced only by convention plus the Clone removal; a missed or duplicated release in any of the ~10 sites would be a regression. My first review of this PR did find one such gap (the two PipeHandler impls), which was fixed in e92d3c1.
Other factors
- Both of my prior inline findings (test stderr snapshot,
PipeHandlerprotect leak) and CodeRabbit's nit have been addressed. - The bug-hunting system found nothing on the latest revision.
- No CODEOWNERS coverage for these paths.
- Two earlier PRs (#29822, #29745) were subsets of this fix scoped to single producer sites, suggesting this area has been tricky to get right.
- The
mem::replace-based ownership transfers inByteStream::on_dataandResumableSink::initlook correct, andto_buffered_valuenow resets toDoneafter release to prevent a double-unprotect — but these are subtle enough that a maintainer familiar with the JSC protect model should confirm.
|
CI status for the latest build (#61345, commit 99e6776): 2 failures, both unrelated to this change.
The earlier build #61344 additionally had six unrelated tests failing together on a single macOS aarch64 runner (docker builds, mysql, websocket-proxy, init), which cleared on the rebuild. |
|
Build #61669 (after the merge from main) had no test failures: 68 jobs ran and passed, 25 jobs expired waiting for an agent (macOS and linux capacity), and the remaining 193 steps cascaded to waiting_failed off those. No job exited non-zero. The two earlier blockers are gone: the tree-wide bunx.test.ts failure is skipped upstream (#32042, now included in this branch), and the duckdb segfault did not reproduce. Retrying the failed jobs on #61669, or a fresh build, should go green. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
- Rebase
- Remove usages of unsafe
- Prefer Drop() over manually freeing things
- Delete code comments
StreamError::JSValue held a raw JSValue with no GC root while parked in ByteStream.pending.result across event-loop turns. A GC between the error arriving and the stream being consumed freed the error object, so the promise rejected with a recycled unrelated object or a dead cell (StructureID decode assertion). Change StreamError::JSValue to hold a jsc::strong::Optional, which keeps the value rooted while held and releases on Drop. All four producers now create a Strong; the WasStrong enum, to_js_weak, the consumer-side unprotect branches, and the unused WeakJSValue variant are deleted. The manual release scaffolding in RequestContext::on_pipe and ValueBufferer::on_stream_pipe (including the raw-pointer scopeguard) is removed since the by-value result now drops at scope exit in the trait impl.
robobun
force-pushed
the
farm/6c72759e/s3-stream-error-gc-root
branch
from
bb98e88 to
c129560
Compare
|
Reworked in c129560 on top of current main (7a5293c):
Net change is 8 files, +105/-132. |
|
Build #63845 (the
No stream, fetch, S3, or serve test failures; the suites that exercise the changed code paths all passed. |
There was a problem hiding this comment.
No issues found in the reworked jsc::strong::Optional-based approach — it's cleaner than the earlier protect/unprotect version and the by-value drop in the PipeHandler trait impls now covers the leak path I flagged before. Still deferring given this reshapes GC-rooting/ownership across eight runtime files (streams, fetch, S3, Bun.serve); worth a human pass.
Extended reasoning...
Overview
This PR fixes a use-after-free where StreamError::JSValue held a raw, unrooted JSValue that could be GC'd while parked in ByteStream.pending.result. The final revision (c129560) replaces the payload with jsc::strong::Optional, which roots on create and unroots in its Drop impl (verified at src/jsc/Strong.rs:169). That lets the PR delete the WasStrong enum, to_js_weak, the unused WeakJSValue variant, the Clone impl on StreamError, and the manual scopeguard::defer!/unsafe cleanup in RequestContext::on_pipe — the trait-level on_pipe(&mut self, stream: StreamResult) now owns the value and forwards it by & to the inherent method, so both the Err strong handle and the Owned* Vec<u8> free naturally on scope exit. Touches streams.rs, ByteStream.rs, Body.rs, ResumableSink.rs, ReadableStream.rs, RequestContext.rs, FetchTasklet.rs, s3/client.rs, plus a new regression test.
Security risks
None identified. This is a memory-safety fix (GC rooting), not an auth/permission/input-validation surface. The failure mode being fixed is a UAF (potential type confusion via recycled JSC cells), so the change reduces risk rather than adding it.
Level of scrutiny
High. This is production-critical runtime code on hot paths (every fetch/S3/Bun.serve streamed body error flows through here), and the change reshapes ownership semantics: StreamError loses Clone, on_data now mem::replaces the parked error instead of cloning, on_pipe switches from owning+manual-cleanup to borrow+natural-Drop, and upload_stream in s3/client.rs now moves the pending error out via mem::replace before reading it. Each of these is individually straightforward, but the earlier revision of this same PR had a real protect-leak regression (which I flagged and which was fixed), and the approach was then substantially reworked — so this is exactly the class of change where a second human reading the final diff is worthwhile.
Other factors
The bug-hunting system found nothing on the current revision. All prior review threads (mine, coderabbit's) are resolved, and the test now snapshots {stdout, stderr, exitCode} per the earlier feedback. The new test deterministically reproduces the UAF on an unfixed build. The PR description accurately reflects the final shape (Drop-based, not the intermediate StreamError::strong()/WasStrong design that coderabbit's stale walkthrough still describes). CI on the latest commit was building at the time of review; earlier builds were green modulo unrelated/infra failures. I'm not approving solely because of scope and subtlety, not because of any specific concern with the current diff.
2 participants
What does this PR do?
Fixes a use-after-free found by Fuzzilli (fingerprint
StructureID.h(93),ASSERTION FAILED: decontaminate()inStructureID::decode()).StreamError::JSValueheld a rawJSValuewith no GC root. When an S3 download stream failed before a consumer attached (missing credentials fail synchronously inside.stream()), the error object was parked inByteStream.pending.resultacross event-loop turns. A GC before the stream was consumed freed it, so.text()rejected with whatever now lived in the recycled cell, or crashed on a dead cell. Deterministic on an unfixed build:The fetch, request body size limit, and
Body::ValueError::to_stream_errorproducers had the same shape.The fix
StreamError::JSValuenow holdsjsc::strong::Optional, which keeps the value rooted while held and releases it onDrop(implements the existing TODO on the variant).This deletes most of the surrounding machinery:
to_js_weakbecomesto_js, theWasStrongenum and every consumer-sideif was_strong == Strong { unprotect() }branch are gone, the unusedWeakJSValuevariant is gone,StreamErroris no longerClone,RequestContext::on_pipedrops its raw-pointerscopeguard::defer!/unsafeblock (the by-value result drops naturally in the trait impl), andByteStream::on_datamoves a parked error instead of cloning it.How did you verify your code works?
test/js/bun/s3/s3-stream-error-gc.test.tsfails on the unfixed build (rejects with a recycled decoy object) and passes with this change.test/js/web/streams/streams.test.js,test/js/web/fetch/{body-stream,body-mixin-errors,fetch.stream}.test.ts,test/js/web/streams/readable-stream-blob-consumed.test.ts, andtest/js/bun/http/serve-pending-promise-abort-leak.test.tsagainst the debug build; only behavioral delta vs an unmodified build is the new test passing (the remaining failures are pre-existing 5s debug+ASAN timeouts that reproduce identically on the unmodified debug build).cargo clippy -p bun_runtimeandbun run rust:check-allpass.