From 747df35827fa68b764967cddc88a37f830f341f5 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 4 Jun 2026 21:09:53 +0000 Subject: [PATCH] socket: validate handler callbacks before constructing Handlers Handlers::from_generated returned errors (non-callable callback, missing data/drain callback) after constructing the Handlers value but before protect(). Dropping that value ran unprotect() on callbacks that were never protected: debug builds panic on the protection_count assertion, and release builds unbalance the JSC GC protection of any callback object protected elsewhere. Read and validate the callbacks into locals first so a Handlers only exists once it can be protected. --- src/runtime/socket/Handlers.rs | 85 ++++++++++++++++++---------------- test/js/bun/net/socket.test.ts | 13 ++++++ 2 files changed, 58 insertions(+), 40 deletions(-) diff --git a/src/runtime/socket/Handlers.rs b/src/runtime/socket/Handlers.rs index df148474690f..4259dfce641d 100644 --- a/src/runtime/socket/Handlers.rs +++ b/src/runtime/socket/Handlers.rs @@ -286,16 +286,52 @@ impl Handlers { generated: &GeneratedSocketConfigHandlers, is_server: bool, ) -> JsResult { + // Validate the callbacks before constructing `Handlers`: an error + // return after construction would run `Drop` → `unprotect()` on + // values that were never `protect()`ed. + // + // inline for (callback_fields) |field| { ... @field(generated, field) ... } + macro_rules! read_callback { + ($field:ident, $name:literal) => {{ + let value = generated.$field; + if value.is_undefined_or_null() { + JSValue::ZERO + } else if !value.is_callable() { + return Err(global_object.throw_invalid_arguments(format_args!( + "Expected \"{}\" callback to be a function", + $name + ))); + } else { + value + } + }}; + } + let on_open = read_callback!(on_open, "onOpen"); + let on_close = read_callback!(on_close, "onClose"); + let on_data = read_callback!(on_data, "onData"); + let on_writable = read_callback!(on_writable, "onWritable"); + let on_timeout = read_callback!(on_timeout, "onTimeout"); + let on_connect_error = read_callback!(on_connect_error, "onConnectError"); + let on_end = read_callback!(on_end, "onEnd"); + let on_error = read_callback!(on_error, "onError"); + let on_handshake = read_callback!(on_handshake, "onHandshake"); + + if on_data.is_empty() && on_writable.is_empty() { + return Err(global_object.throw_invalid_arguments(format_args!( + "Expected at least \"data\" or \"drain\" callback" + ))); + } + let mut result = Handlers { - on_open: JSValue::ZERO, - on_close: JSValue::ZERO, - on_data: JSValue::ZERO, - on_writable: JSValue::ZERO, - on_timeout: JSValue::ZERO, - on_connect_error: JSValue::ZERO, - on_end: JSValue::ZERO, - on_error: JSValue::ZERO, - on_handshake: JSValue::ZERO, + on_open, + on_close, + on_data, + on_writable, + on_timeout, + on_connect_error, + on_end, + on_error, + on_handshake, binary_type: match generated.binary_type { GeneratedBinaryType::Arraybuffer => BinaryType::ArrayBuffer, GeneratedBinaryType::Buffer => BinaryType::Buffer, @@ -315,37 +351,6 @@ impl Handlers { #[cfg(debug_assertions)] protection_count: 0, }; - - // inline for (callback_fields) |field| { ... @field(generated, field) ... } - macro_rules! assign_callback { - ($field:ident, $name:literal) => {{ - let value = generated.$field; - if value.is_undefined_or_null() { - } else if !value.is_callable() { - return Err(global_object.throw_invalid_arguments(format_args!( - "Expected \"{}\" callback to be a function", - $name - ))); - } else { - result.$field = value; - } - }}; - } - assign_callback!(on_open, "onOpen"); - assign_callback!(on_close, "onClose"); - assign_callback!(on_data, "onData"); - assign_callback!(on_writable, "onWritable"); - assign_callback!(on_timeout, "onTimeout"); - assign_callback!(on_connect_error, "onConnectError"); - assign_callback!(on_end, "onEnd"); - assign_callback!(on_error, "onError"); - assign_callback!(on_handshake, "onHandshake"); - - if result.on_data.is_empty() && result.on_writable.is_empty() { - return Err(global_object.throw_invalid_arguments(format_args!( - "Expected at least \"data\" or \"drain\" callback" - ))); - } result.with_async_context_if_needed(global_object); result.protect(); Ok(result) diff --git a/test/js/bun/net/socket.test.ts b/test/js/bun/net/socket.test.ts index ead9e8d856e9..2cb1f1ec0260 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -821,6 +821,19 @@ it("should throw on empty unix path from truthy non-string value", () => { expect(() => Bun.connect({ unix: [] as any, socket })).toThrow("SocketOptions.unix must be a string"); }); +it("should throw on invalid socket handlers, not crash", () => { + // Handler validation errors fire after the callback JSValues were read but + // before they were GC-protected; the error path must not unprotect them + for (const api of [Bun.listen, Bun.connect] as any[]) { + expect(() => api({ hostname: "localhost", port: 0, socket: { open() {} } })).toThrow( + 'Expected at least "data" or "drain" callback', + ); + expect(() => api({ hostname: "localhost", port: 0, socket: { open() {}, data: 123 } })).toThrow( + 'Expected "onData" callback to be a function', + ); + } +}); + it("reading .listener on a closed client socket does not use-after-free handlers", async () => { // Client-mode Handlers is heap-allocated per-connect and freed in // markInactive once the socket closes. `socket.listener` read