Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
20 changes: 18 additions & 2 deletions src/runtime/api/bun/h2_frame_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5354,6 +5354,13 @@ 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 drain
// pending_engine_stream_closes at depth 0 and free the box under us.
// Deliberately not enter_stream_dispatch: no `&mut Stream` may live across the
// call — the streamStart handler's refused-stream path re-enters rst_stream,
// which forms its own `&mut` to this stream.
Comment thread
robobun marked this conversation as resolved.
Outdated
let _dispatch = self.enter_dispatch();
match callback.call(
&global,
ctx_value,
Expand All @@ -5363,12 +5370,21 @@ 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() {
// Skip if the callback closed the stream (free_resources queued its id and
// dropped its sctx root): re-rooting it would pin the dead JS stream until
// the session dies.
Comment thread
robobun marked this conversation as resolved.
Outdated
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
// entry exists — the armed dispatch guard deferred the only free path
// (rewrite_read's pending close drain) while the callback ran.
Comment thread
robobun marked this conversation as resolved.
Outdated
unsafe { (*stream).set_context(returned, &global) };
}
}
Expand Down
96 changes: 96 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,102 @@ 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) {
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