Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
30 changes: 21 additions & 9 deletions packages/bun-usockets/src/node_quic_shim.c
Original file line number Diff line number Diff line change
Expand Up @@ -294,19 +294,27 @@
}
void us_nq_hset_free(void *hset) { nq_hsi_discard_header_set(hset); }

#define US_NQ_MAX_HEADERS 128
/* Stack allocation covers node's default maxHeaderPairs; above that we
* spill to the heap. maxHeaderPairs is an inbound decoder limit only
* (node/src/quic/application.cc), so the encoder path imposes no policy cap. */
#define US_NQ_STACK_HEADERS 128
int us_nq_stream_send_headers(lsquic_stream_t *s, const char *buf, size_t len,
int expected, int eos) {
struct lsxpack_header hdrs[US_NQ_MAX_HEADERS];
int count = 0;
struct lsxpack_header stack_hdrs[US_NQ_STACK_HEADERS];
struct lsxpack_header *hdrs = stack_hdrs;
int count = 0, rv = -1;
size_t i = 0;
if (expected < 0 || expected > US_NQ_MAX_HEADERS) return -1;
if (expected < 0) return -1;
if (expected > US_NQ_STACK_HEADERS) {
hdrs = malloc((size_t) expected * sizeof(*hdrs));
if (!hdrs) return -1;
}
while (i < len) {
/* The caller's pair count is authoritative: latin1 encoding of the
* NUL-joined buffer maps U+0100-style code points onto the delimiter,
* which would otherwise splice extra headers out of one user value
* (node/src/node_http_common-inl.h bails the same way on n >= count_). */
if (count >= expected) return -1;
if (count >= expected) goto done;
size_t name_off = i;
while (i < len && buf[i]) i++;
size_t name_len = i - name_off;
Expand All @@ -318,19 +326,23 @@
if (i >= len) break;
i++;
unsigned char flags = (i < len) ? (unsigned char) buf[i++] : 0;
if (name_len > LSXPACK_MAX_STRLEN || val_len > LSXPACK_MAX_STRLEN)
return -1;
goto done;
lsxpack_header_set_offset2(&hdrs[count], buf, name_off, name_len,
val_off, val_len);

Check failure on line 332 in packages/bun-usockets/src/node_quic_shim.c

View check run for this annotation

Claude / Claude Code Review

lsxpack_header offset truncation past 64KB silently corrupts headers

`name_off`/`val_off` are `size_t` byte offsets into the NUL-joined buffer, but `lsxpack_header_set_offset2` stores them as `lsxpack_strlen_t` — which is `uint16_t` in Bun's build (no `LSXPACK_MAX_STRLEN` override) — so once the buffer crosses 64 KB the offsets silently wrap and lsquic encodes bytes from `(offset & 0xFFFF)`, putting garbage header names/values on the wire. Lifting the 128-header cap makes this reachable with ordinary inputs (e.g. ~200 headers × ~330-byte values), replacing the ol
Comment thread
claude[bot] marked this conversation as resolved.
Resolved
if (flags & 1) {
hdrs[count].flags = LSXPACK_NEVER_INDEX;
hdrs[count].indexed_type = 2;
}
count++;
}
if (count != expected) return -1;
lsquic_http_headers_t list = { count, hdrs };
return lsquic_stream_send_headers(s, &list, eos);
if (count == expected) {
lsquic_http_headers_t list = { count, hdrs };
rv = lsquic_stream_send_headers(s, &list, eos);
}
done:
if (hdrs != stack_hdrs) free(hdrs);
return rv;
}

static const struct lsquic_stream_if nq_stream_if = {
Expand Down
16 changes: 9 additions & 7 deletions src/runtime/node/quic/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -226,13 +226,15 @@ impl QuicStream {
let _ = s.set_http_prio(urgency, incremental);
}
for (bytes, count, eos) in self.pending_headers.with_mut(core::mem::take) {
self.wrote_to_lsquic.set(true);
if s.send_headers(&bytes, count, eos) == 0 && eos {
self.with_state(|st| {
st.fin_sent = 1;
st.write_ended = 1;
});
s.shutdown(1);
if s.send_headers(&bytes, count, eos) == 0 {
self.wrote_to_lsquic.set(true);
if eos {
self.with_state(|st| {
st.fin_sent = 1;
st.write_ended = 1;
});
s.shutdown(1);
}
}
}
if uni {
Expand Down
74 changes: 74 additions & 0 deletions test/js/node/quic/quic-stream.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,4 +178,78 @@
expect(seen.length).toBe(1);
expect(Object.keys(seen[0])).not.toContain("authorization");
});

// `application.maxHeaderPairs` is an inbound decoder limit; the outbound
// encoder path must not impose its own cap. Before the fix, the shim's
// stack buffer rejected any block with >128 field lines, sendHeaders()
// returned false, nothing went on the wire, and the request hung until the
// idle timeout tore down the whole connection.
test("encodes a HEADERS block with more than 128 field lines", async () => {
const CUSTOM = 200;
const app = { maxHeaderPairs: 1024, maxHeaderLength: 1 << 20 };
const serverSaw = Promise.withResolvers<Record<string, string>>();
await using server = await listen(
async serverSession => {
serverSession.onstream = (stream: any) => {
stream.closed.catch(() => {});
};
await serverSession.closed.catch(() => {});
},
{
sni: { "*": { keys: [key], certs: [cert] } },
application: app,
transportParams: { maxIdleTimeout: 1 },
onheaders(this: any, headers: Record<string, string>) {
serverSaw.resolve(headers);
const resp: Record<string, string> = { ":status": "200" };
for (let i = 0; i < CUSTOM; i++) resp["x-resp-" + i] = String(i);
// Server direction too: more than 128 response headers.
expect(this.sendHeaders(resp)).toBe(true);

Check warning on line 207 in test/js/node/quic/quic-stream.test.ts

View check run for this annotation

Claude / Claude Code Review

expect() inside server onheaders callback

The `expect(this.sendHeaders(resp)).toBe(true)` runs inside the server's `onheaders` callback — REVIEW.md explicitly rejects this pattern ("never throw inside event callbacks"). If the server→client encode path regresses, the throw skips `this.writer.endSync()`, `gotHeaders` never resolves, and the test hangs until the jest timeout instead of surfacing the real assertion. Consider capturing the return value into a resolver (e.g. `serverSendOk.resolve(this.sendHeaders(resp))`) and asserting it fr
Comment thread
claude[bot] marked this conversation as resolved.
Resolved
this.writer.endSync();
},
},
);

// A fresh endpoint so this test's `application` options reach the lsquic
// engine rather than whatever an earlier `connect()` in this file built.
const client = await connect(server.address, {
endpoint: {},
servername: "localhost",
verifyPeer: "manual",
application: app,
transportParams: { maxIdleTimeout: 1 },
});
await client.opened;

const req: Record<string, string> = {
":method": "GET",
":path": "/",
":scheme": "https",
":authority": "localhost",
};
for (let i = 0; i < CUSTOM; i++) req["x-req-" + i] = String(i);

const gotHeaders = Promise.withResolvers<Record<string, string>>();
const stream = await client.createBidirectionalStream({
onheaders(headers: Record<string, string>) {
gotHeaders.resolve(headers);
},
});
expect(stream.sendHeaders(req, { terminal: true })).toBe(true);

const seen = await serverSaw.promise;
expect(Object.keys(seen)).toHaveLength(CUSTOM + 4);
expect(seen["x-req-0"]).toBe("0");
expect(seen["x-req-" + (CUSTOM - 1)]).toBe(String(CUSTOM - 1));

const resp = await gotHeaders.promise;
expect(resp[":status"]).toBe("200");
expect(Object.keys(resp)).toHaveLength(CUSTOM + 1);
expect(resp["x-resp-" + (CUSTOM - 1)]).toBe(String(CUSTOM - 1));

for await (const _ of stream) {
}
expect(client.destroyed).toBe(false);
client.close();
});
});
Loading