Fix node:http UAF (#18485)

Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
Co-authored-by: Dylan Conway <35280289+dylan-conway@users.noreply.github.com>

packages/bun-usockets/src/loop.c

@@ -213,21 +213,21 @@ void us_internal_free_closed_sockets(struct us_loop_t *loop) {
         us_poll_free((struct us_poll_t *) s, loop);
         s = next;
     }
-    loop->data.closed_head = 0;
+    loop->data.closed_head = NULL;
 
     for (struct us_udp_socket_t *s = loop->data.closed_udp_head; s; ) {
         struct us_udp_socket_t *next = s->next;
         us_poll_free((struct us_poll_t *) s, loop);
         s = next;
     }
-    loop->data.closed_udp_head = 0;
+    loop->data.closed_udp_head = NULL;
 
     for (struct us_connecting_socket_t *s = loop->data.closed_connecting_head; s; ) {
         struct us_connecting_socket_t *next = s->next;
         us_free(s);
         s = next;
     }
-    loop->data.closed_connecting_head = 0;
+    loop->data.closed_connecting_head = NULL;
 }
 
 void us_internal_free_closed_contexts(struct us_loop_t *loop) {
@@ -236,7 +236,7 @@ void us_internal_free_closed_contexts(struct us_loop_t *loop) {
         us_free(ctx);
         ctx = next;
     }
-    loop->data.closed_context_head = 0;
+    loop->data.closed_context_head = NULL;
 }
 
 void sweep_timer_cb(struct us_internal_callback_t *cb) {

packages/bun-uws/src/AsyncSocket.h

@@ -131,7 +131,7 @@ public:
         getLoopData()->setCorkedSocket(this, SSL);
     }
 
-    /* Returns wheter we are corked or not */
+    /* Returns whether we are corked */
     bool isCorked() {
         return getLoopData()->isCorkedWith(this);
     }

packages/bun-uws/src/HttpContext.h

@@ -125,7 +125,7 @@ private:
             }
 
             /* Signal broken HTTP request only if we have a pending request */
-            if (httpResponseData->onAborted) {
+            if (httpResponseData->onAborted != nullptr && httpResponseData->userData != nullptr) {
                 httpResponseData->onAborted((HttpResponse<SSL> *)s, httpResponseData->userData);
             }
 

packages/bun-uws/src/HttpResponse.h

@@ -660,12 +660,6 @@ public:
         return httpResponseData->socketData;
     }
 
-    void setSocketData(void* socketData) {
-        HttpResponseData<SSL> *httpResponseData = getHttpResponseData();
-
-        httpResponseData->socketData = socketData;
-    }
-
     void setWriteOffset(uint64_t offset) {
         HttpResponseData<SSL> *httpResponseData = getHttpResponseData();
 

src/bun.js/api/server/NodeHTTPResponse.zig

@@ -9,6 +9,7 @@ aborted: bool = false,
 finished: bool = false,
 ended: bool = false,
 upgraded: bool = false,
+closed: bool = false,
 hasCustomOnData: bool = false,
 is_request_pending: bool = true,
 body_read_state: BodyReadState = .none,
@@ -991,7 +992,7 @@ pub fn cork(this: *NodeHTTPResponse, globalObject: *JSC.JSGlobalObject, callfram
         return globalObject.throwInvalidArgumentTypeValue("cork", "function", arguments[0]);
     }
 
-    if (this.finished or this.aborted) {
+    if (this.finished or this.aborted or this.closed) {
         return globalObject.ERR_STREAM_ALREADY_FINISHED("Stream is already ended", .{}).throw();
     }
 
@@ -1042,6 +1043,10 @@ comptime {
 }
 extern "c" fn Bun__setNodeHTTPServerSocketUsSocketValue(JSC.JSValue, ?*anyopaque) void;
 
+pub export fn Bun__NodeHTTPResponse_setClosed(response: *NodeHTTPResponse) void {
+    response.closed = true;
+}
+
 const NodeHTTPResponse = @This();
 
 const JSGlobalObject = JSC.JSGlobalObject;

src/bun.js/bindings/NodeHTTP.cpp

@@ -25,6 +25,8 @@
 extern "C" uint64_t uws_res_get_remote_address_info(void* res, const char** dest, int* port, bool* is_ipv6);
 extern "C" uint64_t uws_res_get_local_address_info(void* res, const char** dest, int* port, bool* is_ipv6);
 
+extern "C" void Bun__NodeHTTPResponse_setClosed(void* zigResponse);
+
 namespace Bun {
 
 using namespace JSC;
@@ -121,9 +123,7 @@ public:
     static void clearSocketData(us_socket_t* socket)
     {
         auto* httpResponseData = (uWS::HttpResponseData<SSL>*)us_socket_ext(SSL, socket);
-        if (httpResponseData->socketData) {
-            httpResponseData->socketData = nullptr;
-        }
+        httpResponseData->socketData = nullptr;
     }
 
     void close()
@@ -187,6 +187,9 @@ public:
     {
         this->socket = nullptr;
         this->m_duplex.clear();
+        if (auto* res = this->currentResponseObject.get(); res != nullptr && res->m_ctx != nullptr) {
+            Bun__NodeHTTPResponse_setClosed(res->m_ctx);
+        }
         this->currentResponseObject.clear();
 
         // This function can be called during GC!
@@ -200,20 +203,18 @@ public:
         WebCore::ScriptExecutionContext* scriptExecutionContext = globalObject->scriptExecutionContext();
 
         if (scriptExecutionContext) {
-            JSC::gcProtect(this);
             scriptExecutionContext->postTask([self = this](ScriptExecutionContext& context) {
                 WTF::NakedPtr<JSC::Exception> exception;
                 auto* globalObject = defaultGlobalObject(context.globalObject());
                 auto* thisObject = self;
                 auto* callbackObject = thisObject->functionToCallOnClose.get();
                 if (!callbackObject) {
-                    JSC::gcUnprotect(self);
+                    self->strongThis.clear();
                     return;
                 }
                 auto callData = JSC::getCallData(callbackObject);
                 MarkedArgumentBuffer args;
                 EnsureStillAliveScope ensureStillAlive(self);
-                JSC::gcUnprotect(self);
 
                 if (globalObject->scriptExecutionStatus(globalObject, thisObject) == ScriptExecutionStatus::Running) {
                     profiledCall(globalObject, JSC::ProfilingReason::API, callbackObject, callData, thisObject, args, exception);
@@ -223,10 +224,9 @@ public:
                         globalObject->reportUncaughtExceptionAtEventLoop(globalObject, ptr);
                     }
                 }
+                self->strongThis.clear();
             });
         }
-
-        this->strongThis.clear();
     }
 
     static Structure* createStructure(JSC::VM& vm, JSC::JSGlobalObject* globalObject)

test/js/node/http/node-http-uaf-fixture.ts

@@ -0,0 +1,53 @@
+import express from "express";
+import bodyParser from "body-parser";
+import { fetch } from "undici";
+import { setTimeout as sleep } from "node:timers/promises";
+
+const CONCURRENCY = 100;
+
+const app = express();
+app.use(bodyParser.json());
+
+app.post("/error", (req, res) => {
+  try {
+    // This specific pattern causes the segfault in Bun v1.2.6
+    const headers = { location: undefined };
+    headers.location.split("*/")["2"].split(")")["0"];
+  } catch (err) {
+    setTimeout(() => res.status(500).json({ error: err.message }), 1);
+  }
+});
+
+const server = app.listen(0, async () => {
+  const port = server.address().port;
+  console.log(`Server running on http://localhost:${port}`);
+
+  const active = new Set();
+
+  async function makeRequest(id) {
+    const controller = new AbortController();
+
+    setTimeout(() => controller.abort(), Math.random() * 5 + 1);
+
+    try {
+      await fetch(`http://localhost:${port}/error`, {
+        method: "POST",
+        headers: { "Content-Type": "application/json" },
+        body: "{}",
+        signal: controller.signal,
+      }).catch(() => {});
+    } catch (e) {}
+
+    active.delete(id);
+  }
+
+  console.log(`Starting concurrent requests...`);
+  for (let i = 0; i < 10000; i++) {
+    while (active.size >= CONCURRENCY) {
+      await sleep(1);
+    }
+
+    active.add(i);
+    makeRequest(i);
+  }
+});

test/js/node/http/node-http-uaf.test.ts

@@ -0,0 +1,17 @@
+import { test, expect } from "bun:test";
+import { join } from "path";
+import { bunExe, bunEnv } from "harness";
+
+test("should not crash on abort", async () => {
+  for (let i = 0; i < 2; i++) {
+    const { exitCode, signalCode } = await Bun.spawnSync({
+      cmd: [bunExe(), join(import.meta.dir, "node-http-uaf-fixture.ts")],
+      env: bunEnv,
+      stdout: "ignore",
+      stderr: "ignore",
+      stdin: "ignore",
+    });
+    expect(exitCode).not.toBeNull();
+    expect(signalCode).toBeUndefined();
+  }
+});