Fix panic when socket handler validation fails in Bun.listen/Bun.connect

src/runtime/socket/Handlers.rs

@@ -286,16 +286,52 @@ impl Handlers {
         generated: &GeneratedSocketConfigHandlers,
         is_server: bool,
     ) -> JsResult<Handlers> {
+        // 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)

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