diff --git a/src/runtime/socket/Handlers.rs b/src/runtime/socket/Handlers.rs index 2cc2462c5c30..471016260d8f 100644 --- a/src/runtime/socket/Handlers.rs +++ b/src/runtime/socket/Handlers.rs @@ -302,6 +302,42 @@ impl Handlers { generated: &GeneratedSocketConfigHandlers, is_server: bool, ) -> JsResult { + // Validate the config before constructing a `Handlers`: an invalid + // handler must return an error while there is nothing to clean up. + // A constructed-but-not-yet-`protect()`ed `Handlers` whose `Drop` runs + // `unprotect()` would trip `debug_assert!(protection_count > 0)` (debug + // abort) and unprotect callbacks `protect()` never rooted (release). + macro_rules! validate_callback { + ($field:ident, $name:literal) => {{ + let value = generated.$field; + if !value.is_undefined_or_null() && !value.is_callable() { + return Err(global_object.throw_invalid_arguments(format_args!( + "Expected \"{}\" callback to be a function", + $name + ))); + } + }}; + } + validate_callback!(on_open, "onOpen"); + validate_callback!(on_close, "onClose"); + validate_callback!(on_data, "onData"); + validate_callback!(on_writable, "onWritable"); + validate_callback!(on_timeout, "onTimeout"); + validate_callback!(on_connect_error, "onConnectError"); + validate_callback!(on_end, "onEnd"); + validate_callback!(on_error, "onError"); + validate_callback!(on_handshake, "onHandshake"); + validate_callback!(on_session, "onSession"); + validate_callback!(on_keylog, "onKeylog"); + validate_callback!(on_server_name, "onServerName"); + validate_callback!(on_alpn_callback, "onALPNCallback"); + + if !generated.on_data.is_callable() && !generated.on_writable.is_callable() { + 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, @@ -336,40 +372,27 @@ impl Handlers { protection_count: 0, }; - // inline for (callback_fields) |field| { ... @field(generated, field) ... } + // Callbacks are validated above; assign the ones that are present. 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; + ($field:ident) => {{ + if generated.$field.is_callable() { + result.$field = generated.$field; } }}; } - 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"); - assign_callback!(on_session, "onSession"); - assign_callback!(on_keylog, "onKeylog"); - assign_callback!(on_server_name, "onServerName"); - assign_callback!(on_alpn_callback, "onALPNCallback"); - - 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" - ))); - } + assign_callback!(on_open); + assign_callback!(on_close); + assign_callback!(on_data); + assign_callback!(on_writable); + assign_callback!(on_timeout); + assign_callback!(on_connect_error); + assign_callback!(on_end); + assign_callback!(on_error); + assign_callback!(on_handshake); + assign_callback!(on_session); + assign_callback!(on_keylog); + assign_callback!(on_server_name); + assign_callback!(on_alpn_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 e972e663ccd6..ada72e5bca06 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -1559,6 +1559,41 @@ it("node:net connect() reusing a server-accepted handle keeps the listener's han void stderr; }); +it("Bun.listen with an invalid socket handler throws ERR_INVALID_ARG_TYPE instead of aborting", async () => { + // Regression: an invalid handlers object used to abort the debug process on + // both validation error paths (Drop ran unprotect() on a Handlers that + // protect() never rooted). It must throw a catchable error. Run in a + // subprocess so a regression (abort) shows up as a non-zero exit rather than + // killing the test runner. + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `try { + Bun.listen({ port: 0, hostname: "127.0.0.1", socket: { data: 123 } }); + process.exit(2); // did not throw + } catch (e) { + console.log(e.code + ": " + e.message); + } + try { + Bun.listen({ port: 0, hostname: "127.0.0.1", socket: {} }); + process.exit(2); // did not throw + } catch (e) { + console.log(e.code + ": " + e.message); + }`, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toBe( + 'ERR_INVALID_ARG_TYPE: Expected "onData" callback to be a function\n' + + 'ERR_INVALID_ARG_TYPE: Expected at least "data" or "drain" callback\n', + ); + expect(exitCode).toBe(0); +}); + it.concurrent("setTypeOfService validates its argument instead of asserting", async () => { // The unfixed native binding called JSValue::asInt32() on the raw argument, // which asserts isInt32() on assert builds and silently feeds garbage to