Protect ByteStream pending error JSValue across GC#32579
Conversation
When a ByteStream source (S3, fetch, request body) stores a StreamError::JSValue into pending.result via append(), the value was not GC-protected. If GC ran before the consumer (text()/bytes()/etc.) read it back in to_buffered_value(), the error object was collected and the subsequent JSPromise::setSlot hit a freed cell (decontaminate() assert / SIGILL). StreamResult::release() already unprotects StreamError::JSValue, so protect it at the point it is parked in pending.result for later consumption.
|
Updated 4:45 PM PT - Jun 21st, 2026
❌ @robobun, your commit 32db77f has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32579That installs a local version of the PR into your bun-32579 --bun |
|
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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughIn ChangesGC protection for ByteStream JSValue errors
🚥 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.
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/ByteStream.rs (1)
296-302:⚠️ Potential issue | 🔴 CriticalAdd
protect()call at line 299 to match error-handling pattern at lines 368 and 394.Line 299 stores
StreamError::JSValuewithout callingprotect()first, while the parallel error-handling paths inappend(lines 368 and 394) both protect JSValue errors before storage. Since JSValue errors are delivered toon_data()from FetchTasklet (confirmed at FetchTasklet.rs:670), this code path is reachable and exposes the same UAF risk: the JSValue is stored inpending.resultwithout a GC root, allowing premature collection whilependingstill holds the reference.Apply the same fix as lines 368 and 394:
if let streams::StreamError::JSValue(v) = &err { v.protect(); }Insert before line 299 (
p.result = streams::Result::Err(err.clone());).🤖 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/ByteStream.rs` around lines 296 - 302, The code at line 299 storing a StreamError to pending.result is missing the protect() call that is applied in the parallel error-handling paths at lines 368 and 394. Before storing the error via p.result = streams::Result::Err(err.clone()), add a guard that checks if the error is a StreamError::JSValue variant and calls protect() on it to prevent premature garbage collection. Insert this protection check immediately before the line that assigns the error to pending.result, matching the exact pattern used in the append method's error-handling blocks to ensure consistent JSValue lifecycle management across all error storage paths in ByteStream.
🤖 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.
Outside diff comments:
In `@src/runtime/webcore/ByteStream.rs`:
- Around line 296-302: The code at line 299 storing a StreamError to
pending.result is missing the protect() call that is applied in the parallel
error-handling paths at lines 368 and 394. Before storing the error via p.result
= streams::Result::Err(err.clone()), add a guard that checks if the error is a
StreamError::JSValue variant and calls protect() on it to prevent premature
garbage collection. Insert this protection check immediately before the line
that assigns the error to pending.result, matching the exact pattern used in the
append method's error-handling blocks to ensure consistent JSValue lifecycle
management across all error storage paths in ByteStream.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9cbba5ed-6e80-448a-92cf-6be89b9cd424
📒 Files selected for processing (2)
src/runtime/webcore/ByteStream.rstest/js/bun/s3/s3.test.ts
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Re: the coderabbit note on That branch is only reached when The two |
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 `@test/js/bun/s3/s3-stream-error-gc.test.ts`:
- Around line 32-36: The stderr output from the process is being collected but
is not asserted in the final expectation. Modify the expect statement to include
stderr in the expected object alongside stdout and exitCode, so that the
assertion validates all three process outputs: stderr (with trim() applied),
stdout (already present), and exitCode (already present). This will ensure
unexpected runtime warnings or errors in stderr are caught and prevent false
positives.
🪄 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: 980e4ab7-8423-4392-af82-c64febcac5e0
📒 Files selected for processing (1)
test/js/bun/s3/s3-stream-error-gc.test.ts
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
This is the minimal two-line version of #31963, which takes the broader route of protecting at every |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
test/js/bun/s3/s3.test.ts:1617-1621— nit:stderris read into a local but dropped from theexpect, so if this regression ever recurs (child crashes with SIGILL/abort) the crash diagnostic on stderr won't appear in the CI failure diff. Per CLAUDE.md's subprocess-test convention, includestderrin the asserted object — e.g.expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ stdout: ..., stderr: expect.any(String), exitCode: 0 })— so it surfaces on failure without requiring it to be exactly empty (ASAN/debug builds emit benign warnings).Extended reasoning...
What's wrong
The new test at
test/js/bun/s3/s3.test.ts:1617pipesstderr, awaitsproc.stderr.text(), and destructures it into astderrlocal — but the assertion on the next line only checks{ stdout: stdout.trim(), exitCode }. Thestderrvalue is read and then silently discarded.Why it matters here specifically
This test exists to catch a GC crash: before the fix, the child process died with SIGILL (release) or an
ASSERTION FAILED: decontaminate()abort (debug). In both cases the only useful diagnostic — the assertion text, the crash backtrace, the signal — lands on stderr, not stdout. If this regression (or a related one) ever reappears, the test will fail with a diff like:- exitCode: 0 + exitCode: 132 - stdout: "rejected ERR_S3_MISSING_CREDENTIALS" + stdout: ""…and nothing else. Whoever triages it in CI has no idea why the child died, because the stderr that explains it was thrown away.
Repo convention
CLAUDE.md:197("Tests reviewers reject") is explicit about this pattern:Subprocess tests: … assert a combined
{ stdout, stderr, exitCode }object.The same line also says:
Never assert stderr is exactly empty (ASAN/debug builds emit benign warnings)
So the fix is not
stderr: ""— that would flake on ASAN builds. Instead, includestderrin the asserted object with a permissive matcher so it shows up in the failure diff without constraining the pass case:expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ stdout: "rejected ERR_S3_MISSING_CREDENTIALS", stderr: expect.any(String), exitCode: 0, });
(or wrap with
normalizeBunSnapshotper the surrounding test conventions — either way, the point is thatstderrparticipates in the diff.)Step-by-step proof
- Revert the
v.protect()calls inByteStream::append. - Run this test. The child crashes;
exitCodeis 132 (or 134 in debug),stdoutis empty,stderrcontainsASSERTION FAILED: decontaminate()plus a stack trace. - With the current assertion, Jest's diff shows only the stdout/exitCode mismatch —
stderris not part of the compared object, so the crash message never appears in the test output. - With
stderrincluded in the object, the same failure prints the full stderr in the "Received" half of the diff, immediately telling the reader what crashed.
Severity
Nit — this is test diagnostics quality, not a correctness bug in the fix itself. The runtime change is fine; this just makes the guard test more useful when it fails.
- Revert the
append() now protects a parked StreamError::JSValue, so on_cancel must release it even when no pull is outstanding (view is empty). Previously the release was inside the !view.is_empty() guard, and finalize() skips its own release once done is set, so a cancel-without-pull would leak the protect handle.
Matches every other release site in this file so a stale Err(JSValue) with an already-unprotected handle is never observable.
|
CI status:
The fix is verified locally and all review feedback is applied. Retrying the failed shards once GitHub recovers should be enough. |
There was a problem hiding this comment.
All three of my earlier comments are addressed and I found nothing new on 32db77f, but this is GC protect/unprotect bookkeeping in a core stream path (S3/fetch/serve bodies) and there's still the open question of landing this vs #31963 — worth a human pass.
Extended reasoning...
Overview
Adds protect() on StreamError::JSValue at the two ByteStream::append sites that park an error in pending.result, plus three follow-up consistency fixes from review: release() before overwrite in both append Err arms, unconditional release() in on_cancel, and release() + reset to Done (with ensure_still_alive()) in to_buffered_value. One new regression test spawns a child with BUN_JSC_slowPathAllocsBetweenGCs=5 and verifies the parked error survives a forced GC.
Security risks
None in the conventional sense. The change is a memory-safety fix (UAF → crash) in the JS runtime; the risk profile is over/under-protect rather than auth/injection/data exposure. An unbalanced protect() would leak a GC root; an unbalanced unprotect() would reintroduce a UAF. After three review iterations the visible mutation sites in this file all pair release() with a reset to Done or a fresh value.
Level of scrutiny
High. ByteStream backs ReadableStream sources for S3, fetch response bodies, and Bun.serve request bodies — production-critical paths. JSC protect/unprotect discipline is notoriously easy to get subtly wrong: the original two-line fix introduced two latent leaks (overwrite-without-release, and the on_cancel no-pull-outstanding path) that only surfaced under review. That history argues for a maintainer who knows the StreamResult/Pending ownership model to confirm no other teardown path was missed.
Other factors
- All three of my prior inline comments are applied and resolved; the bug hunter found nothing on 32db77f.
- A competing PR (#31963) takes the broader
StreamError::strong()approach at every producer; the author has offered to close whichever isn't wanted. That's a design call a maintainer should make. - CI on cc52e27 was 285/286 with one known-flaky
fetch-leak.test.tsunrelated to the Err path; 32db77f's only failures so far are infra (GitHub tarball 5xx). The musl build failures noted on f3dee09 should be confirmed resolved before merge.
|
Build 63838 finished 280/286. Remaining failures:
darwin-14-aarch64, which flaked on |
1 participant
Fuzzilli found a flaky crash (fingerprint
StructureID.h(76),ASSERTION FAILED: decontaminate()) that reproduces reliably with:What happened
When an S3 download fails synchronously during
.stream()(for example, missing credentials), the error is delivered toByteStream::on_dataasStreamError::JSValue(err)and parked inpending.resultviaByteStream::appendfor later consumption. The rawJSValuewas stored in a plain heap field with no GC root, so the error object could be collected before the user called.text()/.bytes()/.json().to_buffered_valuethen read the freed cell and handed it toJSPromise::setSlot, whose write barrier dereferenced the swept structure ID (assert in debug, SIGILL in release).The same storage path is reachable from fetch and
Bun.serverequest bodies, which passStreamError::JSValuetoon_datathe same way.Fix
StreamResult::release()and everyto_js_weakconsumer already treatStreamError::JSValueas a protected handle and callunprotect()when done. Match that contract by callingprotect()inByteStream::appendat the two points where the error is parked inpending.result, so the value survives until one of those consumers unprotects it.Test
Added a regression test in the "s3 missing credentials" block that spawns a child with
BUN_JSC_slowPathAllocsBetweenGCs=5, creates a failing S3 stream, forces GC, and asserts.text()rejects withERR_S3_MISSING_CREDENTIALSand exit code 0. Before this change the child crashed (exit 132 in release, abort in debug).