diff --git a/src/http/H2Client.rs b/src/http/H2Client.rs index 707d844633de..13ba23ccd947 100644 --- a/src/http/H2Client.rs +++ b/src/http/H2Client.rs @@ -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 // `Request` is still live. Same pattern as lib.rs `on_writable`. unsafe { self.build_request(body_len).detach_lifetime() } } diff --git a/src/http/lib.rs b/src/http/lib.rs index c0d854921532..7856cf519999 100644 --- a/src/http/lib.rs +++ b/src/http/lib.rs @@ -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> = + 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]> = @@ -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 { + // 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() } @@ -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). + 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() + }; let mut override_accept_encoding = false; let mut override_accept_header = false; @@ -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") => { @@ -2418,34 +2425,21 @@ 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()) => { @@ -2453,18 +2447,11 @@ impl<'a> HTTPClient<'a> { 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])); @@ -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. 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, } diff --git a/test/js/bun/http/fetch-header-count-limit.test.ts b/test/js/bun/http/fetch-header-count-limit.test.ts index c12684890953..b503ed1a58be 100644 --- a/test/js/bun/http/fetch-header-count-limit.test.ts +++ b/test/js/bun/http/fetch-header-count-limit.test.ts @@ -16,17 +16,20 @@ function makeRawHttpServer() { // First line is the request line, rest are headers. let customCount = 0; const headerNames: string[] = []; + const headers: Record = {}; for (let i = 1; i < lines.length; i++) { const lower = lines[i].toLowerCase(); const colonIdx = lines[i].indexOf(":"); if (colonIdx > 0) { - headerNames.push(lines[i].substring(0, colonIdx).toLowerCase()); + const name = lines[i].substring(0, colonIdx).toLowerCase(); + headerNames.push(name); + headers[name] = lines[i].substring(colonIdx + 1).trim(); } if (lower.startsWith("x-h-")) { customCount++; } } - const body = JSON.stringify({ customCount, headerNames }); + const body = JSON.stringify({ customCount, headerNames, headers }); socket.write( `HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: ${body.length}\r\nConnection: close\r\n\r\n${body}`, ); @@ -42,7 +45,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}`); @@ -52,8 +55,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 () => { @@ -73,22 +76,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++) { + for (let i = 0; i < 251; i++) { headers.set(`a-${String(i).padStart(4, "0")}`, `v${i}`); } - // 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"); @@ -96,12 +95,15 @@ test("default headers preserved when user headers overflow the buffer", async () const res = await fetch(`http://127.0.0.1:${port}/test`, { headers }); expect(res.status).toBe(200); - const { headerNames } = await res.json(); + const { headers: received } = 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"); + expect({ + host: received.host, + "user-agent": received["user-agent"], + accept: received.accept, + }).toEqual({ + host: "custom-host.example.com", + "user-agent": "custom-agent", + accept: "text/html", + }); }); diff --git a/test/js/web/fetch/fetch_headers.test.js b/test/js/web/fetch/fetch_headers.test.js index bee25ee3f7cf..ba4d7d180552 100644 --- a/test/js/web/fetch/fetch_headers.test.js +++ b/test/js/web/fetch/fetch_headers.test.js @@ -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').