Fix null SSL* dereference in TLSSocket.getServername after close#30145
Conversation
socket.ssl() returns null once the socket is detached (e.g. inside the close handler). Every other TLS getter in tls_socket_functions.zig guards this with `orelse return .js_undefined`, but getServername passed the null straight to SSL_get_servername, which dereferences ssl->hostname.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a null-check in ChangesNull-Safe Servername Retrieval
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Review rate limit: 0/5 reviews remaining, refill in 56 minutes and 16 seconds. Comment |
|
Updated 4:45 AM PT - May 3rd, 2026
❌ @robobun, your commit 52bdf47 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 30145That installs a local version of the PR into your bun-30145 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line null guard matching the pattern used by every other TLS getter in this file, with a regression test.
Extended reasoning...
Overview
This PR fixes a segfault in TLSSocket.getServername() when called after the socket has been closed/detached. The fix is a single-line change in src/bun.js/api/bun/socket/tls_socket_functions.zig: const ssl_ptr = this.socket.ssl(); → const ssl_ptr = this.socket.ssl() orelse return .js_undefined;. A regression test is added to test/js/bun/net/socket.test.ts that connects via TLS, ends the socket in handshake, and calls getServername() from the close handler (where the SSL* is already null).
Security risks
None. This adds a null guard that prevents a null-pointer dereference; it does not change auth, crypto behavior, or expose any new data. Returning undefined after close is consistent with all other TLS getters in this file and is the safe defensive choice.
Level of scrutiny
Low. The change is mechanical and follows an established pattern: I verified that ssl() returns ?*BoringSSL.SSL (src/deps/uws/socket.zig:120, UpgradedDuplex.zig:424, WindowsNamedPipe.zig:543), and that every sibling getter in this file (getCipher, getTLSVersion, getCertificate, getSession, getTLSTicket, getPeerX509Certificate, etc.) already uses the identical orelse return .js_undefined guard. getServername was simply the one outlier that was missed.
Other factors
- No CODEOWNERS entry covers these paths.
- The bug-hunting system found no issues.
- The test is self-contained (loopback TLS listener on port 0,
usingfor cleanup) and assertsundefinedis returned both inside theclosehandler and on the resolved client object afterward. - The PR description includes ASAN repro output confirming the crash and verification that the fix resolves it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/net/socket.test.ts`:
- Line 867: The test currently swallows socket errors by defining an empty
error() handler which can leave "await promise" hanging; update the error
handler (the error() function attached to the socket in the test) to reject the
test promise with the received error (e.g., error(err) { reject(err || new
Error('socket error')) }) so any TLS or socket failures cause the awaiting
promise to reject and make failures deterministic and debuggable; ensure the
handler closes the socket if necessary and that the promise variable referenced
by "await promise" is the same promise being rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef3f45ec-a134-4d11-b9e3-3127b6951a97
📒 Files selected for processing (2)
src/bun.js/api/bun/socket/tls_socket_functions.zigtest/js/bun/net/socket.test.ts
There was a problem hiding this comment.
LGTM — one-line null guard matching the existing pattern in this file, with a regression test.
Extended reasoning...
Overview
This PR adds a null check to getServername in src/bun.js/api/bun/socket/tls_socket_functions.zig, changing const ssl_ptr = this.socket.ssl(); to const ssl_ptr = this.socket.ssl() orelse return .js_undefined;. It also adds a regression test in test/js/bun/net/socket.test.ts that calls getServername() from the close handler (after the socket has been detached) and asserts it returns undefined rather than crashing.
Security risks
None. This is purely a defensive null check that prevents a segfault when SSL_get_servername would otherwise be called with a null SSL*. It does not change auth, crypto, or permission semantics — it just returns undefined instead of dereferencing null.
Level of scrutiny
Low. The change is a single-line application of an idiom already used by every sibling function in the same file (getCipher, getTLSVersion, getCertificate, getSession, getTLSTicket, getPeerX509Certificate, etc. all use this.socket.ssl() orelse return ...). I verified that ssl() returns ?*BoringSSL.SSL in src/deps/uws/socket.zig:120 (and the UpgradedDuplex / WindowsNamedPipe variants), so the orelse is type-correct. getServername was simply the one getter that had been missed.
Other factors
- The PR description includes an ASAN repro confirming the SEGV at offset 0x90 without the fix.
- The new test is self-contained, uses an ephemeral port and
usingfor cleanup, and follows the style of adjacent tests in the file. - No CODEOWNERS entries cover these paths.
- No bugs were flagged by the bug-hunting system and there are no outstanding reviewer comments.
There was a problem hiding this comment.
LGTM — one-line null guard matching the pattern used by every other TLS getter in this file.
Extended reasoning...
Overview
This PR changes a single line in src/bun.js/api/bun/socket/tls_socket_functions.zig, adding orelse return .js_undefined to the this.socket.ssl() call in getServername. It also adds a regression test in test/js/bun/net/socket.test.ts that calls getServername() from the close handler (after the underlying SSL* is detached) and asserts it returns undefined instead of segfaulting.
Security risks
None. This is a defensive null check that prevents a segfault. It does not change auth, crypto behavior, or expose any data — it returns undefined where previously the process would crash on a null deref. No new attack surface is introduced.
Level of scrutiny
Low. The fix is purely mechanical: ssl() returns ?*BoringSSL.SSL (verified in src/deps/uws/socket.zig and friends), and literally every other function in this file — getCipher, getTLSVersion, getCertificate, getSession, getTLSTicket, getPeerX509Certificate, getALPNProtocol, etc. — already uses the exact orelse return .js_undefined (or .jsNull()/.false) guard. getServername was the lone outlier. The PR description includes an ASAN repro confirming the segfault and verification that the fix resolves it.
Other factors
- No CODEOWNERS cover the modified paths.
- The bug-hunting system found no issues.
- The one CodeRabbit suggestion (reject the promise on socket error in the test) was addressed in commit 28522bd and the inline comment is resolved.
- The test follows existing patterns in the file (
Promise.withResolvers,using listener, no timers) and won't hang on failure since botherrorandclosesettle the promise.
…servername-null-ssl
There was a problem hiding this comment.
LGTM — one-line null guard matching the pattern used by every other TLS getter in this file.
Extended reasoning...
Overview
This PR adds orelse return .js_undefined to getServername in src/bun.js/api/bun/socket/tls_socket_functions.zig, fixing a segfault when getServername() is called after the socket is closed/detached. A regression test in test/js/bun/net/socket.test.ts exercises the crash path.
Security risks
None introduced. This removes a null-pointer dereference (SEGV at offset 0x90 in BoringSSL). Returning undefined when the SSL handle is gone is the same defensive behavior already used by getCipher, getTLSVersion, getCertificate, getSession, getTLSTicket, getPeerX509Certificate, getX509Certificate, getALPNProtocol, renegotiate, etc. in the same file — getServername was the lone holdout.
Level of scrutiny
Low. The native change is a single mechanical line that copies the established idiom verbatim from sibling functions. There is no new control flow, no API surface change, and no ambiguity about the correct return value (undefined is what the function already returns when SSL_get_servername yields null). The test is straightforward and the coderabbit feedback (reject on error) was addressed in 28522bd and marked resolved.
Other factors
The two CI failures reported by robobun (test/bake/dev-and-prod.test.ts on Windows aarch64 and test/js/web/fetch/fetch-http2-client.test.ts on x64-asan) are in unrelated subsystems and not plausibly caused by adding a null check to a TLS getter — they look like pre-existing flakes. No outstanding reviewer comments remain.
|
CI triage (build 50529): all three failures are pre-existing flakes affecting other branches, unrelated to this one-line null guard.
|
2 participants
Repro
onClosedetaches the socket (this.socket.detach()) before invoking the JSclosehandler, sothis.socket.ssl()returnsnullthere.getServernamepassed that null straight toSSL_get_servername, which dereferencesssl->hostname→ segfault at offset 0x90.Cause
Every other TLS getter in
tls_socket_functions.zig(getCipher,getTLSVersion,getCertificate,getSession,getTLSTicket, …) guards withthis.socket.ssl() orelse return .js_undefined.getServernamewas the one that didn't.Fix
Verification
Without fix (
bun bd, ASAN):With fix: test passes,
getServername()returnsundefinedafter close.