Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR fixes multiple safety issues (UAF, unbounded reads, closed/canceled state handling) in the draining read path for standard streams, places the new pumpTo implementation behind an autogate (ENABLE_DRAINING_READ_ON_STANDARD_STREAMS), and fixes a real bug where @all-autogates test variants were ineffective when TestFixture called initAutogate({}) early. Excellent test coverage with regression tests for each bug class.
Issues
-
(Medium) Duplicated code between ValueQueue and ByteQueue
drainingRead()The post-pump-loop logic (lines 253-303 for ValueQueue, lines 723-770 for ByteQueue) is nearly identical across the two implementations:
!impl.state.isActive()check,impl.queue == kj::nonecheck,isClosingfinalization, and the normal data-return path. This is a maintenance hazard — if a bug is found in one path, it must be fixed in both. Not a blocker but worth noting as a follow-up to extract the shared post-pump logic into a common helper. -
(Low)
benchChunkCounterStaticis a file-scope mutable globalIn
bench-pumpto.c++,benchChunkCounterStatic(line ~133 of the new file) is a file-scopedstatic size_tthat gets reset increateConfiguredStream()and mutated via raw pointer in pull callbacks. Google Benchmark can run iterations in parallel in some configurations. This is unlikely to cause issues with the current--benchmark_min_timedefaults, but a per-invocationkj::heap<size_t>()or a member of a benchmark fixture would be more robust. Not blocking since the benchmark istags = ["manual"].
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
I have read the CLA Document and I hereby sign the CLA |
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: jasnell <jasnell@users.noreply.github.com>
jasnell
force-pushed
the
jasnell/fixup-drainingread
branch
from
fa9c248 to
e41bcda
Compare
This reverts commit c9309c6.
…uction The pumpToImpl coroutine uses DrainingReader which calls `consumer->drainingRead()``. That call may trigger `onConsumerWantsData` -> `forcePull` -> `pull` callback -> synchronous close/error -> `deferTransitionTo<Closed>`. Previously, both `ValueReadable::drainingRead()` and the caller (`ReadableStreamJsController::drainingRead`) each called `beginOperation()`/`endOperation()` independently. The inner `endOperation()` in ValueReadable/ByteReadable fired the deferred state transition before the caller's `wrapDrainingRead` could set up `.then()` callbacks on the returned promise. Since those callbacks capture `this` (the Consumer), the transition destroyed the Consumer out from under them triggering the UAF. The fix removes `beginOperation()`/`endOperation()` from `ValueReadable::drainingRead()` and `ByteReadable::drainingRead()`, and moves the single `beginOperation()` call to before `consumer->drainingRead()` at each call site in `ReadableStreamJsController::drainingRead`. The matching `endOperation()` remains in the `.then()`/`.catch()` callbacks of `wrapDrainingRead`, ensuring the deferred state change only fires after the Consumer's this-capturing callbacks have already run. `js.tryCatch` wraps each call site for exception safety. The tests do not perfectly catch the UAF even with ASAN because the conditions are extremely timing-sensitive and I haven't yet found a way to reproduce the exact timing reliably in workerd.
jasnell
force-pushed
the
jasnell/fixup-drainingread
branch
from
e41bcda to
cc5c650
Compare
|
|
||
| # Benchmark for PumpToReader (ReadableStream::pumpTo path in standard.c++). | ||
| # Run before and after DrainingReader adoption to measure improvement. | ||
| # bazel run --config=opt //src/workerd/tests:bench-pumpto |
2 participants
Fixes up multiple issues with the draining read implementation, and more importantly, places the change behind an autogate.
Along the way,
Fixes a bug in the@all-autogatesvariant that caused it to be ineffective for any test usingTestFixture