diff --git a/packages/bun-usockets/src/loop.c b/packages/bun-usockets/src/loop.c index bbb1ff879d..e4efad6725 100644 --- a/packages/bun-usockets/src/loop.c +++ b/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) { diff --git a/packages/bun-uws/src/AsyncSocket.h b/packages/bun-uws/src/AsyncSocket.h index 80743b960a..f61b23a5de 100644 --- a/packages/bun-uws/src/AsyncSocket.h +++ b/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); } diff --git a/packages/bun-uws/src/HttpContext.h b/packages/bun-uws/src/HttpContext.h index 89888317b8..7ecddfc7fa 100644 --- a/packages/bun-uws/src/HttpContext.h +++ b/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 *)s, httpResponseData->userData); } diff --git a/packages/bun-uws/src/HttpResponse.h b/packages/bun-uws/src/HttpResponse.h index 0fcc2cc208..7524cf2324 100644 --- a/packages/bun-uws/src/HttpResponse.h +++ b/packages/bun-uws/src/HttpResponse.h @@ -660,12 +660,6 @@ public: return httpResponseData->socketData; } - void setSocketData(void* socketData) { - HttpResponseData *httpResponseData = getHttpResponseData(); - - httpResponseData->socketData = socketData; - } - void setWriteOffset(uint64_t offset) { HttpResponseData *httpResponseData = getHttpResponseData(); diff --git a/src/bun.js/api/server/NodeHTTPResponse.zig b/src/bun.js/api/server/NodeHTTPResponse.zig index 0d25f75a08..96cdf42a22 100644 --- a/src/bun.js/api/server/NodeHTTPResponse.zig +++ b/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; diff --git a/src/bun.js/bindings/NodeHTTP.cpp b/src/bun.js/bindings/NodeHTTP.cpp index d7f2a944d7..b8605c7b09 100644 --- a/src/bun.js/bindings/NodeHTTP.cpp +++ b/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*)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 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) diff --git a/test/js/node/http/node-http-uaf-fixture.ts b/test/js/node/http/node-http-uaf-fixture.ts new file mode 100644 index 0000000000..68202ffe6b --- /dev/null +++ b/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); + } +}); diff --git a/test/js/node/http/node-http-uaf.test.ts b/test/js/node/http/node-http-uaf.test.ts new file mode 100644 index 0000000000..34955e5c46 --- /dev/null +++ b/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(); + } +});