diff --git a/src/bun.js/api/bun/socket.zig b/src/bun.js/api/bun/socket.zig index 485bdefd12..872fd902a3 100644 --- a/src/bun.js/api/bun/socket.zig +++ b/src/bun.js/api/bun/socket.zig @@ -1707,6 +1707,15 @@ pub fn NewWrappedHandler(comptime tls: bool) type { pub fn onClose(this: WrappedSocket, socket: Socket, err: c_int, data: ?*anyopaque) bun.JSError!void { if (comptime tls) { + // Clean up the raw TCP socket from upgradeTLS() — its onClose + // never fires because uws closes through the TLS context only. + defer { + if (!this.tcp.socket.isDetached()) { + this.tcp.socket.detach(); + this.tcp.has_pending_activity.store(false, .release); + this.tcp.deref(); + } + } try TLSSocket.onClose(this.tls, socket, err, data); } else { try TLSSocket.onClose(this.tcp, socket, err, data); diff --git a/src/bun.js/bindings/BunDebugger.cpp b/src/bun.js/bindings/BunDebugger.cpp index a9f67f47ab..1bc33290ee 100644 --- a/src/bun.js/bindings/BunDebugger.cpp +++ b/src/bun.js/bindings/BunDebugger.cpp @@ -368,7 +368,6 @@ public: this->jsThreadMessages.swap(messages); } - if (!debugger) { for (auto message : messages) { dispatcher.dispatchMessageFromRemote(WTF::move(message)); @@ -396,7 +395,6 @@ public: this->debuggerThreadMessages.swap(messages); } - JSFunction* onMessageFn = jsCast(jsBunDebuggerOnMessageFunction.get()); MarkedArgumentBuffer arguments; arguments.ensureCapacity(messages.size()); diff --git a/src/bun.js/bindings/FormatStackTraceForJS.cpp b/src/bun.js/bindings/FormatStackTraceForJS.cpp index dee8b43601..52fc508112 100644 --- a/src/bun.js/bindings/FormatStackTraceForJS.cpp +++ b/src/bun.js/bindings/FormatStackTraceForJS.cpp @@ -641,13 +641,16 @@ JSC_DEFINE_CUSTOM_GETTER(errorInstanceLazyStackCustomGetter, (JSGlobalObject * g OrdinalNumber column; String sourceURL; auto stackTrace = errorObject->stackTrace(); - if (stackTrace == nullptr) { - return JSValue::encode(jsUndefined()); - } - JSValue result = computeErrorInfoToJSValue(vm, *stackTrace, line, column, sourceURL, errorObject, nullptr); - stackTrace->clear(); - errorObject->setStackFrames(vm, {}); + JSValue result; + if (stackTrace == nullptr) { + WTF::Vector emptyTrace; + result = computeErrorInfoToJSValue(vm, emptyTrace, line, column, sourceURL, errorObject, nullptr); + } else { + result = computeErrorInfoToJSValue(vm, *stackTrace, line, column, sourceURL, errorObject, nullptr); + stackTrace->clear(); + errorObject->setStackFrames(vm, {}); + } RETURN_IF_EXCEPTION(scope, {}); errorObject->putDirect(vm, vm.propertyNames->stack, result, 0); return JSValue::encode(result); @@ -687,17 +690,27 @@ JSC_DEFINE_HOST_FUNCTION(errorConstructorFuncCaptureStackTrace, (JSC::JSGlobalOb JSCStackTrace::getFramesForCaller(vm, callFrame, errorObject, caller, stackTrace, stackTraceLimit); if (auto* instance = jsDynamicCast(errorObject)) { + // Force materialization before replacing the stack frames, so that JSC's + // internal lazy error info mechanism doesn't later see the replaced (possibly empty) + // stack trace and fail to create the stack property. + if (!instance->hasMaterializedErrorInfo()) + instance->materializeErrorInfoIfNeeded(vm); + RETURN_IF_EXCEPTION(scope, {}); + instance->setStackFrames(vm, WTF::move(stackTrace)); - if (instance->hasMaterializedErrorInfo()) { + + { const auto& propertyName = vm.propertyNames->stack; - VM::DeletePropertyModeScope scope(vm, VM::DeletePropertyMode::IgnoreConfigurable); + VM::DeletePropertyModeScope deleteScope(vm, VM::DeletePropertyMode::IgnoreConfigurable); DeletePropertySlot slot; JSObject::deleteProperty(instance, globalObject, propertyName, slot); - if (auto* zigGlobalObject = jsDynamicCast(globalObject)) { - instance->putDirectCustomAccessor(vm, vm.propertyNames->stack, zigGlobalObject->m_lazyStackCustomGetterSetter.get(zigGlobalObject), JSC::PropertyAttribute::CustomAccessor | 0); - } else { - instance->putDirectCustomAccessor(vm, vm.propertyNames->stack, CustomGetterSetter::create(vm, errorInstanceLazyStackCustomGetter, errorInstanceLazyStackCustomSetter), JSC::PropertyAttribute::CustomAccessor | 0); - } + } + RETURN_IF_EXCEPTION(scope, {}); + + if (auto* zigGlobalObject = jsDynamicCast(globalObject)) { + instance->putDirectCustomAccessor(vm, vm.propertyNames->stack, zigGlobalObject->m_lazyStackCustomGetterSetter.get(zigGlobalObject), JSC::PropertyAttribute::CustomAccessor | 0); + } else { + instance->putDirectCustomAccessor(vm, vm.propertyNames->stack, CustomGetterSetter::create(vm, errorInstanceLazyStackCustomGetter, errorInstanceLazyStackCustomSetter), JSC::PropertyAttribute::CustomAccessor | 0); } } else { OrdinalNumber line; diff --git a/src/bun.js/bindings/bindings.cpp b/src/bun.js/bindings/bindings.cpp index f3bf8d1f97..52703cf262 100644 --- a/src/bun.js/bindings/bindings.cpp +++ b/src/bun.js/bindings/bindings.cpp @@ -3026,22 +3026,20 @@ JSC::EncodedJSValue JSC__JSValue__fromEntries(JSC::JSGlobalObject* globalObject, return JSC::JSValue::encode(JSC::constructEmptyObject(globalObject)); } - JSC::JSObject* object = nullptr; - { - JSC::ObjectInitializationScope initializationScope(vm); - object = JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), std::min(static_cast(initialCapacity), JSFinalObject::maxInlineCapacity)); + JSC::JSObject* object = JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), std::min(static_cast(initialCapacity), JSFinalObject::maxInlineCapacity)); - if (!clone) { - for (size_t i = 0; i < initialCapacity; ++i) { - object->putDirect( - vm, JSC::PropertyName(JSC::Identifier::fromString(vm, Zig::toString(keys[i]))), - Zig::toJSStringGC(values[i], globalObject), 0); - } - } else { - for (size_t i = 0; i < initialCapacity; ++i) { - object->putDirect(vm, JSC::PropertyName(Zig::toIdentifier(keys[i], globalObject)), - Zig::toJSStringGC(values[i], globalObject), 0); - } + if (!clone) { + for (size_t i = 0; i < initialCapacity; ++i) { + object->putDirect( + vm, JSC::PropertyName(JSC::Identifier::fromString(vm, Zig::toString(keys[i]))), + Zig::toJSStringGC(values[i], globalObject), 0); + RETURN_IF_EXCEPTION(scope, {}); + } + } else { + for (size_t i = 0; i < initialCapacity; ++i) { + object->putDirect(vm, JSC::PropertyName(Zig::toIdentifier(keys[i], globalObject)), + Zig::toJSStringGC(values[i], globalObject), 0); + RETURN_IF_EXCEPTION(scope, {}); } } diff --git a/src/js/internal/assert/assertion_error.ts b/src/js/internal/assert/assertion_error.ts index c12a3f9cd9..e328f75221 100644 --- a/src/js/internal/assert/assertion_error.ts +++ b/src/js/internal/assert/assertion_error.ts @@ -378,10 +378,15 @@ class AssertionError extends Error { this.operator = operator; } ErrorCaptureStackTrace(this, stackStartFn || stackStartFunction); - // JSC::Interpreter::getStackTrace() sometimes short-circuits without creating a .stack property. - // e.g.: https://github.com/oven-sh/WebKit/blob/e32c6356625cfacebff0c61d182f759abf6f508a/Source/JavaScriptCore/interpreter/Interpreter.cpp#L501 - if ($isUndefinedOrNull(this.stack)) { - ErrorCaptureStackTrace(this, AssertionError); + // When all stack frames are above the stackStartFn (e.g. in async + // contexts), captureStackTrace produces a stack with just the error + // message and no frame lines. Retry with AssertionError as the filter + // so we get at least the frames below the constructor. + { + const s = this.stack; + if ($isUndefinedOrNull(s) || (typeof s === "string" && s.indexOf("\n at ") === -1)) { + ErrorCaptureStackTrace(this, AssertionError); + } } // Create error message including the error code in the name. this.stack; // eslint-disable-line no-unused-expressions diff --git a/test/js/bun/util/filesystem_router.test.ts b/test/js/bun/util/filesystem_router.test.ts index a8db9474f0..5753538f36 100644 --- a/test/js/bun/util/filesystem_router.test.ts +++ b/test/js/bun/util/filesystem_router.test.ts @@ -91,6 +91,30 @@ it("should find files", () => { expect(Object.values(routes).length).toBe(Object.values(fixture).length); }); +it("should handle routes under GC pressure", () => { + // Regression test for BUN-1K54: fromEntries used ObjectInitializationScope + // with putDirect, which could crash when GC triggers during string allocation. + const files = Array.from({ length: 128 }, (_, i) => `route${i}/index.tsx`); + const { dir } = make(files); + + const router = new FileSystemRouter({ + dir, + fileExtensions: [".tsx"], + style: "nextjs", + }); + + // Access routes repeatedly with GC pressure to exercise the fromEntries path + for (let i = 0; i < 10; i++) { + Bun.gc(true); + const routes = router.routes; + const keys = Object.keys(routes); + expect(keys.length).toBe(128); + for (let j = 0; j < 128; j++) { + expect(routes[`/route${j}`]).toBe(`${dir}/route${j}/index.tsx`); + } + } +}); + it("should handle empty dirs", () => { const { dir } = make([]); diff --git a/test/js/node/v8/capture-stack-trace.test.js b/test/js/node/v8/capture-stack-trace.test.js index 6e5d2008f2..aff2891f50 100644 --- a/test/js/node/v8/capture-stack-trace.test.js +++ b/test/js/node/v8/capture-stack-trace.test.js @@ -754,3 +754,39 @@ test("CallFrame.p.isAsync", async () => { expect(prepare).toHaveBeenCalledTimes(1); }); + +test("captureStackTrace with constructor function not in stack returns error string", () => { + // When the second argument to captureStackTrace is a function that isn't in + // the call stack, all frames are filtered out and .stack should still return + // the error name and message (matching Node.js behavior). + function notInStack() {} + + // Case 1: stack not accessed before captureStackTrace + { + const e = new Error("test"); + Error.captureStackTrace(e, notInStack); + expect(e.stack).toBe("Error: test"); + } + + // Case 2: stack accessed before captureStackTrace + { + const e = new Error("test"); + void e.stack; + Error.captureStackTrace(e, notInStack); + expect(e.stack).toBe("Error: test"); + } + + // Case 3: empty message + { + const e = new Error(); + Error.captureStackTrace(e, notInStack); + expect(e.stack).toBe("Error"); + } + + // Case 4: custom error name + { + const e = new TypeError("bad type"); + Error.captureStackTrace(e, notInStack); + expect(e.stack).toBe("TypeError: bad type"); + } +}); diff --git a/test/regression/issue/12117.test.ts b/test/regression/issue/12117.test.ts new file mode 100644 index 0000000000..eca8e40d0d --- /dev/null +++ b/test/regression/issue/12117.test.ts @@ -0,0 +1,63 @@ +// Regression test for TLS upgrade raw socket leak (#12117, #24118, #25948) +// When a TCP socket is upgraded to TLS via tls.connect({ socket }), +// both a TLS wrapper and a raw TCP wrapper are created in Zig. +// Previously, the raw socket's has_pending_activity was never set to +// false on close, causing it (and all its retained objects) to leak. + +import { describe, expect, it } from "bun:test"; +import { tls as COMMON_CERT, expectMaxObjectTypeCount } from "harness"; +import { once } from "node:events"; +import net from "node:net"; +import tls from "node:tls"; + +describe("TLS upgrade", () => { + it("should not leak TLSSocket objects after close", async () => { + // Create a TLS server that echoes data and closes + const server = tls.createServer( + { + key: COMMON_CERT.key, + cert: COMMON_CERT.cert, + }, + socket => { + socket.end("hello"); + }, + ); + + await once(server.listen(0, "127.0.0.1"), "listening"); + const port = (server.address() as net.AddressInfo).port; + + // Simulate the MongoDB driver pattern: create a plain TCP socket, + // then upgrade it to TLS via tls.connect({ socket }). + // Do this multiple times to accumulate leaked objects. + const iterations = 50; + + try { + for (let i = 0; i < iterations; i++) { + const tcpSocket = net.createConnection({ host: "127.0.0.1", port }); + await once(tcpSocket, "connect"); + + const tlsSocket = tls.connect({ + socket: tcpSocket, + ca: COMMON_CERT.cert, + rejectUnauthorized: false, + }); + await once(tlsSocket, "secureConnect"); + + // Read any data and destroy the TLS socket (simulates SDAM close) + tlsSocket.on("data", () => {}); + tlsSocket.destroy(); + + await once(tlsSocket, "close"); + } + } finally { + server.close(); + await once(server, "close"); + } + + // After all connections are closed and GC runs, the TLSSocket count + // should be low. Before the fix, each iteration would leak 1 raw + // TLSSocket (the TCP wrapper from upgradeTLS), accumulating over time. + // Allow some slack for prototypes/structures (typically 2-3 baseline). + await expectMaxObjectTypeCount(expect, "TLSSocket", 10, 1000); + }); +});