Skip to content
Open
Show file tree
Hide file tree
Changes from 6 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
10 changes: 5 additions & 5 deletions src/http/H2Client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,11 +127,11 @@ mod bridge {
#[inline]
pub(crate) fn h2_build_request(&mut self, body_len: usize) -> picohttp::Request<'static> {
// SAFETY: `build_request` returns a `Request<'_>` whose borrowed
// slices point only at (a) the thread-local
// `SHARED_REQUEST_HEADERS_BUF` static and (b) `self.header_buf`,
// which is itself `&'static [u8]` — neither is tied to the `&mut
// self` borrow. Erasing to `'static` lets
// `ClientSession::attach` re-borrow `client` while the
// slices point only at (a) the per-HTTP-thread request-header
// scratch (`SHARED_REQUEST_HEADERS_BUF` or its `_OVERFLOW` Vec)
// and (b) `self.header_buf`, which is itself `&'static [u8]` —
// neither is tied to the `&mut self` borrow. Erasing to `'static`
// lets `ClientSession::attach` re-borrow `client` while the
Comment thread
robobun marked this conversation as resolved.
// `Request` is still live. Same pattern as lib.rs `on_writable`.
Comment thread
robobun marked this conversation as resolved.
unsafe { self.build_request(body_len).detach_lifetime() }
}
Expand Down
115 changes: 52 additions & 63 deletions src/http/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1047,9 +1047,14 @@ static PRINT_EVERY_I: AtomicUsize = AtomicUsize::new(0);

// we always rewrite the entire HTTP request when write() returns EAGAIN
// so we can reuse this buffer
const MAX_REQUEST_HEADERS: usize = 256;
static SHARED_REQUEST_HEADERS_BUF: bun_core::RacyCell<[picohttp::Header; MAX_REQUEST_HEADERS]> =
bun_core::RacyCell::new([picohttp::Header::ZERO; MAX_REQUEST_HEADERS]);
const MAX_REQUEST_HEADERS_INLINE: usize = 256;
static SHARED_REQUEST_HEADERS_BUF: bun_core::RacyCell<
[picohttp::Header; MAX_REQUEST_HEADERS_INLINE],
> = bun_core::RacyCell::new([picohttp::Header::ZERO; MAX_REQUEST_HEADERS_INLINE]);

// Spillover for requests with more than MAX_REQUEST_HEADERS_INLINE fields.
static SHARED_REQUEST_HEADERS_OVERFLOW: bun_core::RacyCell<Vec<picohttp::Header>> =
bun_core::RacyCell::new(Vec::new());

// this doesn't need to be stack memory because it is immediately cloned after use
static SHARED_RESPONSE_HEADERS_BUF: bun_core::RacyCell<[picohttp::Header; 256]> =
Expand All @@ -1071,11 +1076,16 @@ static SINGLE_PACKET_SMALL_BUFFER: bun_core::RacyCell<[u8; 16 * 1024]> =
mod scratch {
use super::*;
#[inline]
pub(super) fn request_headers() -> &'static mut [picohttp::Header; MAX_REQUEST_HEADERS] {
pub(super) fn request_headers() -> &'static mut [picohttp::Header; MAX_REQUEST_HEADERS_INLINE] {
// SAFETY: see module-level INVARIANT.
unsafe { &mut *SHARED_REQUEST_HEADERS_BUF.get() }
}
#[inline]
pub(super) fn request_headers_overflow() -> &'static mut Vec<picohttp::Header> {
// SAFETY: see module-level INVARIANT.
unsafe { &mut *SHARED_REQUEST_HEADERS_OVERFLOW.get() }
}
#[inline]
pub(super) fn response_headers() -> &'static mut [picohttp::Header; 256] {
// SAFETY: see module-level INVARIANT.
unsafe { &mut *SHARED_RESPONSE_HEADERS_BUF.get() }
Expand Down Expand Up @@ -2357,7 +2367,18 @@ impl<'a> HTTPClient<'a> {
let header_entries = self.header_entries.slice();
let header_names = header_entries.items_name();
let header_values = header_entries.items_value();
let request_headers_buf = scratch::request_headers();

// Default headers that may be appended after user headers
// (Connection, User-Agent, Accept, Host, Accept-Encoding, Content-Length/Transfer-Encoding).
Comment thread
robobun marked this conversation as resolved.
const MAX_DEFAULT_HEADERS: usize = 6;
let needed = header_names.len() + MAX_DEFAULT_HEADERS;
let request_headers_buf: &mut [picohttp::Header] = if needed <= MAX_REQUEST_HEADERS_INLINE {
scratch::request_headers().as_mut_slice()
} else {
let overflow = scratch::request_headers_overflow();
overflow.resize(needed, picohttp::Header::ZERO);
overflow.as_mut_slice()
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.

let mut override_accept_encoding = false;
let mut override_accept_header = false;
Expand All @@ -2367,45 +2388,31 @@ impl<'a> HTTPClient<'a> {
let mut add_transfer_encoding = true;
let mut original_content_length: Option<&[u8]> = None;

// Reserve slots for default headers that may be appended after user headers
// (Connection, User-Agent, Accept, Host, Accept-Encoding, Content-Length/Transfer-Encoding).
const MAX_DEFAULT_HEADERS: usize = 6;
const MAX_USER_HEADERS: usize = MAX_REQUEST_HEADERS - MAX_DEFAULT_HEADERS;

for (i, head) in header_names.iter().enumerate() {
let name = self.header_str(*head);
// Hash it as lowercase
let hash = hash_header_name(name);

// Whether this header will actually be written to the buffer.
// Override flags must only be set when the header is kept, otherwise
// the default header is suppressed but the user header is dropped,
// leaving the header entirely absent from the request.
let will_append = header_count < MAX_USER_HEADERS;

// Skip host and connection header
// we manage those
match hash {
h if h == hash_header_const(b"Content-Length") => {
// Content-Length is always consumed (never written to the buffer).
original_content_length = Some(self.header_str(header_values[i]));
continue;
}
h if h == hash_header_const(b"Connection") => {
if will_append {
override_connection_header = true;
let connection_value = self.header_str(header_values[i]);
if bun_core::strings::eql_case_insensitive_ascii_check_length(
connection_value,
b"close",
) {
self.flags.disable_keepalive = true;
} else if bun_core::strings::eql_case_insensitive_ascii_check_length(
connection_value,
b"keep-alive",
) {
self.flags.disable_keepalive = false;
}
override_connection_header = true;
let connection_value = self.header_str(header_values[i]);
if bun_core::strings::eql_case_insensitive_ascii_check_length(
connection_value,
b"close",
) {
self.flags.disable_keepalive = true;
} else if bun_core::strings::eql_case_insensitive_ascii_check_length(
connection_value,
b"keep-alive",
) {
self.flags.disable_keepalive = false;
}
}
h if h == hash_header_const(b"if-modified-since") => {
Expand All @@ -2418,53 +2425,33 @@ impl<'a> HTTPClient<'a> {
}
}
h if h == hash_header_const(HOST_HEADER_NAME) => {
if will_append {
override_host_header = true;
}
override_host_header = true;
}
h if h == hash_header_const(b"Accept") => {
if will_append {
override_accept_header = true;
}
override_accept_header = true;
}
h if h == hash_header_const(b"User-Agent") => {
if will_append {
override_user_agent = true;
}
override_user_agent = true;
}
h if h == hash_header_const(b"Accept-Encoding") => {
if will_append {
override_accept_encoding = true;
}
override_accept_encoding = true;
}
h if h == hash_header_const(b"Upgrade") => {
if will_append {
let value = self.header_str(header_values[i]);
if !bun_core::strings::eql_any_case_insensitive_ascii(
value,
&[b"h2", b"h2c"],
) {
self.flags.upgrade_state = HTTPUpgradeState::Pending;
}
let value = self.header_str(header_values[i]);
if !bun_core::strings::eql_any_case_insensitive_ascii(value, &[b"h2", b"h2c"]) {
self.flags.upgrade_state = HTTPUpgradeState::Pending;
}
}
h if h == hash_header_const(CHUNKED_ENCODED_HEADER.name()) => {
if !self.flags.is_streaming_request_body {
continue;
}
// We don't want to override chunked encoding header if it was set by the user
if will_append {
add_transfer_encoding = false;
}
add_transfer_encoding = false;
}
_ => {}
}

// Silently drop excess headers to stay within the fixed-size request header buffer.
if !will_append {
continue;
}

request_headers_buf[header_count] =
picohttp::Header::new(name, self.header_str(header_values[i]));

Expand Down Expand Up @@ -2544,15 +2531,17 @@ impl<'a> HTTPClient<'a> {
// SAFETY: every borrowed slice points into storage that outlives the
// returned `Request` — `Method::as_str()` is `'static`; `url.pathname`
// borrows `self.url` (lives for the client); `request_headers_buf` is
// the per-HTTP-thread `SHARED_REQUEST_HEADERS_BUF` static. Return as
// `'static` so callers don't pin `&mut self` for the rest of their fn.
// either the per-HTTP-thread `SHARED_REQUEST_HEADERS_BUF` static or the
// `SHARED_REQUEST_HEADERS_OVERFLOW` Vec (buffer moves on resize). Callers
// serialize the returned `Request` before the next `build_request()` call
// resizes, so the erased borrow never dangles. Return as `'static` so
// callers don't pin `&mut self` for the rest of their fn.
Comment thread
robobun marked this conversation as resolved.
picohttp::Request {
method: self.method.as_str().as_bytes(),
// SAFETY: `url.pathname` borrows `self.url`, which outlives the returned `Request`.
path: unsafe { bun_ptr::detach_lifetime(self.url.pathname) },
minor_version: 1,
// SAFETY: `request_headers_buf` is the per-HTTP-thread
// `SHARED_REQUEST_HEADERS_BUF` static, outliving the returned `Request`.
// SAFETY: see the block-level comment above.
headers: unsafe { bun_ptr::detach_lifetime(&request_headers_buf[0..header_count]) },
bytes_read: 0,
}
Expand Down
21 changes: 7 additions & 14 deletions test/js/bun/http/fetch-header-count-limit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ test("fetch with many headers does not crash", async () => {
await once(server, "listening");
const port = (server.address() as any).port;

// Build a request with more headers than the internal fixed-size buffer (256).
// Build a request with more headers than the inline fixed-size scratch (256).
const headers = new Headers();
for (let i = 0; i < 300; i++) {
headers.set(`x-h-${i}`, `v${i}`);
Expand All @@ -52,8 +52,8 @@ test("fetch with many headers does not crash", async () => {
expect(res.status).toBe(200);

const { customCount } = await res.json();
// Excess headers beyond the internal cap (250 user headers) are silently dropped.
expect(customCount).toBe(250);
// There is no request-side field-count cap; every header reaches the origin.
expect(customCount).toBe(300);
});

test("fetch with exactly 250 custom headers sends all of them", async () => {
Expand All @@ -73,22 +73,18 @@ test("fetch with exactly 250 custom headers sends all of them", async () => {
expect(customCount).toBe(250);
});

test("default headers preserved when user headers overflow the buffer", async () => {
test("user-supplied Host/User-Agent/Accept are sent alongside >250 other headers", async () => {
await using server = makeRawHttpServer().listen(0);
await once(server, "listening");
const port = (server.address() as any).port;

// Use "a-" prefixed headers which sort alphabetically before "accept",
// "host", "user-agent", etc. This ensures the filler headers consume all
// 250 user-header slots first, pushing the special headers into overflow.
// Without the fix, the override flags for Host/Accept/User-Agent would
// still be set (suppressing defaults), but the headers themselves would be
// dropped — resulting in missing mandatory headers like Host.
// "a-" prefixed headers sort before "accept", "host", "user-agent" in a
// Headers object, so the special headers land past the inline-scratch
// boundary and exercise the overflow path.
const headers = new Headers();
for (let i = 0; i < 250; i++) {
headers.set(`a-${String(i).padStart(4, "0")}`, `v${i}`);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// These special headers sort after "a-*" and will overflow.
headers.set("Host", "custom-host.example.com");
headers.set("User-Agent", "custom-agent");
headers.set("Accept", "text/html");
Expand All @@ -98,9 +94,6 @@ test("default headers preserved when user headers overflow the buffer", async ()

const { headerNames } = await res.json();

// Even though the user-supplied Host, User-Agent, and Accept were dropped
// due to overflow, the DEFAULT versions of these headers must still be
// present (the override flags should not have been set for dropped headers).
expect(headerNames).toContain("host");
expect(headerNames).toContain("user-agent");
expect(headerNames).toContain("accept");
Expand Down
37 changes: 37 additions & 0 deletions test/js/web/fetch/fetch_headers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,43 @@ describe("Headers", async () => {
}
});

it.each([251, 300])("sends every request header field on the wire (%i user headers)", async N => {
// build_request() previously capped user headers at 250 (256-slot scratch
// minus 6 defaults) and silently dropped the rest; the origin must receive
// every field.
const { promise: gotHead, resolve, reject } = Promise.withResolvers();
const srv = net.createServer(s => {
let b = Buffer.alloc(0);
s.on("data", d => {
b = Buffer.concat([b, d]);
if (b.indexOf("\r\n\r\n") >= 0) {
resolve(b.toString("latin1").split("\r\n\r\n")[0]);
s.end("HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\nok");
}
});
s.once("error", reject);
});
srv.listen(0, "127.0.0.1");
await once(srv, "listening");
try {
const port = srv.address().port;
const headers = {};
for (let i = 0; i < N; i++) headers["x-" + String(i).padStart(4, "0")] = "v";
const [res, head] = await Promise.all([fetch(`http://127.0.0.1:${port}/`, { headers }), gotHead]);
await res.text();
const lines = head.split("\r\n").slice(1);
const received = lines.filter(l => l.startsWith("x-")).sort();
const expected = Object.keys(headers)
.map(k => `${k}: v`)
.sort();
expect(received.length).toBe(N);
expect(received).toEqual(expected);
expect(res.status).toBe(200);
} finally {
await new Promise(r => srv.close(r));
}
});

it("Invalid values for well-known headers name the header, not its index", () => {
// The HTTPHeaderName fast path must report the header's name (e.g. 'Location'),
// not its numeric enum value (e.g. '51').
Expand Down
Loading