Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
55 changes: 15 additions & 40 deletions src/runtime/api/bun/h2_frame_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1119,7 +1119,6 @@ thread_local! {
// pool allocation itself.
static POOL: RefCell<Option<Box<ManuallyDrop<H2FrameParserHiveAllocator>>>> =
const { RefCell::new(None) };
static SHARED_REQUEST_BUFFER: RefCell<Box<[u8; 16384]>> = RefCell::new(Box::new([0u8; 16384]));
}
Comment thread
robobun marked this conversation as resolved.

/// One wire-order piece of a multi-frame send_data batch (see BATCH_SEGMENTS).
Expand Down Expand Up @@ -1738,19 +1737,7 @@ impl Stream {
};
let _ = data_header.write(&mut writer);
if padding != 0 {
break 'brk SHARED_REQUEST_BUFFER.with_borrow_mut(|buffer| {
// SAFETY: src/dst may overlap — use ptr::copy (memmove)
unsafe {
core::ptr::copy(
able_to_send.as_ptr(),
buffer.as_mut_ptr().add(1),
able_to_send.len(),
);
}
buffer[0] = padding;
buffer[1 + able_to_send.len()..payload_size].fill(0);
writer.write_all(&buffer[0..payload_size]).is_ok()
});
break 'brk writer.write_padded(&able_to_send, padding).is_ok();
} else {
break 'brk writer.write_all(&able_to_send).is_ok();
}
Expand Down Expand Up @@ -1802,19 +1789,7 @@ impl Stream {
};
let _ = data_header.write(&mut writer);
if padding != 0 {
break 'brk SHARED_REQUEST_BUFFER.with_borrow_mut(|buffer| {
// SAFETY: src/dst may overlap — ptr::copy is memmove; dst capacity covers payload_size
unsafe {
core::ptr::copy(
frame_slice.as_ptr(),
buffer.as_mut_ptr().add(1),
frame_slice.len(),
);
}
buffer[0] = padding;
buffer[1 + frame_slice.len()..payload_size].fill(0);
writer.write_all(&buffer[0..payload_size]).is_ok()
});
break 'brk writer.write_padded(frame_slice, padding).is_ok();
} else {
break 'brk writer.write_all(frame_slice).is_ok();
}
Expand Down Expand Up @@ -6165,6 +6140,18 @@ impl bun_io::Write for DirectWriterStruct {
}
}

impl DirectWriterStruct {
/// The payload of a PADDED DATA frame (RFC 9113 6.1); the caller wrote the frame header.
fn write_padded(&mut self, data: &[u8], padding: u8) -> bun_io::Result<()> {
// Owned per call: `write()` can re-enter this path through a JS transport mid-frame.
let mut payload = Vec::with_capacity(1 + data.len() + padding as usize);
payload.push(padding);
payload.extend_from_slice(data);
payload.resize(payload.len() + padding as usize, 0);
self.write_all(&payload)
}
Comment thread
claude[bot] marked this conversation as resolved.
}

// ──────────────────────────────────────────────────────────────────────────
// H2FrameParser impl — JS host fns (part 1)
// ──────────────────────────────────────────────────────────────────────────
Expand Down Expand Up @@ -7294,19 +7281,7 @@ impl H2FrameParser {
let mut writer = self.to_writer();
let _ = data_header.write(&mut writer);
if padding != 0 {
SHARED_REQUEST_BUFFER.with_borrow_mut(|buffer| {
// SAFETY: src/dst may overlap — ptr::copy is memmove; dst capacity covers payload_size
unsafe {
core::ptr::copy(
slice.as_ptr(),
buffer.as_mut_ptr().add(1),
slice.len(),
);
}
buffer[0] = padding;
buffer[1 + slice.len()..payload_size].fill(0);
let _ = writer.write_all(&buffer[0..payload_size]);
});
let _ = writer.write_padded(slice, padding);
} else {
let _ = writer.write_all(slice);
}
Expand Down
72 changes: 72 additions & 0 deletions test/js/node/http2/node-http2.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1987,6 +1987,78 @@ it("http2 session.goaway() opaqueData survives re-entrant buffer detach over a J
expect(exitCode).toBe(0);
}, 15_000);

it("http2 padded DATA write survives a re-entrant stream write from a JS Duplex _write", async () => {
// With padding enabled, a single-frame DATA write that crosses the 16 KiB cork boundary
// flushes the cork into the JS transport mid-frame (onWrite → Duplex _write). A transport
// that writes to another padded stream from inside its _write re-enters the same padded
// send path while the outer frame is still being assembled. That must neither abort the
// process nor clobber the outer frame's bytes: every payload byte of all three writes has
// to reach the transport (same totals as node).
const fixture = `
const http2 = require("node:http2");
const { Duplex } = require("node:stream");
// fill: 13000 -> 9+13256 corked; outer: 12000 -> 9+12256, crosses the cork boundary;
// side: 3000 -> 9+3256, written from inside the transport _write during that flush.
const EXPECTED_WIRE = 9 + 13256 + 9 + 12256 + 9 + 3256;
let side, armed = false, reentered = false, total = 0, onComplete;
const counts = { 0x41: 0, 0x42: 0, 0x43: 0 };
const fail = what => err => {
console.error(what, err);
process.exit(1);
};
const duplex = new Duplex({
read() {},
write(chunk, enc, cb) {
if (armed) {
for (const b of chunk) if (b in counts) counts[b]++;
total += chunk.length;
// The first chunk carrying outer's payload is the mid-frame cork flush.
if (!reentered && chunk.includes(0x41)) {
reentered = true;
side.write(Buffer.alloc(3000, 0x42));
}
if (total >= EXPECTED_WIRE && onComplete) onComplete();
}
cb();
},
});
const session = http2.connect("http://127.0.0.1:1", {
createConnection: () => duplex,
paddingStrategy: http2.constants.PADDING_STRATEGY_MAX,
});
session.on("error", fail("session error"));
await new Promise(r => session.once("connect", r));
const frame = (type, flags) => Buffer.from([0, 0, 0, type, flags, 0, 0, 0, 0]);
// server preface by hand: empty SETTINGS, then the ACK for the client's SETTINGS
duplex.push(Buffer.concat([frame(4, 0), frame(4, 1)]));
await new Promise(r => setImmediate(r));
side = session.request({ ":method": "POST", ":path": "/side" }, { endStream: false });
const outer = session.request({ ":method": "POST", ":path": "/outer" }, { endStream: false });
const fill = session.request({ ":method": "POST", ":path": "/fill" }, { endStream: false });
for (const s of [side, outer, fill]) s.on("error", fail("stream error"));
// The three HEADERS frames leave the cork in this tick's deferred auto-flush, which runs
// before the loop reaches the next immediate.
await new Promise(r => setImmediate(r));
const complete = new Promise(r => (onComplete = r));
fill.write(Buffer.alloc(13000, 0x43));
armed = true;
outer.write(Buffer.alloc(12000, 0x41));
if (total < EXPECTED_WIRE) await complete;
console.log(JSON.stringify({ reentered, total, A: counts[0x41], B: counts[0x42], C: counts[0x43] }));
process.exit(0);
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", fixture],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(JSON.parse(stdout.trim())).toEqual({ reentered: true, total: 28795, A: 12000, B: 3000, C: 13000 });
Comment thread
coderabbitai[bot] marked this conversation as resolved.
expect(exitCode).toBe(0);
});

it("http2 server sends protocol-error GOAWAY on stream 0", async () => {
// RFC 9113 section 6.8: GOAWAY frames MUST be sent with a stream identifier
// of 0 in the frame header; the last processed stream id lives in the
Expand Down
Loading