From 7848648e095727064aaa28f092439b1f3e5e88fe Mon Sep 17 00:00:00 2001 From: Alan Stott Date: Tue, 17 Feb 2026 07:42:57 +0000 Subject: [PATCH 1/4] fix: clean up raw TCP socket on TLS upgrade close (#26766) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fixes #12117, #24118, #25948 When a TCP socket is upgraded to TLS via `tls.connect({ socket })`, `upgradeTLS()` creates **two** `TLSSocket` structs — a TLS wrapper and a raw TCP wrapper. Both are `markActive()`'d and `ref()`'d. On close, uws fires `onClose` through the **TLS context only**, so the TLS wrapper is properly cleaned up, but the raw TCP wrapper's `onClose` never fires. Its `has_pending_activity` stays `true` forever and its `ref_count` is never decremented, **leaking one raw `TLSSocket` per upgrade cycle**. This affects any code using the `tls.connect({ socket })` "starttls" pattern: - **MongoDB Node.js driver** — SDAM heartbeat connections cycle TLS upgrades every ~10s, causing unbounded memory growth in production - **mysql2** TLS upgrade path - Any custom starttls implementation ### The fix Adds a `defer` block in `NewWrappedHandler(true).onClose` that cleans up the raw TCP socket when the TLS socket closes: ```zig defer { if (!this.tcp.socket.isDetached()) { this.tcp.socket.detach(); this.tcp.has_pending_activity.store(false, .release); this.tcp.deref(); } } ``` - **`isDetached()` guard** — skips cleanup if the raw socket was already closed through another code path (e.g., JS-side `handle.close()`) - **`socket.detach()`** — marks `InternalSocket` as `.detached` so `isClosed()` returns `true` safely (the underlying `us_socket_t` is freed when uws closes the TLS context) - **`has_pending_activity.store(false)`** — allows JSC GC to collect the raw socket's JS wrapper - **`deref()`** — balances the `ref()` from `upgradeTLS`; the remaining ref is the implicit one from JSC (`ref_count.init() == 1`). When GC later calls `finalize()` → `deref()`, ref_count hits 0 and `deinit()` runs the full cleanup chain (markInactive, handlers, poll_ref, socket_context) `markInactive()` is intentionally **not** called in the defer — it must run inside `deinit()` to avoid double-freeing the handlers struct. ### Why Node.js doesn't have this bug Node.js implements TLS upgrades purely in JavaScript/C++ with OpenSSL, where the TLS wrapper takes ownership of the underlying socket. There is no separate "raw socket wrapper" that needs independent cleanup. ## Test Results ### Regression test ``` $ bun test test/js/node/tls/node-tls-upgrade-leak.test.ts 1 pass, 0 fail ``` Creates 20 TCP→TLS upgrade cycles, closes all connections, runs GC, asserts `TLSSocket` count stays below 10. ### Existing TLS test suite (all passing) ``` node-tls-upgrade.test.ts 1 pass node-tls-connect.test.ts 24 pass, 1 skip node-tls-server.test.ts 21 pass node-tls-cert.test.ts 25 pass, 3 todo renegotiation.test.ts 6 pass ``` ### MongoDB TLS scenario (patched Bun, 4 minutes) ``` Baseline: RSS=282.4 MB | Heap Used=26.4 MB Check #4: RSS=166.7 MB | Heap Used=24.2 MB — No TLSSocket growth. RSS DECREASED. ``` ## Test plan - [x] New regression test passes (`node-tls-upgrade-leak.test.ts`) - [x] All existing TLS tests pass (upgrade, connect, server, cert, renegotiation) - [x] MongoDB TLS scenario shows zero `TLSSocket` accumulation - [x] Node.js control confirms leak is Bun-specific - [ ] CI passes 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner --- src/bun.js/api/bun/socket.zig | 9 +++++ test/regression/issue/12117.test.ts | 63 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 test/regression/issue/12117.test.ts 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/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); + }); +}); From 4c61221eb41de6f751eea2d15166bf3807d8ad20 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Tue, 17 Feb 2026 20:28:22 +0000 Subject: [PATCH 2/4] [autofix.ci] apply automated fixes --- src/bun.js/bindings/BunDebugger.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/bun.js/bindings/BunDebugger.cpp b/src/bun.js/bindings/BunDebugger.cpp index 66e7be1063..37a428f1d5 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()); From 6763fe5a8a2be0bb5eb2a0b1f7f64e5a744b5061 Mon Sep 17 00:00:00 2001 From: SUZUKI Sosuke Date: Wed, 18 Feb 2026 05:30:09 +0900 Subject: [PATCH 3/4] fix: remove unsafe ObjectInitializationScope from fromEntries (#27088) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Speculative fix for [BUN-1K54](https://bun-p9.sentry.io/issues/7260165386/) — a segfault in `JSC__JSValue__fromEntries` with 238 occurrences on Windows x86_64 (Bun 1.3.9). ## Problem `JSC__JSValue__fromEntries` wraps its property insertion loop inside a `JSC::ObjectInitializationScope`. This scope is designed for fast, allocation-free object initialization using `putDirectOffset`. However, the code uses `putDirect` (which can trigger structure transitions) along with `toJSStringGC` and `toIdentifier` (which allocate on the GC heap). In **debug builds**, `ObjectInitializationScope` includes `AssertNoGC` and `DisallowVMEntry` guards that would catch this misuse immediately. In **release builds**, the scope is essentially a no-op (only emits a `mutatorFence` on destruction), so GC can silently trigger during the loop. When this happens, the GC may encounter partially-initialized object slots containing garbage values, leading to a segfault. ## Fix - Remove the `ObjectInitializationScope` block, since `putDirect` with allocating helpers is incompatible with its contract. - Add `RETURN_IF_EXCEPTION` checks inside each loop iteration to properly propagate exceptions (e.g., OOM during string allocation). ## Test Added a regression test that creates a `FileSystemRouter` with 128 routes and accesses `router.routes` under GC pressure. Verified via temporary `fprintf` logging that the test exercises the modified `JSC__JSValue__fromEntries` code path with `initialCapacity=128, clone=true`. Note: The original crash is a GC timing issue that cannot be deterministically reproduced in a test. This test validates correctness of the code path rather than reproducing the specific crash. Co-authored-by: Claude Opus 4.6 --- src/bun.js/bindings/bindings.cpp | 28 ++++++++++------------ test/js/bun/util/filesystem_router.test.ts | 24 +++++++++++++++++++ 2 files changed, 37 insertions(+), 15 deletions(-) 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/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([]); From 9256b3d77784c9e4e094dbbf3dc34e7824c8e39d Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Tue, 17 Feb 2026 12:34:19 -0800 Subject: [PATCH 4/4] fix(Error): captureStackTrace with non-stack function returns proper stack string (#27017) ### What does this PR do? When Error.captureStackTrace(e, fn) is called with a function that isn't in the call stack, all frames are filtered out and e.stack should return just the error name and message (e.g. "Error: test"), matching Node.js behavior. Previously Bun returned undefined because: 1. The empty frame vector replaced the original stack frames via setStackFrames(), but the lazy stack accessor was only installed when hasMaterializedErrorInfo() was true (i.e. stack was previously accessed). When it wasn't, JSC's internal materialization saw the empty/null frames and produced no stack property at all. 2. The custom lazy getter returned undefined when stackTrace was nullptr, instead of computing the error name+message string with zero frames. Fix: always force materialization before replacing frames, always install the custom lazy accessor, and handle nullptr stackTrace in the getter by computing the error string with an empty frame list. ### How did you verify your code works? --------- Co-authored-by: Claude Bot Co-authored-by: Claude Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> --- src/bun.js/bindings/FormatStackTraceForJS.cpp | 39 ++++++++++++------- src/js/internal/assert/assertion_error.ts | 13 +++++-- test/js/node/v8/capture-stack-trace.test.js | 36 +++++++++++++++++ 3 files changed, 71 insertions(+), 17 deletions(-) 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/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/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"); + } +});