webstreams: fix five crash/hang/data-loss bugs in the C++ streams implementation - #33849
Conversation
|
Updated 6:16 PM PT - Jul 9th, 2026
❌ @Jarred-Sumner, your commit 452e023 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33849That installs a local version of the PR into your bun-33849 --bun |
|
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 (2)
WalkthroughThis PR centralizes stream constructor structure resolution, fixes reentrancy in async iterators and bulk reader draining, updates text stream abrupt-completion handling, implements fixed-length transfer for resizable buffers, and adds regression tests. ChangesStream construction and structure resolution
Reentrancy and completion handling
Regression coverage
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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
🤖 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/web/streams/streams.test.js`:
- Around line 2348-2388: Mark the independent bunExe() subprocess test in
streams.test.js as test.concurrent so it can run in parallel with the other
subprocess cases, and keep the in-process iterator/assertion tests sequential.
Update the streams constructors test around the test("streams constructors
survive a foreign-realm (node:vm) newTarget", ...) block to use the concurrent
test wrapper only for the isolated subprocess case, without changing the shared
logic inside the script or the existing prototype assertions.
🪄 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: 4d4a0acf-aa00-4c4a-8d37-404b4528c63e
📒 Files selected for processing (16)
src/jsc/bindings/webcore/streams/JSByteLengthQueuingStrategy.cppsrc/jsc/bindings/webcore/streams/JSCountQueuingStrategy.cppsrc/jsc/bindings/webcore/streams/JSReadableStream.cppsrc/jsc/bindings/webcore/streams/JSReadableStreamAsyncIterator.cppsrc/jsc/bindings/webcore/streams/JSReadableStreamBYOBReader.cppsrc/jsc/bindings/webcore/streams/JSReadableStreamDefaultReader.cppsrc/jsc/bindings/webcore/streams/JSTextDecoderStream.cppsrc/jsc/bindings/webcore/streams/JSTextEncoderStream.cppsrc/jsc/bindings/webcore/streams/JSTransformStream.cppsrc/jsc/bindings/webcore/streams/JSWritableStream.cppsrc/jsc/bindings/webcore/streams/JSWritableStreamDefaultWriter.cppsrc/jsc/bindings/webcore/streams/StreamConstructor.hsrc/jsc/bindings/webcore/streams/WebStreamsMisc.cpptest/js/web/encoding/text-decoder-stream.test.tstest/js/web/encoding/text-encoder-stream.test.tstest/js/web/streams/streams.test.js
💤 Files with no reviewable changes (7)
- src/jsc/bindings/webcore/streams/JSByteLengthQueuingStrategy.cpp
- src/jsc/bindings/webcore/streams/JSWritableStreamDefaultWriter.cpp
- src/jsc/bindings/webcore/streams/JSCountQueuingStrategy.cpp
- src/jsc/bindings/webcore/streams/JSReadableStreamBYOBReader.cpp
- src/jsc/bindings/webcore/streams/JSReadableStream.cpp
- src/jsc/bindings/webcore/streams/JSTransformStream.cpp
- src/jsc/bindings/webcore/streams/JSWritableStream.cpp
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No correctness issues found beyond the stale-comment nit, but five separate fixes across JSC bindings — two memory-safety, three reentrancy/ordering — is well past what I'd auto-approve; the async-iterator promise-publication change and the new hwm - size > 0 gate in drainQueueEntriesInto in particular deserve human eyes.
Extended reasoning...
Overview
This PR touches 12 C++ files in src/jsc/bindings/webcore/streams/ plus 3 test files, delivering five independent bug fixes to the WebStreams C++ rewrite from #33193:
structureForNewTarget— replaces 10 copy-pasted static functions (each doing anuncheckedDowncast<JSDOMGlobalObject>on a foreign-realm global) with one shared template inStreamConstructor.hthatdynamicDowncasts and falls back to the constructor's own cached Structure.transferArrayBufferImpl— adds a resizable-buffer slow path that copies into a fixed-length block before detaching, sobyobRequest.view.buffer.resize()can no longer invalidate recorded byte lengths.drainQueueEntriesInto— reordersresetQueue()to run before the close/pull step, and adds an explicithwm - size > 0gate aroundCallPullIfNeededto preserve the readMany batching cadence now that the queue is empty when the decision runs.JSReadableStreamAsyncIterator— publishes the result promise intom_ongoingPromisebefore running user JS, guarded by a "don't rewind the chain tail" check, and adds aclearOngoingIfOurscleanup lambda for exception paths.decodeAndEnqueue/ newencodeAndEnqueue— widens the catch scope to covertransformStreamDefaultControllerEnqueue, and unifies the encoder's transform/flush into one helper.
Security risks
Fixes #1 and #2 are themselves security-relevant: #1 closes a heap type-confusion (invalid static_cast on a sibling global-object class → getDOMStructure reading/writing past allocation bounds), and #2 closes an OOB heap read reachable via buffer.resize() on a stream-internal buffer. Both are reachable from a few lines of untrusted user JS in release builds. The fixes look correct on inspection (dynamicDowncast + fallback; tryCreate(span) + detach), but this is exactly the code class where a subtle mistake becomes an exploitable primitive, so it warrants a second reviewer.
Level of scrutiny
High. This is hand-written C++ in JSC bindings — the CLAUDE.md "most-blocked category" — with exception-scope discipline (DECLARE_TOP_EXCEPTION_SCOPE, takeAbruptCompletion, RETURN_IF_EXCEPTION on new paths), GC-visited WriteBarrier fields (m_ongoingPromise publication timing), and reentrancy-sensitive state-machine ordering. Each of the five fixes individually is small and well-argued in the description, but together they change control flow at ~15 distinct sites.
Other factors
- The PR ships 9 targeted regression tests (subprocess-isolated for the crashers, in-process for the hangs) and the description states they were verified to fail on main. Test construction follows repo conventions (
bunExe/bunEnv, concurrent pipe drains,{stdout, exitCode, signalCode}object assertions). - Fix #3's new
hwm - size > 0gate is a behavioral approximation of the old "CallPullIfNeeded against pre-drain totalSize" — the description says the existingreadMany batches the pipelined pull's chunktest still passes byte-for-byte, but this is the kind of gate that could subtly change pull cadence for edge cases (e.g.,hwm === 0, negative desired size from a size algorithm) and deserves reviewer confirmation. - Fix #4's
clearOngoingIfOurslambda and the "only publish if current ongoing is not pending" guard are non-obvious invariants; the three added tests cover the reentrancy crash, the ordering property, and the queued-next()-resolution-order property, which is reassuring, but the interaction withonAsyncIteratorNextAfterOngoingSettled(which also callsrunAsyncIteratorNextSteps) is subtle enough that a human familiar with #33193 should confirm. - The one bug-hunter finding is a documentation nit (stale "runs no user JS" comment on
invokeDecode) — no correctness issues surfaced.
|
@robobun can you rebase and fix the conflicts |
|
✅ Rebased onto main; one conflict in The remaining red lane is |
Constructing any of the ten stream classes via Reflect.construct with a newTarget whose function realm is not a Zig global (e.g. a node:vm context) ran uncheckedDowncast<JSDOMGlobalObject> on that realm's global object — an invalid static_cast in release builds, reading and writing the DOM structure caches past the end of the smaller global allocation. Replace the ten copy-pasted structureForNewTarget helpers with one shared template in StreamConstructor.h that dynamicDowncasts the newTarget realm and falls back to the constructor's own realm's cached instance Structure when the realm has no DOM structure caches.
TransferArrayBuffer is specified as ArrayBufferCopyAndDetach(O, undefined, fixed-length), but transferArrayBufferImpl used ArrayBuffer::transferTo directly, which carries m_maxByteLength across the transfer — so the stream-internal buffer behind byte-queue entries and BYOB pull-into descriptors stayed resizable. User JS reaching that buffer through controller.byobRequest.view.buffer could then resize() it, invalidating every byte length the controller recorded at transfer time: enqueue after a shrink tripped the canCopyDataBlockBytes RELEASE_ASSERT (process abort), and the respond() remainder-clone path subspanned past the live length (out-of-bounds read surfaced to a subsequent read()). Mirror JSC's arrayBufferCopyAndDetach FixedLength slow path: copy a resizable source into a fixed-length block, then detach the original. Non-resizable buffers keep the zero-copy transfer.
drainQueueEntriesInto (behind reader.readMany() and the buffered consumers: text(), bytes(), Bun.readableStreamTo*, ...) removed every queue entry, ran the close-if-requested / pull-if-needed step, and only then called ResetQueue. When that pull fired (queue total below the high-water mark), a chunk it enqueued synchronously landed in the still-live queue and was wiped by the final reset, and a close() in the same pull saw a momentarily non-empty queue, never re-evaluated, and left the stream readable forever — consumers hung and the data vanished. Reset the queue first (the drain loop has already removed every entry), then run the close/pull step. The pull DECISION still runs against the pre-drain [[queueTotalSize]] — the established readMany cadence, where a full batch defers the next pull to the next wake — but anything a firing pull enqueues now survives into the next drain instead of being destroyed.
The async iterator only published m_ongoingPromise AFTER runAsyncIteratorNextSteps returned, but those steps run the user pull() synchronously (via the dequeue fast path's CallPullIfNeeded, or via the issued read). A next()/return() reentrantly called from inside that pull saw a stale non-pending ongoing promise, skipped the chaining path, and ran the return steps against an in-flight read — tripping ASSERT(reader->m_readRequests.isEmpty()) in debug builds and releasing the reader out from under the pending read in release builds. Publish the result promise as m_ongoingPromise before any user JS can run, but only when the current ongoing promise is not pending: when the steps run as an ongoing-settled reaction, the pending chained promise already serves as the guard and must remain the chain tail, or queued next() calls could resolve out of order. The host function no longer overwrites m_ongoingPromise afterwards, since a reentrant caller may have chained onto it while the steps ran.
36747eb to
ee6974d
Compare
The TextEncoderStream/TextDecoderStream transform and flush algorithms wrapped only the encode/decode invocation in the completion-record catch; the subsequent transformStreamDefaultControllerEnqueue was followed by a plain RETURN_IF_EXCEPTION. Both encode and decode can run user JS (ToString of the chunk; the patchable TextDecoder.prototype .decode), which can cancel the readable side mid-transform — the enqueue then throws TypeError, and that exception escaped synchronously out of writer.write()/writer.close() (a transform algorithm must never throw into ProcessWrite/ProcessClose). Worse, the in-flight write/close was already marked and never settled, so writer.abort()/close()/closed hung forever and pinned the stream graph. Extend the catch to cover the enqueue at all three sites (encoder transform, encoder flush, decoder transform+flush), converting the abrupt completion into a rejected promise that flows through onTSPerformTransformRejected / onTSSinkCloseFlushRejected into transformStreamError, matching defaultTransformAlgorithm.
ee6974d to
116fcfb
Compare
There was a problem hiding this comment.
No issues found and the stale comment I flagged earlier is now fixed — but given this touches memory-safety-critical JSC bindings (type confusion, OOB read, reentrancy ordering across five separate fixes), it deserves a human pass before landing.
Extended reasoning...
Overview
This PR fixes five distinct bugs in the C++ WebStreams implementation (src/jsc/bindings/webcore/streams/): a foreign-realm newTarget type-confusion across all 10 stream constructors (consolidated into a shared template in StreamConstructor.h), a resizable-ArrayBuffer transfer that permitted user resize() to invalidate byte-controller invariants (OOB heap read), a reset-before-pull reordering in drainQueueEntriesInto, an async-iterator m_ongoingPromise publish-before-user-JS reordering, and widened catch scopes in the TextEncoderStream/TextDecoderStream transform algorithms. Nine regression tests are added across three test files.
Security risks
Two of the five fixes are memory-safety issues reachable from user JS in release builds: an invalid static_cast of a sibling global-object type (heap type confusion via getDOMStructure), and an out-of-bounds heap read whose bytes were delivered to a subsequent read(). The fixes look correct — dynamicDowncast with a per-realm fallback, and copy-into-fixed-length matching JSC's own arrayBufferCopyAndDetach slow path — but these are exactly the paths where a second pair of eyes on the invariants is warranted.
Level of scrutiny
High. This is hand-written C++ operating on JSC cells with GC-visible state, WriteBarriers, and reentrancy windows where user JS can synchronously mutate the objects being iterated. The drainQueueEntriesInto change also introduces a new hwm - size > 0 gate around CallPullIfNeeded (previously the pull-decision was delegated entirely to the controller with the un-reset queue size); the PR description explains this preserves the readMany batching cadence and the existing readMany batches the pipelined pull's chunk test still passes, but it's a behavioral subtlety worth a maintainer confirming. The async-iterator m_ongoingPromise publish/clear dance (clearOngoingIfOurs) is similarly subtle.
Other factors
The bug-hunting system found no issues. My earlier inline comment (stale invokeDecode doc comment) was addressed in the rebase. CodeRabbit's only note was a trivial test.concurrent nit (resolved). The PR is well-tested and well-documented, but the combination of memory safety, GC interaction, and reentrancy ordering across five independent fixes puts it well outside the auto-approval threshold.
3 participants
What
Five self-contained fixes (one commit each) for bugs in the C++ WebStreams implementation introduced by #33193, found while auditing the rewrite. Two are memory-safety issues reachable from a few lines of user JS in release builds; three are hangs/data loss via reentrancy. Each commit ships regression tests verified to fail before the fix and pass after.
1. Foreign-realm
newTargettype-confuses the global object (all 10 constructors)Every stream constructor's subclass slow path did
uncheckedDowncast<JSDOMGlobalObject>(newTargetGlobalObject). Anode:vmcontext's global is a sibling class, so the downcast is an invalidstatic_castin release builds —getDOMStructurethen reads and writes structure caches past the end of the smaller allocation.The ten copy-pasted
structureForNewTargetstatics are replaced with one shared template inStreamConstructor.hthatdynamicDowncasts and falls back to the constructor's own realm's cached Structure (per-VM correct, unlike a process-global fallback).2.
TransferArrayBufferleft transferred buffers resizableThe spec defines TransferArrayBuffer as
ArrayBufferCopyAndDetach(O, undefined, fixed-length), buttransferArrayBufferImplusedArrayBuffer::transferTo, which carriesmaxByteLengthacross the transfer. User JS reaching the stream-internal buffer throughbyobRequest.view.buffercouldresize()it, invalidating every byte length the controller recorded:A second variant (
resize(2)+respond()with a remainder) made the remainder-clone pathsubspanpast the live length — an out-of-bounds heap read whose bytes were delivered to a subsequentread(). Fix mirrors JSC's ownarrayBufferCopyAndDetachFixedLength slow path: resizable sources are copied into a fixed-length block, then detached; non-resizable buffers keep the zero-copy transfer. (The WPT streams suite has no resizable-ArrayBuffer coverage, so tests are added.)3. Bulk drain ran the user
pull()beforeResetQueuedrainQueueEntriesInto— behindreader.readMany()and the buffered consumers (text(),bytes(),Bun.readableStreamTo*) — removed every queue entry, ran the close/pull step, and only then reset the queue. A chunk enqueued synchronously by that pull landed in the still-live queue and was wiped by the reset; aclose()in the same pull saw a momentarily non-empty queue and never re-evaluated:The queue is now reset before the close/pull step. The pull decision still runs against the pre-drain
[[queueTotalSize]], preserving the existing readMany batching cadence (thereadMany batches the pipelined pull's chunktest still passes byte-for-byte).4. Async iterator: reentrant
next()/return()from a synchronouspull()The iterator published
m_ongoingPromiseonly after running steps that invoke the userpull()synchronously. Areturn()called from inside that pull saw a stale non-pending ongoing promise, skipped the chaining path, and released the reader under an in-flight read (ASSERT(reader->m_readRequests.isEmpty())in debug):The result promise is now published before any user JS can run — but only when the current ongoing promise is not pending, so ongoing-settled reactions never rewind the chain tail (queued
next()calls still resolve in call order; tests cover both properties).5.
TextEncoderStream/TextDecoderStreamlet transform failures escape synchronouslyThe codec transform/flush algorithms wrapped only the encode/decode call in the completion-record catch. Both run user JS (
ToStringof the chunk; the patchableTextDecoder.prototype.decode), which can cancel the readable mid-transform — the subsequent enqueue's TypeError then escaped synchronously out ofwriter.write()/writer.close(), and the in-flight operation never settled:The catch now covers the enqueue at all sites (encoder transform+flush unified into one helper, mirroring the decoder), converting abrupt completions into rejected promises that flow through
transformStreamError— write rejects, abort settles, matching Node.Test notes
src/jsc/bindings/webcore/streams/to main and re-running.test/js/web/streams/+ WPT streams + encoding suites: 1,421 tests, no regressions (the one pre-existing failure,streams-leakabsolute-RSS floor under ASAN, fails identically on main with a negative RSS delta).no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/streams/streams.test.js