Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/runtime/api/bun/h2_frame_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5354,6 +5354,11 @@ impl H2FrameParser {
if global.has_exception() {
return Some(stream);
}
// The callback runs arbitrary JS while `stream` is held (here and by every
// caller): arm the dispatch guard so a reentrant read() cannot free the box at
// depth 0. Bare guard, not enter_stream_dispatch — rst_stream reached from the
// callback takes its own `&mut` to this stream, so ours must wait for the return.
Comment thread
robobun marked this conversation as resolved.
let _dispatch = self.enter_dispatch();
match callback.call(
&global,
ctx_value,
Expand All @@ -5363,13 +5368,19 @@ impl H2FrameParser {
Ok(returned) => {
// streamStart returns the JS stream it created; storing it here saves the
// setStreamContext host call the JS layer used to make per stream.
if returned.is_object() {
// Skipped when the callback closed the stream: free_resources dropped its
// sctx root, and re-rooting would pin the dead JS stream until session death.
Comment thread
robobun marked this conversation as resolved.
if returned.is_object()
&& !self
.pending_engine_stream_closes
.get()
.contains(&stream_identifier)
{
self.sctx.with_mut(|m| {
m.insert(stream_identifier, StrongOptional::create(returned, &global));
});
// SAFETY: stream is *mut Stream from self.streams; valid while the map
// entry exists
unsafe { (*stream).set_context(returned, &global) };
self.enter_stream_dispatch(stream)
.set_context(returned, &global);
}
}
}
Expand Down
100 changes: 100 additions & 0 deletions test/js/node/http2/node-http2-streams-rehash.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,106 @@ test(
10_000 * ASAN_MULTIPLIER,
);

// handle_received_stream_id invoked the JS streamStart callback without arming the
// dispatch guard while holding the just-created *Stream. JS reached from inside that
// callback (here: EventEmitter.prototype.on, called by the Http2Stream constructor)
// could close the stream and then re-enter parser.read() at dispatch depth 0, where
// the deferred-close drain frees the Stream box; the native caller then wrote the
// stream context through the dangling pointer (ASAN: heap-use-after-free in
// Stream::set_context). The parser is driven directly because the hook must observe
// the window inside the native callback, before setStreamContext runs.
test(
"closing the new stream and re-entering read() inside the streamStart callback does not UAF",
async () => {
const script = /* js */ `
const http2 = require("node:http2");
const { Duplex } = require("node:stream");
const EE = require("node:events");

const socket = new Duplex({
write(chunk, enc, cb) {
cb();
},
read() {},
});

const session = http2.performServerHandshake(socket);
const parser = session[Symbol.for("::bunhttp2native::")];

const origOn = EE.prototype.on;
let hooked = false;
let armed = false;
EE.prototype.on = function (ev, fn) {
// Http2Stream's constructor calls this.on("pause", ...) from inside the
// native onStreamStart callback for the stream getNextStream() allocates.
if (armed && ev === "pause") {
armed = false;
hooked = true;
parser.rstStream(2, 8 /* NGHTTP2_CANCEL */); // queue the new stream's deferred close
parser.read(Buffer.from("PRI * HTTP/2.0\\r\\n\\r\\nSM\\r\\n\\r\\n")); // depth-0 read used to drain it
}
return origOn.call(this, ev, fn);
};

armed = true;
const id = parser.getNextStream();
EE.prototype.on = origOn;
if (!hooked) {
console.error("hook was never invoked");
process.exit(1);
}
if (id !== 2) {
console.error("unexpected stream id: " + id);
process.exit(1);
}
// The close must have been deferred, not drained inside the callback: the native
// entry is still alive (pre-fix this throws "Invalid stream id" on every build
// tier because the drain freed it), and no context may have been installed for
// the closed stream (a guard-only fix would return the Http2Stream here).
let ctx;
try {
ctx = parser.getStreamContext(2);
} catch (e) {
console.error("getStreamContext threw: " + e.message);
process.exit(1);
}
if (ctx !== undefined) {
console.error("context installed for closed stream");
process.exit(1);
}
// One depth-0 read runs the deferred drain; the entry must actually go away.
parser.read(Buffer.alloc(0));
let drained = false;
try {
parser.getStreamContext(2);
} catch (e) {
if (e.message !== "Invalid stream id") {
console.error("unexpected getStreamContext error: " + e.message);
process.exit(1);
}
drained = true;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if (!drained) {
console.error("deferred close never drained");
process.exit(1);
}
session.destroy();
console.log("OK");
process.exit(0);
`;

await using proc = Bun.spawn({
cmd: [bunExe(), "-e", script],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout: stdout.trim(), exitCode, stderr }).toMatchObject({ stdout: "OK", exitCode: 0 });
},
10_000 * ASAN_MULTIPLIER,
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

test("http2 client write callback that opens new streams during flushQueue does not UAF", async () => {
await using proc = Bun.spawn({
cmd: [bunExe(), path.join(import.meta.dir, "node-http2-flush-rehash.fixture.js")],
Expand Down