Author

James M Snell <jsnell@cloudflare.com> 2026-03-05 11:40:34 -0800

Parents

28394f6

Tree

452811f

Fix use-after-free in drainingRead caused by premature Consumer destruction

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.