[Jarred-Sumner]

What

Same programme as #40055 … #40235. Stacked on #40139 (base branch claude/socket-zero-unsafe; retarget to main once that lands). Takes the rest of src/runtime/socket/ to zero: Listener.rs 37 → 0, tls_socket_functions.rs 36 → 0, WindowsNamedPipe.rs 36 → 0, WindowsNamedPipeContext.rs 31 → 0, udp_socket.rs 30 → 0, SSLConfig.rs 11 → 0, Handlers.rs 2 → 0, SocketAddress.rs 15 → 1 / uws_jsc.rs 5 → 1 / JSSocketHandlers.rs 1 (each residual is an all-safe fn unsafe extern "C" block). uws_dispatch.rs/uws_handlers.rs (the C→Rust vtable dispatchers) keep theirs.

• Listener is CellRefCounted (listener: JsCell<ListenerType>, connection: JsCell<UnixOrHost>, ListenerType::NamedPipe(OwnedThis<..>)); Handlers::{set_listener(&Listener), clear_listener(), listener() -> Option<BackRef<Listener>>}; connect_inner(.., Option<ThisPtr<TCPSocket>>, Option<ThisPtr<TLSSocket>>, ..); Drop closes a still-set transport. SNI goes through bun_uws_sys::{SniDecision, ServerNameHandler, ListenSocket::on_server_name::<H>()}.
• UDPSocket is CellRefCounted and implements bun_uws_sys::udp::UdpHandler (udp.rs is the same file as node:quic: remove unsafe from the endpoint, session, stream and TLS code #40220, plus create_from_fd / bind_udp_fd).
• TLS extras: new bun_boringssl_sys::conn (safe SSL/SSL_SESSION/SSL_CIPHER/EVP_PKEY/X509/X509_STORE accessors, OwnedSslSession, OwnedX509Store, X509StoreCtx, export_keying_material, get_finished, …) and ctx.rs (same file as node:quic: remove unsafe from the endpoint, session, stream and TLS code #40220); bun_uws_sys::ssl (ssl_new_session, ssl_set_inline_reject, shared_default_ca_store, SslLoopState RAII); x509::to_js(&X509) / to_js_object(OwnedX509).
• Windows named pipes: bun_libuv_sys::Pipe::{listen_named_pipe_with<H: ConnectionHandler>, connect_with<H: ConnectHandler>, close_and_free(Box), close_owner<T: PipeOwner>(OwnedThis<T>)}, UvStream::read_start_owned<T: StreamOwner>(BackRef<T>), open_handles::{HandleOwner, set_owner_ref}; WindowsNamedPipeContext::{create -> ThisPtr, on_*/run_event/release_unrun/stop_for_vm_teardown(ThisPtr<Self>)}; impl_streaming_writer_parent! borrow = this (same hunk as FileSink, Bun.write: remove unsafe from FileSink.rs and blob/write_file.rs #40213).
• Also: bun_ptr::OwnedThis::{from_raw, into_raw} and derive #[ref_count(release = path)] (zero-count allocation handed to fn(OwnedThis<Self>) for deferred frees); bun_sys::net::{Address::{from_storage, ip, port, scope_id}, sockaddr_inet, getsockopt_int, getsockname, listen, if_indextoname, if_nametoindex}; bun_cares_sys::{ip_to_text, pton}; bun_jsc::OwnedURL; bun_core::SensitiveCStr (dupe + zero-on-drop); Bun__parseIpAddress, Bun__WebSocket__{parse,free}SSLConfig, us_socket_buffered_js_write are HOST_EXPORTs.

Pre-existing bugs fixed in passing: getEphemeralKeyInfo leaked an EC_KEY (EVP_PKEY_get1_EC_KEY → get0); handle_path allocated a copy only to zero-free it; double unlink in do_stop. Noted, unchanged: the listener-level SNI path skips the JS callback if the ClientHello socket carries no TLSSocket (unreachable in practice); a nested writer.with_mut re-entrancy on WindowsNamedPipe write/flush error paths is carried over as-is.

Testing

Debug+ASAN: bun/net (9 files), node/net (12), node/tls (26), bun/udp (3), node/dgram, websocket.test.js, node-http.test.ts, socketaddress.spec, blocklist-gc — pass (except the debug-only node-net "reused handle" 60 s cap). Node parallel: test-net-* 139/139, test-tls-* 185/186 (the one uses node:test describe), test-dgram-* 75/75. Hand-driven: echo server + 500 clients, TLS listen + tls.connect with checkServerIdentity, TLSSocket extras + SNICallback, upgradeTLS, unix listen/connect (+ unlink on stop), stop(true) with live sockets + GC, reload, udp send/recv/close, Worker with listener + udp terminated — no ASAN output; Listener/UDPSocket wrappers freed after GC. clippy clean on all touched crates; rust-check-all windows-msvc + apple-darwin pass (named-pipe code cross-compiled and desk-reviewed only).

[robobun]

^{Updated 5:20 PM PT - Sep 6th, 2026}

❌ @Jarred-Sumner, your commit 7b7f80c has 1 failures in Build #111813 (All Failures):

• test/js/node/test/parallel/test-crypto-dh-leak.js - code 1 on 🐧 13 x64-asan

---

🧪   To try this PR locally:

bunx bun-pr 40236

That installs a local version of the PR into your bun-40236 executable, so you can run:

bun-40236 --bun

[claude]

Thanks — commit 977eaa3 addresses all three findings from the earlier pass (the pton() interior-NUL panic, the orphaned bound_ip_into, and the shared_sigalgs_count doc). This run found no further issues. Given the scope — ownership/lifetime rework across Listener/UDPSocket/WindowsNamedPipeContext plus new BoringSSL/uSockets safe-wrapper surface and the SNI/cert-chain paths — a human look is still worthwhile before merge.

Also re-checked this pass: the new UdpSocket<U> handle's ref accounting (poll-error close vs. owner close vs. Drop) balances on every path; Listener::Drop unlinks the group and clears the handlers back-ref before storage is released; resolve_server_name preserves the listener-vs-socket this semantics of the two callbacks it replaced.

Extended reasoning...

Overview

This PR completes the zero-unsafe programme for src/runtime/socket/: ~44 files, ~400KB diff. It converts Listener and UDPSocket to CellRefCounted, reworks WindowsNamedPipeContext around OwnedThis/deferred-free with a new borrow = this streaming-writer parent mode, replaces the two extern "C" SNI dispatchers with a typed ServerNameHandler/SniDecision trait, moves the entire TLSSocket-extras BoringSSL FFI surface into new boringssl_sys::{conn,ctx} safe wrappers, and adds a typed UdpSocket<U: UdpHandler> handle in uws_sys::udp. It also adds bun_sys::net accessors, bun_cares_sys::{pton,ip_to_text}, SensitiveCStr, OwnedURL, and a #[ref_count(release = ...)] derive variant. Stacked on #40139.

Security risks

The TLS surface is extensive: SNI resolution, peer-certificate chain walking through the trust store, session (de)serialization, keying-material export, and set_key_cert_from (retargeting a handshake's identity). These are refactors of existing behavior into safe wrappers rather than new logic, and the bug hunter found no regressions, but the blast radius of a mistake here (wrong cert served, wrong store consulted, ref leak on an SSL_SESSION) is high enough that human review of boringssl_sys/conn.rs and the get_peer_certificate rewrite is warranted.

Level of scrutiny

High. This is a large ownership-model rewrite across intrusive-refcount types with GC-visible wrappers, libuv handle lifetimes, and BoringSSL reference counting — exactly the category REVIEW.md's memory-safety section flags as most-blocked. The Windows named-pipe path was only cross-compiled and desk-reviewed per the PR description.

Other factors

All three findings from the previous automated pass were addressed in 977eaa3: pton() now uses CStr::from_bytes_until_nul (with a new socketaddress.spec.ts case), bound_ip_into is wired into both UDPSocket::get_address and quic/endpoint.rs, and the shared_sigalgs_count doc is corrected. This run's bug hunter found nothing new. The PR description documents extensive local ASAN testing across net/tls/dgram suites and Node parallel tests. Still, the combination of scope, stacked-PR base, TLS-sensitive paths, and Windows code that was not run locally puts this outside what should merge on automated review alone.

[claude]

Automated review is now clean — all four earlier findings (interior-NUL in pton, orphaned bound_ip_into, shared_sigalgs_count doc, and the src/CLAUDE.md borrow = this/ptr wording) are addressed and nothing new turned up on this pass. Given the scope (45 files, Listener/UDPSocket/WindowsNamedPipeContext refcounting rewrites, ~1100 lines of new BoringSSL/libuv safe wrappers, SNI dispatch rework), a human look is still warranted.

What was reviewed:

• Listener → CellRefCounted + Drop: back-pointer cleared in both finalize and Drop, group unlink/LSAN-unregister paired with init_group, error-path cleanup guard releases the ref.
• UdpSocket<U> owner-ref lifecycle: close/closed/Drop each release exactly once; on_close handles the uSockets-initiated case via socket.closed().
• WindowsNamedPipeContext borrow = this + schedule_deinit/self_own: conn_ref/connect_ref released on every terminal path; close_owner guards already-closing pipes.
• tls_socket_functions rewrite over boringssl_sys::conn: get1→get0 EC_KEY leak fix, X509StoreCtx RAII, set_key_cert_from matches the old inline sequence.

Extended reasoning...

Overview

This PR completes the "remove unsafe from src/runtime/socket" programme (stacked on #40139). It converts Listener, UDPSocket, and WindowsNamedPipeContext to CellRefCounted with typed ThisPtr/BackRef/OwnedThis dispatch; introduces ~1100 lines of safe BoringSSL wrappers (conn.rs, ctx.rs) plus uws_sys::ssl, a trait-based UdpHandler/UdpSocket<U> owner handle, libuv ConnectionHandler/ConnectHandler/StreamOwner/PipeOwner traits with close_owner, a new impl_streaming_writer_parent! borrow = this mode, and ServerNameHandler/SniDecision for SNI dispatch. It also adds bun_sys::net::{sockaddr_inet, getsockname, getsockopt_int, listen, if_*}, SensitiveCStr, OwnedURL, #[ref_count(release = ..)], and converts three extern "C" exports to HOST_EXPORT. 45 files changed.

Security risks

The PR directly rewrites TLS certificate/key/chain handling (set_key_cert_from, get_peer_certificate trust-store walk, X509StoreCtx), SNI context selection, ALPN preference storage (ex-data slot with a free hook), and SensitiveCStr zero-on-drop for key/cert material. These are security-sensitive by definition. The refactor is meant to be behaviour-preserving, and the review checked that the new set_key_cert_from matches the previous inline sequence, that X509_STORE_CTX_get1_issuer's +1 refs are held in extras: Vec<OwnedX509> past the loop, and that the shared-default-CA-store fallback still up-refs/frees. Nothing looked wrong, but this class of code merits human review.

Level of scrutiny

High. This is a large-scale memory-safety refactor of listener/socket/named-pipe lifecycle across FFI boundaries — exactly the category REVIEW.md flags as most-blocked. It rewires refcounting (Listener now CellRefCounted with a Drop that unlinks the group; UDPSocket gains a second ref held by the uSockets user slot; WindowsNamedPipeContext moves to OwnedThis-based deferred free via self_own), changes teardown ordering, and introduces new libuv callback trampolines that stash BackRefs in handle.data. Each of these has non-local invariants that are easy to get subtly wrong.

Other factors

• All four findings from the three previous automated review passes have been addressed and the threads resolved.
• The PR description reports extensive debug+ASAN test coverage (bun/net, node/net, node/tls, node/dgram, Node parallel test-net-/test-tls-/test-dgram-*) plus hand-driven scenarios, and rust-check-all cross-compiles for Windows/macOS.
• The Windows named-pipe path is cross-compiled and desk-reviewed only (per the description); it carries the deepest lifecycle changes.
• Robobun shows 4 unrelated bun-install/bunx test failures on Windows/aarch64 — none in the socket/net/tls suites this PR touches.
• The PR notes a known carried-over writer.with_mut re-entrancy on WindowsNamedPipe error paths and a listener-SNI edge case as deliberately unchanged.

Given the scope, the TLS/crypto surface, and the depth of the lifecycle rewrites, this should not be auto-approved.

[claude]

I reviewed this PR again after the fixes and the automated pass found no new issues — the interior-NUL, bound_ip_into, sigalgs doc, and src/CLAUDE.md borrow = this items are all resolved. Given the scope (43 files, ~200 unsafe blocks removed across Listener/UDP/TLS-extras/named-pipe lifecycle and refcounting, new BoringSSL/libuv wrapper crates, Windows paths desk-reviewed only per the description), a human pass would still be worthwhile before merge.

Checked in this round: the new Listener Drop/finalize split vs the old deinit teardown order (error-path deref() reaches Drop with empty strong_data/poll_ref/this_value, so the reduced Drop is sufficient there); SocketGroup::unlink vs destroy after close_all; the UdpSocket<U> owner-ref release on the poll-error on_close path (closed() covers it); and the SNI resolve_server_name merge preserving the listener/socket this distinction.

Extended reasoning...

Overview

This PR is the final tranche of the src/runtime/socket/ zero-unsafe programme, stacked on #40139. It rewrites Listener, UDPSocket, WindowsNamedPipe/WindowsNamedPipeContext, tls_socket_functions, SSLConfig, SocketAddress, and Handlers around CellRefCounted/ThisPtr/OwnedThis/BackRef, and adds substantial safe-wrapper surface to bun_boringssl_sys (conn.rs, ctx.rs), bun_uws_sys (ssl.rs, udp::UdpSocket<U>/UdpHandler, SniDecision/ServerNameHandler), bun_libuv_sys (ConnectionHandler/ConnectHandler/StreamOwner/PipeOwner, close_owner), bun_sys::net (sockaddr_inet, getsockname/getsockopt_int/listen/if_*), and bun_ptr (OwnedThis::{from_raw,into_raw}, derive #[ref_count(release = ...)]). 43 files, roughly 400 KB of diff.

Security risks

The PR touches TLS trust paths directly: SSL::set_key_cert_from, the get_peer_certificate(true) chain walk through X509StoreCtx/shared_default_ca_store, SNI resolution (SniDecision/ServerNameHandler), and SSLConfig cert/key loading. All of these are refactors of existing logic into safe wrappers rather than behavioural changes, and the automated pass did not surface a regression, but this is exactly the category REVIEW.md flags for careful human eyes ("never remove a flag you don't understand in a TLS/crypto path", fail-closed checks). The getEphemeralKeyInfo get1→get0 fix and the handle_path free-then-nothing fix are genuine leak/dead-code fixes noted in the description.

Level of scrutiny

High. This is a large memory-safety refactor of production networking code: Listener/UDP refcounting and Drop ordering, Windows named-pipe deferred-free (schedule_deinit → OwnedThis in self_own), impl_streaming_writer_parent! borrow = this mode, and libuv close_owner ownership transfer are all the kind of change where a subtle ordering mistake is a UAF. The PR description states the Windows named-pipe code was cross-compiled and desk-reviewed only, and explicitly notes a carried-over nested writer.with_mut re-entrancy on the write/flush error path.

Other factors

All four issues raised in earlier automated passes (interior-NUL as_cstr panic, orphan bound_ip_into, shared_sigalgs_count doc, src/CLAUDE.md borrow mode list) have been addressed and the threads resolved. The test suite coverage cited in the description is broad (bun/net, node/net, node/tls, node/dgram, Node parallel test-net-/tls-/dgram-*), and a socketaddress.spec test was added for the interior-NUL case. No open reviewer threads remain. Given the size, the TLS surface, and the Windows-untested caveat, deferring to a human reviewer rather than auto-approving.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

[claude]

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.