Skip to content
Open
13 changes: 8 additions & 5 deletions src/runtime/server/RequestContext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3279,17 +3279,16 @@ where
.set(readable_stream::Strong::init(stream, global_this));

this.byte_stream.set(Some(byte_stream_nn));
let mut response_buf = byte_stream.drain();
let buffer = response_buf.move_to_list();
let has_body_bytes = !buffer.is_empty();
this.response_buf_owned.set(buffer);

// we don't set size here because even if we have a hint
// uWebSockets won't let us partially write streaming content
this.blob.with_mut(|b| b.detach());

// if we've received metadata and part of the body, send everything we can and drain
if has_body_bytes {
if !byte_stream.buffer.get().is_empty() {
let mut buf = byte_stream.buffer.replace(Vec::new());
byte_stream.offset.set(0);
this.response_buf_owned.set(buf.move_to_list());
resp.run_corked_with_type(
Self::drain_response_buffer_and_metadata_corked,
this.as_ctx_ptr(),
Expand All @@ -3308,6 +3307,10 @@ where
this.as_ctx_ptr(),
);
}
// Wake the producer now that the buffered bytes are
// written; any output it emits synchronously reaches
// `write_chunk` after them.
Comment thread
robobun marked this conversation as resolved.
Outdated
byte_stream.flush_to_sink();
return;
}
}
Expand Down
24 changes: 20 additions & 4 deletions src/runtime/webcore/ByteStream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,17 @@ impl ByteStream {
return;
}
self.sink_paused.set(false);
self.flush_to_sink();
}

/// Write any buffered bytes to `self.sink`, then [`signal_drained`]. The
/// native-sink wiring paths call this (instead of [`drain`] + a manual
/// `sink.write`) so the buffered bytes reach the sink before the producer
/// is woken; a producer whose `on_ready` feeds synchronously (RewriterPipe)
/// would otherwise emit newer bytes to the already-installed sink ahead of
/// the ones the caller is still holding. Clears `self.sink` when the write
/// or the last-chunk flag ends the stream.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(crate) fn flush_to_sink(&self) {
let sink = *self.sink.get();
if sink.is_none() {
return;
Expand Down Expand Up @@ -672,12 +682,18 @@ impl ByteStream {
// deallocate the storage backing `&mut self` (dangling UAF).
}

/// JS-reader drain (native-source adapter's `handle.drain()` and the
/// `has_received_last_chunk` blob hand-off). Callers that have already
/// installed `self.sink` must use [`flush_to_sink`] instead so the bytes
/// are written before the producer is woken.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub(crate) fn drain(&self) -> Vec<u8> {
if !self.buffer.get().is_empty() {
self.signal_drained();
return Vec::<u8>::move_from_list(self.buffer.replace(Vec::new()));
debug_assert!(self.sink.get().is_none());
if self.buffer.get().is_empty() {
return Vec::<u8>::default();
}
Vec::<u8>::default()
let drained = Vec::<u8>::move_from_list(self.buffer.replace(Vec::new()));
self.signal_drained();
drained
}
Comment thread
claude[bot] marked this conversation as resolved.

/// Take a pre-attach `StreamResult::Err` stashed by [`Self::append`].
Expand Down
23 changes: 3 additions & 20 deletions src/runtime/webcore/ReadableStream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@
owner_cell: JSValue,
set_source: impl FnOnce(streams::SourceHandle),
) -> NativeWireResult {
use streams::{SourceHandle, Start, StreamError, StreamResult, Writable};
use streams::{SourceHandle, Start, StreamError, StreamResult};
use webcore::SinkHandle;

if let Some(byte_stream) = self.ptr.bytes() {
Expand All @@ -337,27 +337,10 @@
return NativeWireResult::EndedInline(Some(err));
}

let buffered = byte_stream.drain();
let has_last = byte_stream.has_received_last_chunk.get();
if !buffered.is_empty() {
let chunk = if has_last {
StreamResult::OwnedAndDone(buffered)
} else {
StreamResult::Owned(buffered)
};
match sink.write(&chunk) {
Writable::Backpressure(_) => byte_stream.sink_paused.set(true),
Writable::Done | Writable::Err(_) => {
byte_stream.sink.set(SinkHandle::None);
return NativeWireResult::EndedInline(None);
}
_ => {}
}
}
if has_last {
byte_stream.sink.set(SinkHandle::None);
byte_stream.flush_to_sink();
if byte_stream.sink.get().is_none() {
return NativeWireResult::EndedInline(None);
}

Check failure on line 343 in src/runtime/webcore/ReadableStream.rs

View check run for this annotation

Claude / Claude Code Review

flush_to_sink() ends the sink, then wire_native_sink returns EndedInline → callers double-end

`flush_to_sink()` now calls `sink.end(...)` on every path that clears `self.sink`, so when `wire_native_sink` returns `EndedInline(None)` the sink has *already* run its end-of-stream handling — but the `EndedInline` contract still says "the caller runs its own end-of-stream handling", and every caller does. For `FetchTasklet` (FetchTasklet.rs:670-677) that means `write_end_request` runs twice against the single `+1` taken at :636 — a `FetchTasklet::deref` over-release plus a duplicate chunked-EO
Comment thread
robobun marked this conversation as resolved.
Outdated
return NativeWireResult::Wired;
}
}
Expand Down
29 changes: 2 additions & 27 deletions src/runtime/webcore/s3/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1037,36 +1037,11 @@
return Ok(end_promise_value);
}

let buffered = byte_stream.drain();
let has_last = byte_stream.has_received_last_chunk.get();
if !buffered.is_empty() {
let chunk = if has_last {
crate::webcore::streams::StreamResult::OwnedAndDone(buffered)
} else {
crate::webcore::streams::StreamResult::Owned(buffered)
};
match sink.write(&chunk) {
crate::webcore::streams::Writable::Backpressure(_) => {
byte_stream.sink_paused.set(true);
}
crate::webcore::streams::Writable::Done
| crate::webcore::streams::Writable::Err(_) => {
byte_stream.sink.set(crate::webcore::SinkHandle::None);
sink.source.clear();
if !sink.ended {
let _ = sink.end(None);
}
ctx.handle_resolve_stream();
return Ok(end_promise_value);
}
_ => {}
}
}
if has_last {
byte_stream.sink.set(crate::webcore::SinkHandle::None);
byte_stream.flush_to_sink();
if byte_stream.sink.get().is_none() {
sink.source.clear();
if !sink.ended {
let _ = sink.end(None);

Check failure on line 1044 in src/runtime/webcore/s3/client.rs

View check run for this annotation

Claude / Claude Code Review

S3 upload_stream: flush_to_sink() → end_from_stream releases the pump ref, then handle_resolve_stream releases it again → UAF

The `flush_to_sink()` refactor changes what `sink.end()` means on this path: it now dispatches `SinkHandle::S3Upload.end()` → `NetworkSink::end_from_stream`, which — because `sink.source` was set to `SourceHandle::ByteStream` at :1026 — releases the stream-pump +1 via `S3UploadStreamWrapper::deref_(wrapper)` (streams.rs:2479, rc 2→1). This block then calls `ctx.handle_resolve_stream()`, releasing the *same* pump ref again (rc 1→0), freeing the wrapper while `task.callback_context` still points a
Comment thread
robobun marked this conversation as resolved.
Outdated
}
ctx.handle_resolve_stream();
}
Expand Down
130 changes: 130 additions & 0 deletions test/js/workerd/html-rewriter.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1741,6 +1741,136 @@ const payloads = [
},
];

// TextEncoderStream → HTMLRewriter → CompressionStream: the rewriter's input is
// driven by the native-sink bypass (encodeIntoSink), and its output ByteStream
// is consumed by a JS reader (pipeThrough), so its `drain()` is what wakes the
// rewriter once a batch of output bytes has been taken. With enough elements
// that one input chunk's output exceeds the rewriter's output high-water mark,
// the rewriter's `write` returns Backpressure; this must eventually resolve.
describe("output consumed via pipeThrough after a native-sink input transform", () => {
// ~50 elements → ~800 bytes of rewriter output per input chunk, well above
// the default output high-water mark (256).
const unit = "<p>abc</p>";
const chunk = Buffer.alloc(50 * unit.length, unit).toString();
const expected = Buffer.alloc(50 * 16, '<p x="1">abc</p>').toString();

function makeRewritten() {
let i = 0;
const body = new ReadableStream({
async pull(c) {
await Bun.sleep(0);
if (i++ === 0) c.enqueue(chunk);
else c.close();
},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});
return new HTMLRewriter()
.on("p", { element: e => e.setAttribute("x", "1") })
.transform(new Response(body.pipeThrough(new TextEncoderStream())));
}

it("completes when read with arrayBuffer()", async () => {
const compressed = makeRewritten().body.pipeThrough(new CompressionStream("gzip"));
const buf = await new Response(compressed).arrayBuffer();
const text = await new Response(new Blob([buf]).stream().pipeThrough(new DecompressionStream("gzip"))).text();
expect(text).toBe(expected);
});

it("completes when served over HTTP", async () => {
await using server = Bun.serve({
port: 0,
fetch: () => new Response(makeRewritten().body.pipeThrough(new CompressionStream("gzip"))),
});
const buf = await (await fetch(server.url)).arrayBuffer();
const text = await new Response(new Blob([buf]).stream().pipeThrough(new DecompressionStream("gzip"))).text();
expect(text).toBe(expected);
});

// CompressionStream is the only native-byte-transform that can sit downstream
// of the rewriter's ByteStream output; cover each format so a later per-codec
// regression shows up here.
it.each(["gzip", "deflate", "deflate-raw", "brotli", "zstd"])("completes for CompressionStream(%s)", async format => {
const compressed = makeRewritten().body.pipeThrough(new CompressionStream(format));
const text = await new Response(compressed.pipeThrough(new DecompressionStream(format))).text();
expect(text).toBe(expected);
});

// Two input chunks → the second chunk is what the Backpressure wake must
// re-pull from upstream (the single-chunk case only owes the end() call).
it("completes across multiple input chunks", async () => {
let i = 0;
const body = new ReadableStream({
async pull(c) {
await Bun.sleep(0);
if (i++ < 2) c.enqueue(chunk);
else c.close();
},
});
const rewritten = new HTMLRewriter()
.on("p", { element: e => e.setAttribute("x", "1") })
.transform(new Response(body.pipeThrough(new TextEncoderStream())));
const compressed = rewritten.body.pipeThrough(new CompressionStream("gzip"));
const text = await new Response(compressed.pipeThrough(new DecompressionStream("gzip"))).text();
expect(text).toBe(expected + expected);
});
});

// The same output-backpressure state, consumed by a native sink instead of a
// JS reader: the sink is installed on the output ByteStream while it still
// holds more than the rewriter's high-water mark in buffered bytes, and the
// rewriter's input is already fully consumed (input_ended). flush_to_sink()
// must write the buffered bytes to the sink before waking the rewriter (which
// then runs end_rewrite() synchronously).
describe("output consumed by a native sink after output backpressure", () => {
const prefix = "<p>" + Buffer.alloc(100, "A").toString() + "</p>";
const suffix = "<q>" + Buffer.alloc(30_000, "B").toString() + "</q>";
const html = prefix + "<x></x>" + suffix;

// Produce a rewriter output ByteStream in the "input_ended + buffer > hwm"
// state: .body materialises the ByteStream; resolving the handler gate lets
// resume_rewrite() push the 30 KB suffix into that buffer; drain_pending_input
// then returns early on output_backpressured(), leaving end_rewrite() owed.
async function makeParked() {
const gate = Promise.withResolvers();
const out = new HTMLRewriter()
.on("x", { element: () => gate.promise })
.transform(new Response(html));
const body = out.body;
gate.resolve();
await 0;
return { out, body };
}

it("chained HTMLRewriter receives the full output in order", async () => {
const { body } = await makeParked();
const chained = new HTMLRewriter().transform(new Response(body));
expect(await chained.text()).toBe(html);
});

it("Bun.serve returning the rewriter Response sends the full output", async () => {
await using server = Bun.serve({
port: 0,
async fetch() {
return (await makeParked()).out;
},
});
expect(await (await fetch(server.url)).text()).toBe(html);
});

it("fetch request body wired to the output ByteStream uploads the full output", async () => {
const received = Promise.withResolvers();
await using server = Bun.serve({
port: 0,
async fetch(req) {
received.resolve(await req.text());
return new Response("ok");
},
});
const { body } = await makeParked();
await fetch(server.url, { method: "POST", body });
expect(await received.promise).toBe(html);
});
});

payloads.forEach(type => {
type.test(`works with payload of type ${type.name}`, async () => {
let calls = 0;
Expand Down
Loading