Fixes #7001 - #7861
Merged
Merged
Fixes #7001#7861
Conversation
Jarred-Sumner
commented
Dec 27, 2023
| stream: *ReadableStream, | ||
| globalThis: *JSC.JSGlobalObject, | ||
| ) ?JSC.WebCore.AnyBlob { | ||
| stream.reloadTag(globalThis); |
Collaborator
Author
There was a problem hiding this comment.
This is the main cause
We internally store the ReadableStream on the PendingValue, and the pointer becomes stale. It is a use-after-free where the ptr has been detached, but the existing PendingValue hasn't been updated to reflect that. To address this, we reload the tag.
Jarred-Sumner
commented
Dec 27, 2023
| JSC.markBinding(@src()); | ||
| this.value.unprotect(); | ||
| ReadableStream__cancel(this.value, globalThis); | ||
| this.value.unprotect(); |
Collaborator
Author
There was a problem hiding this comment.
Moving this after out of caution that the value gets GC'd immediately after its unprotected
Jarred-Sumner
commented
Dec 27, 2023
|
|
||
| export function readableStreamClose(stream) { | ||
| $assert($getByIdDirectPrivate(stream, "state") === $streamReadable); | ||
| $assert( |
Collaborator
Author
There was a problem hiding this comment.
This fixes the debug assertion failure in the direct readable stream tests.
Contributor
|
❌ @Jarred-Sumner 3 files with test failures on linux-x64-baseline:
|
Contributor
|
❌ @Jarred-Sumner 3 files with test failures on linux-x64:
|
Contributor
|
❌ @Jarred-Sumner 3 files with test failures on bun-darwin-aarch64:
|
Contributor
|
❌ @Jarred-Sumner 5 files with test failures on bun-darwin-x64-baseline:
|
|
|
||
| if (request.body.value == .Locked) { | ||
| if (request.body.value.Locked.readable) |stream| { | ||
| if (stream.isDisturbed(globalThis)) { |
Member
There was a problem hiding this comment.
should we also check isDisturbed in other places we call useAsAnyBlob? like:
bun/src/bun.js/webcore/response.zig
Line 1996 in 99aef8c
bun/src/bun.js/webcore/response.zig
Line 1779 in 99aef8c
| var ptr = this.#bunNativePtr; | ||
| $debug("ptr @ NativeReadable._read", ptr, this.__id); | ||
| if (ptr === 0) { | ||
| if (ptr === 0 || ptr === -1) { |
Member
There was a problem hiding this comment.
maybe adding some comments for what -1 and 0 mean (Locked/Detached) so we can remember next time
cirospaciari
self-requested a review
January 3, 2024 16:08
Contributor
|
❌ @Jarred-Sumner 5 files with test failures on bun-darwin-x64:
|
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 14, 2026
…42609) ### Problem - `Bun.serve()` sends a second `Response` around an already sent `ReadableStream` as a `200` with an empty body. The `error` handler does not run. - A native sink consumer (`Bun.serve()`, a `fetch()` upload, `Bun.write()`, `Bun.spawn()` stdin, S3, `HTMLRewriter`) pumps a JS-backed stream in `BunStreamSource.cpp`. At the end `rsisFinally` (`:853`) releases the reader and `directStreamOnClose` (`:662`) drops the lock: `locked === false`. A `type: "direct"` stream, so every async iterable body, is not disturbed either. `RequestContext.rs:3152` checks the lock alone. - Found by a comparison with node v26.3.0, not by a user report. ### Fix - `rsisBegin` and `readDirectStream` call the new `JSReadableStream::markConsumedAsBody()` once the pump holds the stream. The stream stays locked after the pump lets go. - Behaviour change: after such a consumer took a stream, `getReader()`, `tee()`, `pipeTo()` and `cancel()` fail with a `TypeError`. Body mixin methods (#42116) and natively wired sinks (#42477) already do this. Neither is released. - `pipeTo()`, async iteration and `Bun.readableStreamTo*()` do not reach these pumps and still unlock. - Verified: `web-stream-state.test.ts` (30 new tests fail on main), `body.test.ts` (12), `serve-reused-response.test.ts` (2). Self-reviewed: 3 concerns, 2 addressed, 1 out of scope (Notes). ### Background - A JSSink is a native sink (`HTTPResponseSink`, `FileSink`). `JSSink::assign_to_stream` pumps a stream into it. - `readStreamIntoSink` reads through a default reader. `readDirectStream` gives the sink to the `pull()` of a direct stream. - `m_consumedAsBody` (#42116) is a bit that `isReadableStreamLocked()` includes and nothing clears. The fetch spec never releases the reader of a body. <details><summary>Notes</summary> **The rule.** A consumer that takes a stream as a body keeps it locked: the body mixin methods (#42116), `to_any_blob` lifts (#42516), natively wired `ByteStream`/`FileReader` sinks (#42477), and now the two pumps. A reader at the stream level releases its lock as the Streams spec says. Checked on this branch: `pipeTo()`, `pipeThrough()`, `for await`, `getReader()` + `releaseLock()`, `Bun.readableStreamToText()`, `Bun.readableStreamToArrayBuffer()`, `stream.text()`, `stream.bytes()`, `stream.json()`, `stream.blob()` all end with `locked === false`. **Scope of the pumps.** `readStreamIntoSink` and `readDirectStream` have one caller, `assignToStream`, which only `JSSinkController__assignToStream` calls (Rust `JSSink::assign_to_stream`: `HTTPResponseSink` and its TLS and HTTP/3 siblings, `FetchRequestBodySink`, `NetworkSink`, `FileSink`, `RewriterPipe`). **State on main.** The lock state after the hand-off depends on how the pump ended. A clean end and a sink that closes early (client gone, aborted upload, child exited) go through `rsisFinish` and release the reader one microtask after the stream closes. A producer error goes through `rsisAbrupt`, which orphans the reader, so that stream stays locked. A direct stream is never disturbed. **Repro (the user-visible part).** ```js const stream = new ReadableStream({ start(c) { c.enqueue(new TextEncoder().encode("payload")); c.close(); } }); const responses = [new Response(stream), new Response(stream)]; await using server = Bun.serve({ port: 0, fetch: () => responses.shift(), error: e => new Response(e.code, { status: 500 }) }); for (let i = 0; i < 2; i++) { const r = await fetch(server.url); console.log(r.status, JSON.stringify(await r.text())); } // main: 200 "payload", 200 "" // branch: 200 "payload", 500 "ERR_STREAM_CANNOT_PIPE" (Stream already used, please create a new one) ``` For a direct stream `new Response(stream)` per request shows the same on main: `200 "hello"`, `200 ""`, `500`. On this branch the second `new Response(stream)` throws `Body object should not be disturbed or locked`. **Other observable changes.** `request.bodyUsed` after `fetch(request)` with an async iterable body is now `true` (was `false`, the stream was not disturbed). `Bun.readableStreamToText(stream)` on a stream a sink holds rejects with "ReadableStream has already been used" in place of "ReadableStream is locked" (same `ERR_INVALID_STATE`). **Bare streams.** `fetch(url, { body: stream })`, `Bun.write(path, stream)` and `Bun.spawn({ stdin: stream })` keep a bare stream locked too. Natively wired sources already do this for the same calls (`ReadableStream__lockNative` never unlocks, see "errors a Bun.file() stream whose file does not open" from #42477). **node v26.3.0.** `fetch(url, { method: "POST", body: jsStream, duplex: "half" })`: afterwards `locked === true` and `getReader()` throws. The same after an upload that an `AbortSignal` stopped. The other five consumers are Bun APIs with no Node counterpart. **The mark comes after the reader acquisition.** A stream that another reader already holds is not marked. `Bun.serve()`, `fetch()`, `Bun.write()` and `HTMLRewriter` reject such a stream before the pump. `Bun.spawn()` stdin checks only `is_disturbed` (`stdio.rs:388`, `:536`), so a locked stream reaches the pump there. **Out of scope (the concern not addressed).** `rsisFinally` calls `clearStreamControllerSlots` also when the pump never got the lock. `Bun.spawn({ stdin: stream })` with a stream the caller holds through `getReader()` reaches that: the caller's `reader.read()` then never settles. This is the same on main and on this branch. #41532 rejects a locked stdin stream before the pump. **Not covered here.** - A handler that drains or partly reads a stream with its own reader, releases it, and then returns it in a `Response` still gets a `200` with the empty or remaining body. #36110 covers that gate. - Handing the same Node `Readable` or generator object to a second consumer makes a new stream each time. **Changed tests.** Three tests in `bun-write.test.js` (from #42114) read the terminal state through `stream.getReader().closed`. One test in `streams.test.js` (from #33781) polled `ts.readable.locked` to wait for the sink teardown. Both observed the unlock as a means, not as the subject. They use `finished()` from `node:stream/promises` now. The `streams.test.js` test still proves the teardown ran: `desiredSize` reads `null` only after the controller slot is cleared, and reads `0` for a stream that is only closed. **Fail before.** With `src/` from main and `bun bd`: `web-stream-state.test.ts` 30 of 46 fail, `body.test.ts` 12 of 778 fail, `serve-reused-response.test.ts` 2 of 9 fail, and the three `bun-write.test.js` tests fail on the new `locked` assertion. With this branch all pass. On main only the `Bun.serve()` row of "the stdout of a running child" fails: the other four consumers wire a pipe-backed `FileReader` natively. **Built-in JS.** I searched `src/js` for code that touches a stream after a native sink took it. The only `stream.cancel()` on a web stream is `ReadableFromWeb._destroy`, which #42573 handles for the body mixin case. **Earlier reports of the same class.** #7001 (fixed in #7861) and #6860. **Suites run on the debug build.** `test/js/web/streams/{streams,streams-leak,readable-stream-blob-consumed,readable-stream-terminal-barrier-release,transform-stream-leak}`, `test/js/third_party/wpt-streams`, `test/js/web/fetch/{body,body-stream,body-stream-excess,body-clone,body-async-iterator,body-mixin-errors,blob-write,fetch,fetch.stream,fetch-backpressure,fetch-abort-stream-body,fetch-stream-cancel-leak,fetch-redirect,response}`, `test/js/web/request/request`, `test/js/bun/http/{serve,bun-server,serve-reused-response,serve-direct-readable-stream,serve-body-leak,serve-pending-promise-abort-leak,async-iterator-stream,serve-async-stream-client-abort,serve-error-handler-stream,serve-response-stream-sink-leak,serve-stream-body-error,serve-stream-reject-flush-leak}`, `test/js/bun/spawn/{spawn,spawn-stdin-readable-stream}`, `test/js/bun/io/bun-write`, `test/js/workerd/{html-rewriter,html-rewriter-leak}`, `test/js/bun/s3/{s3-stream-cancel-leak,s3-stream-error-gc,s3-upload-stream-gc,s3-write-to-file-sync-close,s3-connection-close}`, `test/js/node/stream/{node-stream,web-stream-state}`, `test/js/node/async_hooks/AsyncLocalStorage`, `test/regression/issue/07001`. The failures that remain also fail with `src/` from main in this container: IPv6, tests that need a non-root user, external hosts, and 5 s timeouts of the debug build. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · 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, test/js/bun/io/bun-write.test.js <!-- robobun:evidence:end -->
usrbinkat
pushed a commit
to usrbinkat/bun
that referenced
this pull request
Sep 15, 2026
…ven-sh#42609) ### Problem - `Bun.serve()` sends a second `Response` around an already sent `ReadableStream` as a `200` with an empty body. The `error` handler does not run. - A native sink consumer (`Bun.serve()`, a `fetch()` upload, `Bun.write()`, `Bun.spawn()` stdin, S3, `HTMLRewriter`) pumps a JS-backed stream in `BunStreamSource.cpp`. At the end `rsisFinally` (`:853`) releases the reader and `directStreamOnClose` (`:662`) drops the lock: `locked === false`. A `type: "direct"` stream, so every async iterable body, is not disturbed either. `RequestContext.rs:3152` checks the lock alone. - Found by a comparison with node v26.3.0, not by a user report. ### Fix - `rsisBegin` and `readDirectStream` call the new `JSReadableStream::markConsumedAsBody()` once the pump holds the stream. The stream stays locked after the pump lets go. - Behaviour change: after such a consumer took a stream, `getReader()`, `tee()`, `pipeTo()` and `cancel()` fail with a `TypeError`. Body mixin methods (oven-sh#42116) and natively wired sinks (oven-sh#42477) already do this. Neither is released. - `pipeTo()`, async iteration and `Bun.readableStreamTo*()` do not reach these pumps and still unlock. - Verified: `web-stream-state.test.ts` (30 new tests fail on main), `body.test.ts` (12), `serve-reused-response.test.ts` (2). Self-reviewed: 3 concerns, 2 addressed, 1 out of scope (Notes). ### Background - A JSSink is a native sink (`HTTPResponseSink`, `FileSink`). `JSSink::assign_to_stream` pumps a stream into it. - `readStreamIntoSink` reads through a default reader. `readDirectStream` gives the sink to the `pull()` of a direct stream. - `m_consumedAsBody` (oven-sh#42116) is a bit that `isReadableStreamLocked()` includes and nothing clears. The fetch spec never releases the reader of a body. <details><summary>Notes</summary> **The rule.** A consumer that takes a stream as a body keeps it locked: the body mixin methods (oven-sh#42116), `to_any_blob` lifts (oven-sh#42516), natively wired `ByteStream`/`FileReader` sinks (oven-sh#42477), and now the two pumps. A reader at the stream level releases its lock as the Streams spec says. Checked on this branch: `pipeTo()`, `pipeThrough()`, `for await`, `getReader()` + `releaseLock()`, `Bun.readableStreamToText()`, `Bun.readableStreamToArrayBuffer()`, `stream.text()`, `stream.bytes()`, `stream.json()`, `stream.blob()` all end with `locked === false`. **Scope of the pumps.** `readStreamIntoSink` and `readDirectStream` have one caller, `assignToStream`, which only `JSSinkController__assignToStream` calls (Rust `JSSink::assign_to_stream`: `HTTPResponseSink` and its TLS and HTTP/3 siblings, `FetchRequestBodySink`, `NetworkSink`, `FileSink`, `RewriterPipe`). **State on main.** The lock state after the hand-off depends on how the pump ended. A clean end and a sink that closes early (client gone, aborted upload, child exited) go through `rsisFinish` and release the reader one microtask after the stream closes. A producer error goes through `rsisAbrupt`, which orphans the reader, so that stream stays locked. A direct stream is never disturbed. **Repro (the user-visible part).** ```js const stream = new ReadableStream({ start(c) { c.enqueue(new TextEncoder().encode("payload")); c.close(); } }); const responses = [new Response(stream), new Response(stream)]; await using server = Bun.serve({ port: 0, fetch: () => responses.shift(), error: e => new Response(e.code, { status: 500 }) }); for (let i = 0; i < 2; i++) { const r = await fetch(server.url); console.log(r.status, JSON.stringify(await r.text())); } // main: 200 "payload", 200 "" // branch: 200 "payload", 500 "ERR_STREAM_CANNOT_PIPE" (Stream already used, please create a new one) ``` For a direct stream `new Response(stream)` per request shows the same on main: `200 "hello"`, `200 ""`, `500`. On this branch the second `new Response(stream)` throws `Body object should not be disturbed or locked`. **Other observable changes.** `request.bodyUsed` after `fetch(request)` with an async iterable body is now `true` (was `false`, the stream was not disturbed). `Bun.readableStreamToText(stream)` on a stream a sink holds rejects with "ReadableStream has already been used" in place of "ReadableStream is locked" (same `ERR_INVALID_STATE`). **Bare streams.** `fetch(url, { body: stream })`, `Bun.write(path, stream)` and `Bun.spawn({ stdin: stream })` keep a bare stream locked too. Natively wired sources already do this for the same calls (`ReadableStream__lockNative` never unlocks, see "errors a Bun.file() stream whose file does not open" from oven-sh#42477). **node v26.3.0.** `fetch(url, { method: "POST", body: jsStream, duplex: "half" })`: afterwards `locked === true` and `getReader()` throws. The same after an upload that an `AbortSignal` stopped. The other five consumers are Bun APIs with no Node counterpart. **The mark comes after the reader acquisition.** A stream that another reader already holds is not marked. `Bun.serve()`, `fetch()`, `Bun.write()` and `HTMLRewriter` reject such a stream before the pump. `Bun.spawn()` stdin checks only `is_disturbed` (`stdio.rs:388`, `:536`), so a locked stream reaches the pump there. **Out of scope (the concern not addressed).** `rsisFinally` calls `clearStreamControllerSlots` also when the pump never got the lock. `Bun.spawn({ stdin: stream })` with a stream the caller holds through `getReader()` reaches that: the caller's `reader.read()` then never settles. This is the same on main and on this branch. oven-sh#41532 rejects a locked stdin stream before the pump. **Not covered here.** - A handler that drains or partly reads a stream with its own reader, releases it, and then returns it in a `Response` still gets a `200` with the empty or remaining body. oven-sh#36110 covers that gate. - Handing the same Node `Readable` or generator object to a second consumer makes a new stream each time. **Changed tests.** Three tests in `bun-write.test.js` (from oven-sh#42114) read the terminal state through `stream.getReader().closed`. One test in `streams.test.js` (from oven-sh#33781) polled `ts.readable.locked` to wait for the sink teardown. Both observed the unlock as a means, not as the subject. They use `finished()` from `node:stream/promises` now. The `streams.test.js` test still proves the teardown ran: `desiredSize` reads `null` only after the controller slot is cleared, and reads `0` for a stream that is only closed. **Fail before.** With `src/` from main and `bun bd`: `web-stream-state.test.ts` 30 of 46 fail, `body.test.ts` 12 of 778 fail, `serve-reused-response.test.ts` 2 of 9 fail, and the three `bun-write.test.js` tests fail on the new `locked` assertion. With this branch all pass. On main only the `Bun.serve()` row of "the stdout of a running child" fails: the other four consumers wire a pipe-backed `FileReader` natively. **Built-in JS.** I searched `src/js` for code that touches a stream after a native sink took it. The only `stream.cancel()` on a web stream is `ReadableFromWeb._destroy`, which oven-sh#42573 handles for the body mixin case. **Earlier reports of the same class.** oven-sh#7001 (fixed in oven-sh#7861) and oven-sh#6860. **Suites run on the debug build.** `test/js/web/streams/{streams,streams-leak,readable-stream-blob-consumed,readable-stream-terminal-barrier-release,transform-stream-leak}`, `test/js/third_party/wpt-streams`, `test/js/web/fetch/{body,body-stream,body-stream-excess,body-clone,body-async-iterator,body-mixin-errors,blob-write,fetch,fetch.stream,fetch-backpressure,fetch-abort-stream-body,fetch-stream-cancel-leak,fetch-redirect,response}`, `test/js/web/request/request`, `test/js/bun/http/{serve,bun-server,serve-reused-response,serve-direct-readable-stream,serve-body-leak,serve-pending-promise-abort-leak,async-iterator-stream,serve-async-stream-client-abort,serve-error-handler-stream,serve-response-stream-sink-leak,serve-stream-body-error,serve-stream-reject-flush-leak}`, `test/js/bun/spawn/{spawn,spawn-stdin-readable-stream}`, `test/js/bun/io/bun-write`, `test/js/workerd/{html-rewriter,html-rewriter-leak}`, `test/js/bun/s3/{s3-stream-cancel-leak,s3-stream-error-gc,s3-upload-stream-gc,s3-write-to-file-sync-close,s3-connection-close}`, `test/js/node/stream/{node-stream,web-stream-state}`, `test/js/node/async_hooks/AsyncLocalStorage`, `test/regression/issue/07001`. The failures that remain also fail with `src/` from main in this container: IPv6, tests that need a non-root user, external hosts, and 5 s timeouts of the debug build. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · 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, test/js/bun/io/bun-write.test.js <!-- robobun:evidence:end -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This fixes the immediate cause of #7001. It doesn't fix it in every case (such as streams)
ReadableStream.prototype.isLockedreturns true if our hidden native pointer value is set to-1.ReadableStreamis locked, including for Blob valuesHow did you verify your code works?
There are some tests
@cirospaciari please review this closely because it's very easy to subtly break things with this code and I'm worried something will break due to this change