diff --git a/src/runtime/socket/Handlers.rs b/src/runtime/socket/Handlers.rs index 09d05db03bcb..d0838157a558 100644 --- a/src/runtime/socket/Handlers.rs +++ b/src/runtime/socket/Handlers.rs @@ -288,16 +288,51 @@ impl Handlers { generated: &GeneratedSocketConfigHandlers, is_server: bool, ) -> JsResult { + // Validate the callbacks before constructing `Handlers` so the error + // paths never drop (and thus `unprotect`) a value that was never + // `protect`ed. + // inline for (callback_fields) |field| { ... @field(generated, field) ... } + macro_rules! 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 = callback!(on_open, "onOpen"); + let on_close = callback!(on_close, "onClose"); + let on_data = callback!(on_data, "onData"); + let on_writable = callback!(on_writable, "onWritable"); + let on_timeout = callback!(on_timeout, "onTimeout"); + let on_connect_error = callback!(on_connect_error, "onConnectError"); + let on_end = callback!(on_end, "onEnd"); + let on_error = callback!(on_error, "onError"); + let on_handshake = 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, @@ -317,37 +352,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 ab01b3a77dc1..b2f26af5a145 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -821,6 +821,21 @@ 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", () => { + expect(() => Bun.connect({ hostname: "localhost", port: 0, socket: {} as any })).toThrow( + 'Expected at least "data" or "drain" callback', + ); + expect(() => Bun.listen({ hostname: "localhost", port: 0, socket: {} as any })).toThrow( + 'Expected at least "data" or "drain" callback', + ); + expect(() => Bun.connect({ hostname: "localhost", port: 0, socket: { data: 123 } as any })).toThrow( + 'Expected "onData" callback to be a function', + ); + expect(() => Bun.listen({ hostname: "localhost", port: 0, socket: { data() {}, drain: 1 } as any })).toThrow( + 'Expected "onWritable" 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