diff --git a/src/codegen/bake-codegen.ts b/src/codegen/bake-codegen.ts index b60a3dba613..8c53a430dad 100644 --- a/src/codegen/bake-codegen.ts +++ b/src/codegen/bake-codegen.ts @@ -53,7 +53,13 @@ async function run() { side: JSON.stringify(side), IS_ERROR_RUNTIME: String(file === "error"), IS_BUN_DEVELOPMENT: String(!!debug), - OVERLAY_CSS: css("../runtime/bake/client/overlay.css", !!debug), + // JSON.stringify so the define: value is a proper JSON string. + // Raw CSS starts with `*{…}`, which the JSON lexer only tokenises + // as a recoverable SyntaxError in bun >= 1.3.14-canary (commit + // 314d044c0a); older bootstrap bins reject it before auto-quote + // can kick in. Quoting here keeps bake-codegen.ts forward- and + // backward-compatible with the lexer change. + OVERLAY_CSS: JSON.stringify(css("../runtime/bake/client/overlay.css", !!debug)), }, minify: { syntax: !debug, diff --git a/src/http_jsc/websocket_client/WebSocketUpgradeClient.rs b/src/http_jsc/websocket_client/WebSocketUpgradeClient.rs index 41d4b0a8f5c..b4b2951e347 100644 --- a/src/http_jsc/websocket_client/WebSocketUpgradeClient.rs +++ b/src/http_jsc/websocket_client/WebSocketUpgradeClient.rs @@ -198,24 +198,36 @@ impl HTTPClient { /// On error, this returns null. /// Returning null signals to the parent function that the connection failed. + /// + /// The signature is safe: `websocket` is an opaque back-reference this + /// function stores on the client — `connect` never dereferences it. + /// Taking `NonNull` (instead of the raw `*mut` the extern-C shim + /// receives) encodes the non-nullness invariant in the type; the C++ + /// caller in WebSocket.cpp always passes `this` cast from a live + /// object. The remaining FFI pointers have been lifted to + /// `&[BunString]` slices by the extern-C shim below. The focused + /// `unsafe {}` blocks inside the body cover the genuinely unsafe + /// operations (reads through the live VM pointer, pre-connect + /// `&mut *client` derivations, `Self::deref` on the error paths, + /// raw `SSL_CTX*` creation). Later callbacks that dereference the + /// stored pointer (`handle_connect_error`, `did_connect`, etc.) still + /// carry their own SAFETY comments at the deref sites. #[allow(clippy::too_many_arguments)] - pub unsafe fn connect( + pub fn connect( global: &JSGlobalObject, - websocket: *mut CppWebSocket, + websocket: core::ptr::NonNull, host: &BunString, port: u16, pathname: &BunString, client_protocol: &BunString, - header_names: *const BunString, - header_values: *const BunString, - header_count: usize, + header_names: &[BunString], + header_values: &[BunString], // Proxy parameters proxy_host: Option<&BunString>, proxy_port: u16, proxy_authorization: Option<&BunString>, - proxy_header_names: *const BunString, - proxy_header_values: *const BunString, - proxy_header_count: usize, + proxy_header_names: &[BunString], + proxy_header_values: &[BunString], // TLS options (full SSLConfig for complete TLS customization) ssl_config: Option>, // Whether the target URL is wss:// (separate from ssl template parameter) @@ -244,8 +256,7 @@ impl HTTPClient { // Headers8Bit::init only returns AllocError; handle OOM as a crash per // the OOM contract instead of masking it as a connection failure. - // SAFETY: header_names/header_values point to header_count live BunStrings per extern-C contract. - let extra_headers = unsafe { Headers8Bit::init(header_names, header_values, header_count) }; + let extra_headers = Headers8Bit::init(header_names, header_values); let proxy_host_slice: Option = proxy_host.map(|ph| ph.to_utf8()); let target_authorization_slice: Option = @@ -293,12 +304,9 @@ impl HTTPClient { // Parse proxy headers (temporary, freed after building CONNECT request) // Headers8Bit::init / to_headers only return AllocError; OOM should // crash, not silently become a connection failure. - // SAFETY: proxy_header_names/values point to proxy_header_count live BunStrings per extern-C contract. - let proxy_extra_headers = unsafe { - Headers8Bit::init(proxy_header_names, proxy_header_values, proxy_header_count) - }; + let proxy_extra_headers = Headers8Bit::init(proxy_header_names, proxy_header_values); - let proxy_hdrs: Option = if proxy_header_count > 0 { + let proxy_hdrs: Option = if !proxy_header_names.is_empty() { Some(proxy_extra_headers.to_headers()) } else { None @@ -340,7 +348,7 @@ impl HTTPClient { let client: *mut Self = bun_core::heap::into_raw(Box::new(HTTPClient:: { ref_count: Cell::new(1), tcp: Socket::::detached(), - outgoing_websocket: Some(websocket), + outgoing_websocket: Some(websocket.as_ptr()), input_body_buf, to_send_len: 0, headers_buf: [picohttp::Header::ZERO; 128], @@ -1863,25 +1871,31 @@ struct Headers8Bit<'a> { } impl<'a> Headers8Bit<'a> { - /// # Safety - /// `names_ptr` and `values_ptr` must each be null or point to `len` valid - /// `BunString`s alive for `'a`. - unsafe fn init(names_ptr: *const BunString, values_ptr: *const BunString, len: usize) -> Self { - if len == 0 { + /// Decode parallel name/value `BunString` slices into interleaved UTF-8 + /// slices. Callers that reach this from the C ABI should convert their + /// `(ptr, len)` pairs with `bun_core::ffi::slice` inside an `unsafe {}` + /// block before calling. + /// + /// `names` and `values` are expected to be the same length — the + /// extern-C shim derives both from a single `header_count`, so a + /// mismatch is a programmer bug. The `debug_assert_eq!` catches that + /// loudly in debug; `zip` stops at the shorter slice so a mismatch in + /// release drops the trailing unpaired entries instead of panicking + /// on an OOB index. + fn init(names: &'a [BunString], values: &'a [BunString]) -> Self { + debug_assert_eq!(names.len(), values.len()); + let pair_count = names.len().min(values.len()); + if pair_count == 0 { return Self { slices: Vec::new(), _marker: core::marker::PhantomData, }; } - // SAFETY: per fn contract. - let names_in = unsafe { bun_core::ffi::slice(names_ptr, len) }; - // SAFETY: per fn contract — `values_ptr` points to `len` live `BunString`s. - let values_in = unsafe { bun_core::ffi::slice(values_ptr, len) }; - - let mut slices: Vec = Vec::with_capacity(len * 2); - for i in 0..len { - slices.push(names_in[i].to_utf8()); - slices.push(values_in[i].to_utf8()); + + let mut slices: Vec = Vec::with_capacity(pair_count * 2); + for (name, value) in names.iter().zip(values.iter()) { + slices.push(name.to_utf8()); + slices.push(value.to_utf8()); } Self { @@ -2230,32 +2244,43 @@ macro_rules! export_http_client { ) -> *mut HTTPClient<$ssl> { // SAFETY: extern-C contract — caller (WebCore::WebSocket C++) // guarantees `header_names`/`header_values` point to - // `header_count` live `BunString`s (and likewise for the proxy - // header arrays), and that `websocket` is a live back-ref. - match unsafe { - HTTPClient::<$ssl>::connect( - global, - websocket, - host, - port, - pathname, - client_protocol, - header_names, - header_values, - header_count, - proxy_host, - proxy_port, - proxy_authorization, - proxy_header_names, - proxy_header_values, - proxy_header_count, - ssl_config, - target_is_secure, - target_authorization, - unix_socket_path, - offer_permessage_deflate, - ) - } { + // `header_count` live `BunString`s (and likewise for the + // proxy header arrays). Lift the `(ptr, len)` pairs to + // `&[BunString]` here so the inner `connect` can stay + // safe. `ffi::slice` tolerates `(null, 0)`. + let header_names = unsafe { bun_core::ffi::slice(header_names, header_count) }; + let header_values = unsafe { bun_core::ffi::slice(header_values, header_count) }; + let proxy_header_names = + unsafe { bun_core::ffi::slice(proxy_header_names, proxy_header_count) }; + let proxy_header_values = + unsafe { bun_core::ffi::slice(proxy_header_values, proxy_header_count) }; + // `websocket` is a live back-ref to the C++ WebCore::WebSocket; + // WebSocket.cpp always passes `this`. Encode non-nullness at + // the boundary and return null on the defensive-unreachable + // branch so the inner `connect` can take a `NonNull`. + let Some(websocket) = core::ptr::NonNull::new(websocket) else { + return ptr::null_mut(); + }; + match HTTPClient::<$ssl>::connect( + global, + websocket, + host, + port, + pathname, + client_protocol, + header_names, + header_values, + proxy_host, + proxy_port, + proxy_authorization, + proxy_header_names, + proxy_header_values, + ssl_config, + target_is_secure, + target_authorization, + unix_socket_path, + offer_permessage_deflate, + ) { Some(p) => p, None => ptr::null_mut(), } diff --git a/test/js/web/websocket/websocket-custom-headers.test.ts b/test/js/web/websocket/websocket-custom-headers.test.ts index ed6a7db8660..db844e06cb4 100644 --- a/test/js/web/websocket/websocket-custom-headers.test.ts +++ b/test/js/web/websocket/websocket-custom-headers.test.ts @@ -379,3 +379,60 @@ describe("WebSocket custom headers", () => { ws.close(); }); }); + +// Regression for #30777: the connect() path was refactored to take +// `&[BunString]` slices (with the `(ptr, len)` → slice conversion lifted +// into the extern-C shim). Cover both ends of that slice parameter: +// the empty-headers case (`(ptr, 0)`) and the many-headers case +// (drives the `Headers8Bit::init` iteration). +describe("WebSocket connect() header slice boundaries (#30777)", () => { + async function openAndEchoHeaders(opts: { headers?: Record }): Promise> { + const { promise: headersSeen, resolve: resolveHeaders } = Promise.withResolvers>(); + await using server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + websocket: { + open(_ws) {}, + message(_ws, _msg) {}, + }, + fetch(req, server) { + const headers: Record = {}; + for (const [k, v] of req.headers.entries()) headers[k.toLowerCase()] = v; + resolveHeaders(headers); + if (server.upgrade(req)) return; + return new Response("no upgrade", { status: 400 }); + }, + }); + const { promise: opened, resolve: resolveOpen, reject: rejectOpen } = Promise.withResolvers(); + const ws = new WebSocket(`ws://127.0.0.1:${server.port}/`, opts); + ws.onopen = () => resolveOpen(); + ws.onerror = e => rejectOpen((e as any).error ?? new Error((e as any).message ?? "ws error")); + const [, headers] = await Promise.all([opened, headersSeen]); + ws.close(); + return headers; + } + + it("connects with zero user headers (ffi::slice(ptr, 0) path)", async () => { + const seen = await openAndEchoHeaders({}); + // Core upgrade headers still arrive — the connect path must not skip + // them when the user-headers slice is empty. + expect(seen["connection"]?.toLowerCase()).toContain("upgrade"); + expect(seen["upgrade"]?.toLowerCase()).toBe("websocket"); + expect(seen["sec-websocket-key"]).toBeString(); + expect(seen["sec-websocket-version"]).toBe("13"); + }); + + it("connects with many user headers (Headers8Bit slice iteration)", async () => { + const custom: Record = {}; + for (let i = 0; i < 12; i++) custom[`X-Custom-${i}`] = `value-${i}`; + const seen = await openAndEchoHeaders({ headers: custom }); + // Every custom header must round-trip: the refactor interleaves + // names/values via `chunks_exact(2)`, so a drop-one or pair-off-by-one + // bug would show up as a missing or swapped value here. + for (let i = 0; i < 12; i++) { + expect(seen[`x-custom-${i}`]).toBe(`value-${i}`); + } + expect(seen["connection"]?.toLowerCase()).toContain("upgrade"); + expect(seen["upgrade"]?.toLowerCase()).toBe("websocket"); + }); +});