Open
Conversation
yonas
pushed a commit
that referenced
this pull request
Feb 18, 2026
## Summary Fixes oven-sh#12117, oven-sh#24118, oven-sh#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 <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
yonas
pushed a commit
that referenced
this pull request
Apr 9, 2026
Fixes a segfault when reading `.fd` on the result of `Bun.listen({ tls:
{ ... } })`.
`Listener.getFD` was calling `uws_listener.socket(true).fd()` for TLS
listeners. For `is_ssl=true`, the uSockets wrapper
`us_internal_ssl_socket_get_native_handle` returns `s->ssl`, and `fd()`
then calls `SSL_get_fd()` on it. But a listen socket has no SSL object —
SSL is per-connection — so `s->ssl` is uninitialized memory (ASAN poison
`0xbebebe...`) and the call segfaults.
Listen sockets always have a plain poll fd regardless of TLS, so get it
via the non-SSL path.
```
#3 SSL_get_rfd (ssl=0xbebebe0000000018)
#4 SSL_get_fd (ssl=0xbebebe0000000018)
#5 deps.uws.socket.NewSocketHandler(true).fd () at src/deps/uws/socket.zig:283
bun.js.api.bun.socket.Listener.getFD at src/bun.js/api/bun/socket/Listener.zig:532
```
Repro (also triggered when `console.log()` introspects the listener):
```js
const s = Bun.listen({
hostname: "localhost", port: 0,
socket: { data(){}, open(){}, close(){} },
tls: { passphrase: "abc" },
});
console.log(s.fd);
```
Found by Fuzzilli.
---------
Co-authored-by: robobun <robobun@users.noreply.github.com>
yonas
pushed a commit
that referenced
this pull request
Apr 22, 2026
…yToRoot (oven-sh#29483) Fuzzilli found a use-after-poison in the runtime auto-install path. `enqueueDependencyToRoot` passed `&lockfile.buffers.dependencies.items[dep_id]` into `enqueueDependencyWithMainAndSuccessFn`. When the manifest for the requested package is already cached (on disk or in memory) but the extracted tarball is not, control reaches `getOrPutResolvedPackageWithFindResult`, which calls `Lockfile.Package.fromNPM`. That grows `buffers.dependencies` via `ensureUnusedCapacity` to make room for the package's own dependencies, reallocating the backing storage. The subsequent `.extract` branch then read `dependency.behavior.isRequired()` from the freed buffer. ``` #0 getOrPutResolvedPackageWithFindResult PackageManagerEnqueue.zig:1520 dependency.behavior.isRequired() #1 getOrPutResolvedPackage PackageManagerEnqueue.zig:1778 #2 enqueueDependencyWithMainAndSuccessFn PackageManagerEnqueue.zig:523 #3 enqueueDependencyToRoot PackageManagerEnqueue.zig:321 #4 Resolver.enqueueDependencyToResolve resolver.zig:2356 ... #14 Bun__resolveSync #15 functionImportMeta__resolveSyncPrivate (runtime require() path) ``` Two changes: - `enqueueDependencyToRoot` now copies the `Dependency` to the stack before taking its address, matching every other caller of `enqueueDependencyWithMainAndSuccessFn` (`processDependencyListItem`, `processPeerDependencyList`, etc.). - The one read that ran after `fromNPM` now uses the `behavior` parameter that was already passed by value, instead of re-dereferencing `dependency`. Repro (debug/ASAN only): auto-install a package with a warm on-disk manifest but no extracted tarball — `fromNPM` appending even a single dependency forces a realloc of the one-entry buffer. The new test warms the cache, removes the extracted tarballs, and runs `require()` via `-e` so it goes through `Bun__resolveSync` → `enqueueDependencyToRoot`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Updates lolhtml to version v2.7.1
Compare: cloudflare/lol-html@d64457d...e9e16dc
Auto-updated by this workflow