Repository navigation
Conversation
Test-only. Tightens the syscall-fault assertions, covers terminate() sending a bare RST, stop(true) from a selection callback, the redirect-hop TLS identity and the tunnel SNI; runs the cluster setSecureContext test in one process, the handshake-queue fixtures over a unix socket on macOS, and fewer destroySoon connections on debug/ASAN builds. The custom-lookup test passes agent: false so a reused keep-alive socket cannot skip the lookup it asserts on.
BoringSSL's SSL_set_session abort()s when the handshake has started, so any late socket.setSession(validSession) killed the process (exit 134) on node:tls clients and servers, TLS over a Duplex, Bun.connect, Bun.listen and both upgradeTLS halves. set_session now returns undefined, as Node does, for a server SSL or once the client random is filled: do_start_connect fills it right before it moves the handshake off its initial state and nothing clears it again, so it also covers a finished handshake and a renegotiation. The JS guard for standalone wraps is subsumed and removed.
BoringSSL's SSL_CTX_set_timeout only covers TLS <= 1.2; TLS 1.3 reads session_psk_dhe_timeout, so a server with sessionTimeout: 7 issued tickets valid for two days. Set both.
…32824) getCipher().version was BoringSSL's constant "TLSv1/SSLv3". It is now the cipher's minimum protocol version, with OpenSSL's strings. getEphemeralKeyInfo() read SSL_get_privatekey, the client's own long-term key: {} without a client certificate, the certificate's key with one, and an EC_KEY reference leaked per call. It now reports the negotiated group, on TLS 1.3 as well (Node does, for the groups it can name), and nothing for a resumed TLS <= 1.2 session, which has no key exchange.
…N_CONFLICT (#43594) secureProtocol together with minVersion/maxVersion was accepted and secureProtocol silently won; Node throws ERR_TLS_PROTOCOL_VERSION_CONFLICT from createSecureContext, tls.Server, https.Server and tls.connect. tls.Server and https.Server treat a falsy value as absent, as Node does. ERR_TLS_INVALID_PROTOCOL_VERSION formats the value with %j, ERR_HTTP_SOCKET_ASSIGNED and ERR_IPC_CHANNEL_CLOSED use Node's messages, and ChildProcess#send() on a closed channel reports ERR_IPC_CHANNEL_CLOSED.
… them (#39997) createSecureContext({ privateKeyEngine, privateKeyIdentifier }) returned a context with no key. BoringSSL has no ENGINE support, so throw ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED as clientCertEngine already does, with Node's checks in Node's order. The checks move into validateSecureContextOptions so tls.Server runs them at construction.
…he class (#41696) getCiphers() returned the DEFAULT_CIPHERS policy string split on ":", upper-case and with "!aNULL"-style tokens. It now returns the names a context accepts, lower-cased and sorted, as documented. tls.SecureContext had no prototype, so `x instanceof tls.SecureContext` threw. It now shares the prototype of the objects createSecureContext() returns.
Node defines the accessor with configurable: false; Bun's could be redefined or deleted.
…loaded (#42991) node:tls predicted the failure with its own fs.accessSync probe and printed Node's line; the native loader then printed a second `warn:` line. The probe had per-VM state (each Worker warned again), read process.env while the loader reads the C environment, and said nothing for a file that exists but does not parse. fetch and Bun.connect only got the native wording, buffered until exit. The once-per-process native load is now the only emitter: it uses Node's wording and flushes, and node:tls triggers it at the first secure context.
…2996) On Linux the default store and the system store each opened $SSL_CERT_FILE. A pipe yields its bytes once, so the second loader got nothing (its CAs were missing from one store) and a FIFO blocked forever in the second open(2): `SSL_CERT_FILE=<fifo> bun --use-system-ca` hung where Node connects. One shared reader now serves both. It keeps the bytes only when the file is not regular, and holds its lock across open(2) so two threads cannot split one FIFO's contents.
Both add trust anchors to every connection that uses the default CA store and neither was documented.
root_certs.der is a binary blob, so a swapped root does not show in review and passed every test: they compare the export with the blob and the count with certdata.txt. Compare each bundled root's SHA-1 with the entries certdata.txt trusts for server auth.
BoringSSL's legacy certificate slot holds one identity, so a context built from key/cert (or pfx) arrays only kept the last pair: with [rsa, ec] an RSA-only client failed its handshake, and arrays in different orders or of different lengths threw KEY_TYPE_MISMATCH where Node accepts them. Each identity is still loaded into the legacy slot with the two existing loaders, then copied into BoringSSL's credential list: one identity per key type, the last certificate of a type wins, a key or certificate without a partner is ignored, and ECDSA/EdDSA are preferred to RSA. A single key and certificate take the old path. The JS pre-check that predicted KEY_TYPE_MISMATCH is gone. us_ssl_ctx_build_raw also builds the contexts of Bun.serve, Bun.listen and Bun.connect, so two pairs of one key type keep serving the last pair, as before, where Node throws KEY_VALUES_MISMATCH: a key of the right type that does not fit is skipped. If no key fits the certificate and at least one of its type did not, the context still fails with KEY_VALUES_MISMATCH, and a key that does not parse or decrypt stays fatal. Known cost: `cert: [c1], key: [k1, foreignKeyOfTheSameType]` is now accepted, where Node and earlier versions of Bun refuse it. test-tls-multi-pfx.js is upstream's again, and test-tls-multi-key.js skips only its two PKCS#12-chain cases.
ALPNCallback runs inside SSL_do_handshake. Over a Duplex, destroy() there
reached SSLWrapper::deinit(), which freed the SSL below BoringSSL's frames:
heap-use-after-free on TLS 1.2 and 1.3, through server.emit("connection", duplex)
and new TLSSocket(duplex, { isServer: true }).
deinit() still takes the SSL out of the wrapper at once, so every guard sees a
closed wrapper. Inside a handle_traffic pass the outermost pass frees it once it
has returned. The handshake is only driven from a pass.
… Duplex (#43529) tls.connect({ socket: duplex }) is a native socket to the session, but each write to it runs the Duplex's _write. A frame issued there (ping(), settings(), request()) landed inside the outer frame's payload or between HEADERS and CONTINUATION, and a flush issued there sent the connection preface again. write() sends every transport whose write runs JS through write_to_js_transport, which only writes at a frame boundary. generic_flush and generic_write take the pending bytes out of write_buffer before such a write.
…42332) pause_stream/resume_stream were a TODO for a TLS socket over a Duplex, so readStop() did nothing: a paused reader of 1 MiB held all of it and the transport never paused. They now call pause()/resume() on the wrapped stream, like node's JSStreamSocket. A paused transport holds its 'end' event back, so the check that drops a close_notify answer after the transport's EOF reads _readableState.ended in place of a flag set by 'end'. The close resumes the transport: left paused, a net.Socket transport never reads its peer's FIN.
https.Server was http.Server, which decides between TLS and cleartext by
looking for key/cert/ca/pfx in its options. https.createServer() with none of
them (or with undefined values) therefore answered plain HTTP on a port the
caller believes is encrypted.
https.Server is now its own class and marks its prototype, so the listener is
TLS whatever the options hold: with no certificate every handshake fails, as in
Node, and a cleartext request gets no response. The ALPN defaults move from
createServer() into the constructor so new https.Server() gets them too.
http.createServer({ key, cert }) keeps serving TLS.
…CipherOrder to the listener (#41641) The https server copies its TLS options into the native config through a hand-written list that lacked these keys, so they were dropped without an error. The worst case is crl: an mTLS server served a revoked client certificate with socket.authorized === true. Forward them, default honorCipherOrder to true and ecdhCurve to tls.DEFAULT_ECDH_CURVE like tls.Server, and run the validation tls.createServer runs. validateSecureContextOptions moves verbatim from node:tls to internal/tls so both modules share one copy.
The native listener bypasses net.Server's accept path, so a blockList given to https.createServer() or assigned to server.blockList was never consulted. Check it in onServerConnection next to the maxConnections gate and close a listed peer without an event, as Node does. A check() that throws closes the connection too: the native listener serves any socket that is still open when the callback returns. https.Server validates and stores the option whether or not it has a certificate; http.Server ignores it like Node.
Agent#getName() is the key of the socket pool and of the TLS session cache. It
concatenates the options, so any value that stringifies to "[object ...]"
(pfx: [{ buf, passphrase }], an ArrayBuffer, a BunFile) gave every request the
same name, and certFile/keyFile/caFile, allowPartialTrustChain and secureContext
were not part of it at all. A request with one client certificate or CA was then
sent on the connection, or resumed the session, that another one authenticated.
Key such values by their bytes, or by identity when there are none to read, port
Node's getPfxAgentKey() (nodejs/node 9f03017f38), and append the missing options
only when they are set so every name Node produces stays the same.
…unnel (#44346) establishTunnel() calls tls.connect() from the proxy socket's 'readable' handler, after https.request() has returned. An option it rejects (minVersion, a wrong passphrase) was thrown with nothing to catch it: an uncaught exception, no 'error' or 'close' on the request, and the CONNECT socket left open. Catch it, close the proxy socket and report it through the same callback as the other tunnel failures. Port of nodejs/node#66096.
The ws and node-fetch shims each need to turn the options Node code gives an
https.Agent or tls.connect() into the `tls` option of WebSocket and fetch().
nodeClientTlsToNative() in internal/tls does it once: a fixed list of keys read
from an own-property copy, Node's validation, rejectUnauthorized off only for
`false`, allowPartialTrustChain on only for `true`, version strings to numbers,
{ pem } keys and pfx archives to PEM.
The native `ca` replaces the default trust store, so the CAs an archive bundles
extend only a `ca` the caller gave and are dropped otherwise.
normalizePemKeyOption moves verbatim from node:tls, and https.createServer()
now uses it: key: [{ pem }] failed to listen with ERR_INVALID_ARG_TYPE.
The shim passes init straight to fetch(), which has no `agent` option, so the ca, cert, key and rejectUnauthorized of an https.Agent were dropped. That is how @kubernetes/client-node configures mTLS and a private CA. Translate agent.options into init.tls unless the caller gave one.
…, #42325) npm ws gives every constructor option to https.request(), so ca, rejectUnauthorized, cert, key, pfx or minVersion are written at the top level or on the agent. The shim read TLS options only from the Bun-only `tls` option and five keys of the agent, and dropped the rest without an error: a private CA was not trusted, a client certificate in a pfx was not sent, a minVersion was not enforced. Merge the top-level options with the agent's the way http.Agent does, so the agent wins (measured on Node v26.3.0 with ws 8.18.3), and translate the result with nodeClientTlsToNative(). An explicit `tls` option still replaces both, and its pfx is unsealed too.
…he client handshake (#41648) `new WebSocket(url, { tls })` took the SNI and the verified name from the URL only and never read `tls.checkServerIdentity`, so a pin that returns an Error still opened the connection. `serverName` now sets the SNI and the verified name, also inside a proxy tunnel; an HTTPS proxy keeps its own name. The callback replaces the built-in name check for the target. `CppWebSocket` gives the one verdict for every handshake (direct, proxy hop, tunnel, renegotiation). The callback lives in a slot of the WebSocket that its JS wrapper marks, so a callback that captures the WebSocket is still collected with it. A call that `JSValue::call` dropped (stopping VM, disposed graph, retired `--isolate` realm) is not an approval: `call_may_have_been_dropped` sits next to `call` and shares its conditions.
…it (#37487, #43048) Through a CONNECT proxy the socket stays with the upgrade client after the upgrade, and the connected client closed only its own detached handle. So terminate(), a protocol error and teardown left the proxy connection, the tunnel and its SSL_CTX behind, and the upgrade client's keep-alive kept the process running. The three abrupt paths now share `drop_connection`, which closes the socket through the tunnel, and the upgrade client drops its keep-alive once the connected client holds one. After a clean close() nothing bounded the wait for the server's TCP close on TLS and in a tunnel, so a server that keeps the connection open pinned the fd. The client now arms a 30 s socket timeout like `ws` (BUN_CONFIG_WS_CLOSE_TIMEOUT) and ends the connection with close_notify and a FIN. Frames that arrive after the close event are not parsed, so the server's Close frame no longer gets a second one back.
addContext() wrote either the live listener or the contexts map, never both, so an entry added while listening was lost after close() + listen(), and a replaced one came back stale. It now always records the entry, in call order. A falsy servername throws Node's ERR_TLS_REQUIRED_SERVER_NAME.
…42050) A server that is not listening only stashed the raw fields, so key or certificate material BoringSSL rejects surfaced later, out of listen() or the first injected connection. setSecureContext() now builds the shared context from the staged fields before it assigns anything, like Node's _sharedCreds, so the call (and tls.createServer / http2.createSecureServer) throws and the previous credentials stay. The lazy getter, its try/catch in the 'connection' listener and the JS KEY_TYPE_MISMATCH pre-check go away.
…P/3 on the same tree (#37195, #43849) Server names were compared in three places with three sets of rules: the per-listener tree (byte-wise, first registration wins), the HTTP/3 flat array (strcmp, first wins, '*.' covering any number of labels) and App.h's pending list. The entry a name selects carries the certificate and that entry's requestCert/ca policy, so ADMIN.EXAMPLE.COM skipped admin.example.com's policy, and a later duplicate entry's policy was dropped silently. All name semantics now live in sni_tree.cpp: labels compare ASCII case-insensitively and sni_add replaces, handing back the displaced entry. quic.c keeps its contexts in the same tree, so HTTP/3 matches exactly like TCP. With add replacing, nothing removes a name any more: sni_remove, its C, C++ and Rust wrappers and App.h's duplicate rollback go away. domain() picks the last registration, which is the one the tree points at. Drops the LSAN suppression for uws_add_server_name_with_options (stale since #29932), so LSAN now watches the duplicate-entry path.
… parameter The helpers us_internal_ssl_on_data() calls return NULL for a socket that is gone, and the parameter they were assigned to is _Nonnull in release builds (the annotation is compiled out under BUN_DEBUG), so the release ASAN build aborted on -fsanitize=nullability-assign in every handshake that closed its socket.
…t of two generic functions
Whether a peer reads a FIN ahead of the reset behind it, whether unread bytes survive a reset (Windows drops them), how much a loopback buffer holds, and fstat() on a socket handle. Each now says so per platform. The Node.js run of node-tls-wrapped-socket-close fails a hanging test by name instead of running out of time.
The listener took its policy from the options alone. A context created with requestCert and rejectUnauthorized served a client with an untrusted certificate when the options did not repeat them: the certificate was asked for and nothing acted on the verdict. Accepted sockets now also reject when the context in use does, which covers setSecureContext() too. requestCert in the options over a context that does not ask closed every connection. It throws at listen() now: the context is served as it is, so clients would be checked against its store and not the ca next to requestCert.
…stalled socket too While a connection on the loop holds the rest of a batch the kernel refused, application writes leave record by record. The write BIO then flushed a held flight and sent the record behind it with a send() of its own, which is #40653 again: a server that tears down right after the handshake closes over the unread second segment and the client reads ECONNRESET in place of 'end'. The record now joins the flight and both leave at once, so a spill still takes no more than a flight and a record. main has the same split. stalledConnection() left its accepted socket open, so what its client could not send waited on the loop into the next test, which is the one for #40653.
The relay of node-tls-wrapped-socket-close acted on its end of a connection before the 'connection' event had delivered it (Node.js on Windows). The proxy tunnel memory test gave up after 100 requests, which the release ASAN build makes before a collected body is back with the system: it polls to a deadline now.
…ert, from more entry points
An http2 server fed through emit("connection"), a server wrap made by
new TLSSocket(duplex, { isServer }) and by tls.Server.emit("connection", duplex),
a client wrap, a transport that destroys the socket from inside the write of the
alert, fetch and WebSocket through a CONNECT proxy, and a client over a Windows
named pipe. Offered in the review of this pull request.
…ak test Up to 3 of the latest native TLS sockets stay reachable for a while, however many were made: never more than 3 over 1,200 sockets in a fresh process, on main as well. The test allowed 2 of 20, and failed 3 of 5 attempts on the Linux x64 lanes with 3. It now allows 5 of 40. Without the pending_close that it guards, all 40 stay.
…light A TLS socket over a JS stream holds one write in flight and parks the next in node:net. SSLWrapper answered the peer's close_notify at once, so the socket closed under them: write() with no callback, racing the peer's end(), failed with ERR_SOCKET_CLOSED and an 'error' nobody listens for. Node and the fd engine deliver those writes. Handlers::write_in_flight lets the wrapper ask its owner whether the transport still has a write. While it has one, the answer to the peer's close_notify waits. UpgradedDuplex calls answer_peer_close() once its stream completed the write and the socket wrote what it held. Bytes the peer sends behind its close_notify are dropped. An end() that waits for a flush also waits for the stream: the 403 that an http2 server sends a client without ALPN was cut off when the stream completed its writes a turn later.
Collaborator
Author
|
Status Reproduced on #44618 at d42b055 (release build). TLS over an in-memory Duplex pair, where the client's stream completes a write 10 ms later: client.write("x"); // no callback, no 'error' listener
server.end(); // the peer closes in the same tickThe process exits 1 with The twelve new tests fail on d42b055 and pass on this branch: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Beyond the inline findings, I also checked the re-entry in on_write_done (src/runtime/socket/UpgradedDuplex.rs:850-856): on_writable runs JS that can tear the duplex down, and the host_fn::get_function_data(function).is_some() re-check before touching the wrapper does gate that path, so a teardown inside the drain callback does not reach freed state.
Extended reasoning...
The change adds a deferred answer to the peer's TLS close_notify in the Rust SSLWrapper (src/uws/lib.rs) while the transport has a write in flight, wires it through UpgradedDuplex and the node:net end-after-flush predicate, and leaves the two proxy tunnels and the Windows named pipe on the old close-at-once behavior. It touches TLS session shutdown state in native code with no auth or input-parsing surface. Five verified findings are posted inline, so a human look is already signaled; this note only records the one memory-safety concern examined and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
…lose waits The close_notify that waits for a write in flight is the peer's EOF. The wrapper reports the start of the wait through `Handlers::on_peer_close_waits`, and `UpgradedDuplex` passes it on to the socket, which emits 'end' at once, as node does. Before, a socket over a stream that stays open got no 'end' until the stream completed the write. The wrapper asks `write_in_flight` after the data callback. For an owner that can wait, it also sets `received_ssl_shutdown` after that callback. A write that the 'data' handler of the peer's last chunk begins is then a write in flight, and it completes. Before, the socket refused it with ERR_SOCKET_CLOSED. The memory bound of the flood test is wider on debug and sanitizer builds.
Collaborator
Author
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether peer_close_waits can be set and then never consumed through shutdown()'s early return for an already-sent close_notify (src/uws/lib.rs:765): can_wait and the post-dispatch_read check both require !sent_ssl_shutdown, so a set flag always reaches the take() at :838 and the close callback fires.
Extended reasoning...
The second commit moves the write_in_flight decision after dispatch_read and routes the deferred close_notify to on_end via the new on_peer_close_waits handler; the remaining inline findings cover the same-callback completion-then-push race and a test nit. No security-sensitive surface beyond the existing TLS shutdown sequencing is touched.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/runtime/socket/UpgradedDuplex.rs— A TLS-over-Duplex user whose transport completes a write and, in the same callback, pushes the peer's close_notify still getsERR_SOCKET_CLOSED(process exits 1 without an 'error' listener) for a write the transport already delivered. Phase 1 of on_write_done drops in_flight to 0 at src/runtime/socket/UpgradedDuplex.rs:869-870 but the drain that releases the JS write callback only runs in the phase-2 nextTick at :857-859; a close_notify read in that window makes src/uws/lib.rs:1302 see no write in flight and close at once. Fix: treat the one-tick gap as still in flight (decrement in_flight in phase 2, or have write_in_flight also report true while a phase-2 tick is pending) so every write whose JS callback is still parked waits for the close.Why this was flagged
Trigger: tls.connect({ socket: duplex }) where the Duplex's _write completes asynchronously and its completion handler calls the write callback and then synchronously pushes the bytes the peer sent:
cb(); this.push(incoming)whereincomingcarries the peer's close_notify. The write's callback is parked inkwriteCallbackbecause$writereturned false (src/runtime/socket/socket_body.rs:2730-2731 requires transport_idle()). The transport'scb()runs on_write_done phase 1 (src/runtime/socket/UpgradedDuplex.rs:869-877): in_flight becomes 0 and phase 2 is queued with call_next_tick_1. Before that tick, the synchronous push reaches handle_reading, which returns SSL_ERROR_ZERO_RETURN; at src/uws/lib.rs:1301-1302 write_in_flight() is false (in_flight == 0), so the branch calls shutdown(false) at :1320 and trigger_close_callback at :1332. SocketEmitEndNT at src/js/node/net.ts:1030-1034 fails the still-parkedkwriteCallbackwith $ERR_SOCKET_CLOSED even though the transport delivered the bytes. The base branch fails the same write in a wider window; this change narrows the window to the gap between phase 1 and phase 2 but does not close it.Verification: pre-existing (the base d42b055 fails this scenario the same way).
on_write_donephase 1:in_flight→ 0 at src/runtime/socket/UpgradedDuplex.rs:869-870; the drain is only queued viacall_next_tick_1at :875-876. At src/uws/lib.rs:1302write_in_flight()is false, so :1332trigger_close_callback()runs and net.ts:1030-1033pendingWrite($ERR_SOCKET_CLOSED())fires before the nextTick drain releaseskwriteCallback.
…pleted write A stream can complete a write and deliver the peer's close_notify in one callback. `on_write_done` has then counted the write as complete, and the wake that lets the socket continue is still queued. The wrapper saw no write in flight and closed: the socket failed the completed write, and the one it held behind it, with ERR_SOCKET_CLOSED. `UpgradedDuplex` now reports a write in flight until that wake ran (`wake_pending`). The doc comments of the new wrapper fields and functions are one line each. The 403 test rejects its awaited promise when the server creates a session.
Collaborator
Author
|
Pushed 960bb8f and 2c07743 for the review comments.
There are six new tests since ec5eabf. All twelve new tests fail on d42b055 and pass under node v26.3.0. |
Comment on lines
+863
to
+870
| this.wake_pending.set(false); | ||
| if this.in_flight.get() == 0 { | ||
| let peer_close_waits = this.wrapper_ref().is_some_and(|w| w.peer_close_waits()); | ||
| (this.handlers.on_writable)(this.handlers.ctx); | ||
| // That ran JS, which can close this duplex: `teardown` clears the function data. | ||
| if peer_close_waits && host_fn::get_function_data(function).is_some() { | ||
| if let Some(w) = this.wrapper_ref() { | ||
| w.answer_peer_close(); |
Contributor
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing, left by this partial fix: over a transport whose write callback runs synchronously, the second write a 'data' handler issues on the chunk that carries the peer's close_notify still fails ('write EBADF'; exit 1 without an 'error' listener). The wake tick at UpgradedDuplex.rs:870 answers the close_notify before the nextTick'd write callback at net.ts:3028 releases the chunk the Writable buffered behind it, because $write returned true at socket_body.rs:2731. Fix: keep the wait until the stream has handed over what it buffered behind a completed write, e.g. have write_buffered return false while wake_pending so net.ts parks the callback and the drain releases the next chunk before answer_peer_close runs.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
tls.connect({ socket }) or new tls.TLSSocket(duplex) over a Duplex whose write() calls cb synchronously. The peer's last record and its close_notify arrive in one chunk; the 'data' handler calls socket.write(x) then socket.write(y). net.ts _write(x) → $write → the sync cb runs on_write_done at UpgradedDuplex.rs:877-885: in_flight hits 0, wake_pending is set and tick W is queued. write_buffered then returns true because transport_idle() is true (socket_body.rs:2730-2733), so net.ts queues process.nextTick(callback) at net.ts:3028 after W; y stays in the Writable's buffer behind that callback. Tick W runs first: on_write_done clears wake_pending at :863, then answer_peer_close at :870 sees no write in flight and shutdown(false) closes the socket (lib.rs:834-838); socket_body on_close detaches the handle. The stream's callback tick then runs _write(y): readyState is -1 so net.ts:3005-3010 fails y with 'write EBADF'; with no 'error' listener the process exits 1. Base (d42b055) fails both x and y with ERR_SOCKET_CLOSED, so this path is pre-existing.
Verification: pre-existing (the base's immediate SSL_shutdown on ZERO_RETURN loses the queued write by the same route; this PR fixes the class only for transports whose write callback runs asynchronously). Back in socket_body.rs:2730-2733, wrote == total && transport_idle() is TRUE (in_flight is already 0), so net.ts:3021-3028 queues the Writable callback C1 via process.nextTick AFTER W.
Jarred-Sumner
force-pushed
the
claude/tls-consolidated
branch
from
October 8, 2026 03:41
d42b055 to
97ad29f
Compare
Jarred-Sumner
force-pushed
the
claude/tls-consolidated
branch
from
October 8, 2026 03:45
97ad29f to
7819379
Compare
Collaborator
|
Thank you for the report, it was right and it mattered: the branch was green and lost data. This fix does not apply any more, because #43877's mechanism, which it builds on ( |
Jarred-Sumner
added a commit
that referenced
this pull request
Oct 10, 2026
…ps, WebSocket, SQL) (#44618) ### What does this PR do? Consolidates the open TLS pull requests into one. Each was reproduced on `main` and, for `node:*` behavior, on Node v26.3.0 first. About a third are ported as written, the rest are rewritten smaller or merged into one fix where several PRs patched the same cause. One commit per fix, so it can be read commit by commit. Fixes #43520, fixes #31396, fixes #43635, fixes #37193, fixes #43846, fixes #17932, fixes #41061, fixes #36887, fixes #31810, fixes #35240, fixes #32234, fixes #44365, fixes #43807, fixes #42280, fixes #44517. Addresses #41856 (SNI and `servername`; not `checkServerIdentity` for SQL), #24845 (the spin is gone, shown with fault injection on Linux; not run on macOS), #19754 (node-fetch forwards the agent's TLS options; the Kubernetes client itself was not run). #### The ones that matter most | | On `main` | PRs | |---|---|---| | Client certificate disclosure | `https.request()` with a client certificate sends it to a server it then refuses (wrong name, `checkServerIdentity`, `destroy()` in `'secureConnect'`, `terminate()` in `handshake`). A server can force it with a junk record behind its Finished | #43946 | | False `authorized` | Over a Duplex, `secureConnect` with `authorized === true` for a peer that failed the key proof; `secureConnect` for a plaintext peer with `rejectUnauthorized: false` | #44422, #32929 | | Cleartext https | `https.createServer()` without a usable key/cert answers plain HTTP | #41672, #33539 | | Revoked client certificates | An https mTLS server never sees `crl`, so a revoked client is `authorized` | #41641 | | Pooled sockets | Requests with different client certificates or CAs share an `https.Agent` socket and session | #42498 | | Silent plaintext | `tls: [...]` given to `Bun.listen` / `Bun.connect` is plain TCP | #41490 | | Server weakened by a client knob | `NODE_TLS_REJECT_UNAUTHORIZED=0` turns off a server's client-certificate enforcement | #35245 | | Pins never checked | `WebSocket` never calls `tls.checkServerIdentity` and ignores `tls.serverName` | #41648 | | `verify-full` dropped | `PGSSLMODE=verify-*` is lost next to a `TLS_*` URL variable; `tls: true` sends no SNI | #44498 | | Crashes | use-after-free from `destroy()` in `ALPNCallback` over a Duplex; `abort()` on a late `setSession()`; SIGABRT in `fetch` with an https proxy from the environment and a `Bun.file()` body | #44462, #41671, #44458 | | Stream corruption | A TLS `write()` can lose 16 KiB it reported as written while another socket on the loop is stalled | #44529 | | Hangs and spins | 100% CPU on a failing `send()`; a fatal `SSL_write` leaves the socket open forever; `idleTimeout` never sheds a TLS client that ignores `close_notify` | #34510, #38176, #42336 | | Wrong certificate (regression since 1.3.14) | Connections accepted before `stop()` / `close()` get the default certificate and skip their entry's `requestCert` / `ca` | #42355 | | Quadratic Duplex / proxy tunnel | Reading one chunk over a Duplex, CPU: 8 MB 0.88 s → 0.14 s, 16 MB 3.18 s → 0.23 s, 32 MB 11.75 s → 0.39 s; `fetch` upload through CONNECT: 1.7 s → 0.18 s (debug build) | #44464 | #### By area - **fd engine, write path** (`openssl.c`, `socket.c`): #42352, #34510 + #38176 + #42336 as one change, #44529, #44458, #44192. A rejected `send()` ends the write side only and closes at the next writable event unless the peer's bytes are still queued (a 413 sent before a reset is still read). No new per-socket state. Also, on kqueue, **a FIN no longer ends a socket that waits in the low-priority queue** (`loop.c`): with more than 5 TLS handshakes at once, a client that ended right after its handshake could be reset and its server socket report `socket hang up`, because the eof that the sentinel read knote reports was acted on ahead of the unread Finished. That is on `main` too (the macOS entry for `node-tls-server.test.ts` in `test/flaky-tests.txt`: 7 of 48 recent builds of other branches), and this branch made it likelier (6 of 8 builds), since Finished now leaves in one segment with the close_notify. - **Error reporting, both engines**: #44422, #32929, #44516, #37094, #41272 + #42324 + #44223 as one change, #44021, #37472, #43946, #33630. One channel: a fatal error on an established session is reported, then **the engine closes the connection itself**, whatever the owner does with the report. `test/js/bun/net/tls-fatal-error-closes.test.ts` asserts closed-and-nothing-delivered for every owner (node:tls, `Bun.connect`, `Bun.listen`, `fetch` direct and through CONNECT, `Bun.serve`, `WebSocket` direct and through a proxy, Postgres, MySQL, Valkey, Duplex). - **Duplex engine** (`SSLWrapper`, `UpgradedDuplex`): #44462, #43529, #42332, #44464. #43877 + #44394 were in and are **out again**, see "Worth a look" 5. - **node:tls wrap lifecycle** (`net.ts`, `tls.ts`): #38007, #38058, #38028 + #38122 + #38076 as one change (six copies of the attach code become two helpers), #38311, #39008, #38154, #42340 + #42343 + #42339 + #42453 as one change, #43791, #42425, #44085, #42683, #39088, #39040, #40375, and what was still real of #36534. - **SNI, ALPN, server contexts**: #43080, #42050, #37195 + #43849 as one change (**one** SNI matcher for TCP and HTTP/3), #42355, #42285, #33253, part of #37896, part of #37013. A `tls.Server` has one `SSL_CTX`. - **Verification and options**: #44738, #41490, #37005 + the cwd pin of #40984, #31811, #43982, #33483 + #35245, #41810, #32235, #44441, #38092. - **node:tls API and CA store**: #41671, #38145, #32824, #43594, #39997, #41696, #33534, #34748, #42991, #42996, #42970. - **node:https, Agent, `ws`, node-fetch**: #41672 (https half), #41641, #38261, #42498, #44346, #35609, #31397, #42325. - **WebSocket client**: #41648, #37487 + #43048 as one change. - **SQL, Redis**: #33666, #41711, #44498, part of #42054. - **Tests only**: #41426, #40040, #44395, #44016, #37860, #40591, #44440, #41424. Found on the way and fixed here: an upload that a TLS 1.2 server interrupts with a renegotiation never completes on `main` (0 of 32 runs over `https.request`, `fetch`, `node:tls` and `Bun.connect`: the renegotiation ClientHello lands inside an application record that is still unsent, or the socket gets no `drain` again) and completes here, with two tests from robobun; the fix for #40653 (final flight and first write in one segment) stopped working whenever another TLS socket on the loop was stalled, on `main` too; the `tls.Server` prototype pinned the last server constructed and every `SSL_CTX` it owned; `Object.create(process.env).NODE_TLS_REJECT_UNAUTHORIZED = "0"` turned verification off process-wide once a `SHARE_ENV` worker existed; two debug panics when wrapping a shut-down or still-connecting socket; a `fetch` POST through a proxy sent its headers twice when the origin renegotiated; `BlockList` ignored IPv6 zone ids; a test now ties `root_certs.der` to `certdata.txt`. #### Behavior changes - **A server's `ca` without `requestCert` no longer asks for a client certificate** (`Bun.serve`, `Bun.listen`, HTTP/3, node:tls). It matches the docs and Node. On `main` such a server refused clients with no certificate but served any unrelated self-signed one, so it was never authentication. **Set `requestCert: true` to require a certificate.** A matrix test pins that `requestCert: true` still refuses no certificate and an untrusted one on 8 kinds of server, TLS 1.2 and 1.3, with `NODE_TLS_REJECT_UNAUTHORIZED` unset and `0`. - `NODE_TLS_REJECT_UNAUTHORIZED=0` no longer relaxes a server. - `Bun.connect` / `Bun.listen` hear of a fatal TLS error after the handshake through `error(socket, err)`. With no `error` handler the socket just closes. - HTTP/3 server names match like TCP: `*.` covers exactly one label, case is ignored, a trailing dot is ignored, the last registration of a name wins. - `requestCert` on node:https is `=== true`, as in Node. - An array where a generated options dictionary is expected throws (`tls: []`, `jest.useFakeTimers([])`). - `key` / `cert` arrays serve every identity. A client that can use both gets ECDSA, where `main` served whichever pair came last. - `ecdhCurve` is forwarded by node:https, `ws` and node-fetch now, so a group BoringSSL lacks (`X448`) throws there as it already does in `tls.createServer`. - A wrapped socket's error is re-emitted on the TLS socket as in Node, so `raw.destroy(err)` with a listener on `raw` only is uncaught, as in Node. - `sql.options.tls` is always an object, never `true`. `RedisClient` sends SNI. - `tls: { secureContext }` alone asks for TLS on `Bun.listen` / `Bun.connect` (it was plain TCP), and a value that is not a `SecureContext` throws. The context is served as it is: the `requestCert` / `rejectUnauthorized` it was created with hold whatever the options next to it say, and `requestCert` in the options over a context that does not ask throws at `listen()`. - `tls.DEFAULT_CIPHERS` reaches every client once assigned (`fetch`, `WebSocket`, `Bun.connect`, `RedisClient`, `Bun.SQL`, `S3Client`, proxy tunnels) and servers again. A list that selects no cipher throws `ERR_SSL_NO_CIPHER_MATCH` at the assignment. `fetch.preconnect()` dials nothing after an assignment. - The warning for an unreadable `NODE_EXTRA_CA_CERTS` is Node's one line, without the `warn:` prefix. - `BUN_CONFIG_WS_CLOSE_TIMEOUT` (default 30 s): how long a `WebSocket` client waits for the server to close the connection after the closing handshake. #### Worth a look in review 1. **#44529**: the kernel-refused remainder of a TLS write moves from the loop's one slot onto the connection (in the existing rare struct), so the write BIO never refuses a sealed record. Nothing is allocated on an unstalled path (200 writes: 0 appends, same `send()` count as `main`), memory with 16 stalled writers is lower than on `main` (276 KB vs 340 KB, which `main` holds inside BoringSSL's buffers), `us_socket_t` stays 80 bytes. It needs a bound on how long a deferred close waits, or a peer that stops reading pins the fd past `destroy()`: `US_SSL_CLOSE_AFTER_SPILL_TIMEOUT` is a fixed 10 s, not re-armed on progress. Separate commits, but the fix that keeps the client certificate off the wire beside a stalled socket builds on them. 2. **The default name check of node:tls also runs inside the handshake**, so a wrong-name server gets no client certificate on TLS 1.2 either. JS still runs it after every successful handshake, so a difference between the two matchers can only refuse. Error objects are byte-identical. 3. **#44441** widens trust by design: a self-issued leaf whose `keyUsage` lacks `keyCertSign` (`dotnet dev-certs`) is its own anchor when the store holds a byte-identical copy. No BoringSSL change. Expired pin, same subject with another key, wrong EKU and a pinned intermediate are tested to fail. 4. **#32235** only adds Ed25519 and ECDSA P-521 to the verify list. A captured ClientHello shows `main`'s list with the two inserted; `rsa_pkcs1_sha1` stays. 5. **A stream that a TLS socket wraps, when that TLS socket closes.** An earlier state of this branch lost data here while CI was green (found by #44709's report): with the peer closing first, 4 of 8 MiB arrived with TLS in TLS, 4 of 32 MiB on the http2 `emit("connection")` path, and a `write()` with no `'error'` listener ended the process. Three Node-parity changes only hold together: destroying the wrapped stream at the close (#38028 + #38122 + #38076, #38154) is safe only if every write has really completed (#43877), which in turn needs Node's handling of the peer's close_notify, which needs half-open sockets that the GC can collect. So: - #43877 + #44394 are reverted and reopened. A write over a stream completes once the stream has taken the ciphertext, as on `main`. - Until the verdict on the peer lets the session through, the application cannot have written over it. There the wrapped stream is destroyed as in Node, with the sessions below it. That keeps the release of the connection after a failed handshake, a rejected certificate and an early `destroy()`. The same for an http2 socket the application never got, and for `resetAndDestroy()`. - After that it is `main`'s teardown: a `net.Socket` only gets the engine's `end()`, closes at its peer's FIN, keeps its own timeout and reports its own errors. Any other stream is destroyed with the TLS socket. The regular suites cannot see any of this (999 files were green on every broken variant), so it was steered by eleven seeded differential fuzzers run on this build, `main`, Node v26.3.0 and the earlier state: close, `end()`, `destroy()`, `destroySoon()`, resets, hung and half-open peers, paused writers, timeouts, two and three sessions deep, over TCP and over Duplexes, before, at and after the handshake, and http2 requests. See "How did you verify". #### Known limits - `fetch` with a `checkServerIdentity` function still sends the client certificate (not the request) to a server the function refuses. On TLS 1.2 any verdict a JS callback gives is too late, as in Node. - `addContext()` / `SNICallback` still do not apply to a server-side socket on the stream engine (`emit("connection", duplex)`, TLS in TLS, unflushed writes, named pipes), as on `main`. - A CA bundled in a pfx extends an explicit `ca` only, for `ws` / node-fetch / `WebSocket`: the native `ca` can only replace the default store, and that store keeps `SSL_CERT_FILE` / `SSL_CERT_DIR`. - P-521 leaves work on TLS 1.3 only. TLS 1.2 needs secp521r1 in every ClientHello (`it.todo`). - Once `tls.DEFAULT_CIPHERS` is assigned, `fetch(url, { protocol: "http3" })` is `HTTP3Unsupported`, as with an explicit `ciphers`. - `addCACert()` by hand does not extend the chains of a context with several identities. - A throwing `ALPNCallback` sends `no_application_protocol` on both engines. Node sends nothing and its client sees `ECONNRESET`. - TLS in TLS, peer FIN while the outer handshake runs: the inner socket gets one `write EPIPE`, where Node gives `ECONNRESET` (`main` gives it no error at all). - `@SECLEVEL` in `ciphers` is dropped by the `ws` / node-fetch shims, which used to ignore `ciphers`. node:tls keeps throwing `ERR_SSL_INVALID_COMMAND`. - Beside a stalled TLS socket only the first record (16 KiB) of the first write leaves with the handshake flight. The rest goes record by record, which is what bounds the memory of stalled writers. - After a fatal error on an established session the socket emits `'error'` and then `'close'`. Node emits `'error'` and leaves the socket open. - A paused reader whose own write the kernel rejects loses what it had not read yet, with an `EPIPE`, as on Node. `main` reports no error there and delivers it. - On `main` too: a `Bun.listen` socket without `allowHalfOpen` that has unsent ciphertext when the client's `shutdown()` arrives loses that ciphertext (32 KiB), and over plain TCP `end()` with the peer still sending is a close over unread input, so a reset. - Differences from both `main` and Node that the differential runs below found and that stay, all with a peer that aborts: `ECONNRESET` instead of a clean `'end'` after the socket's own `'finish'` when the peer destroyed with unread data; under TLS 1.2, a zero-length `write()` followed by `destroy()` in `'secureConnection'` leaves the client without `'secureConnect'` (a plain `destroy()` there matches Node); a TLS 1.2 client that destroys in `'secureConnect'` gets no `'session'`; a `ClientRequest` whose handshake fails with an alert emits `'error'` and `'close'` but no `'finish'` (`writableFinished` is true). - Once `tls.DEFAULT_CIPHERS` is assigned, `fetch.preconnect()` opens nothing: `fetch()` then uses a context of its own, and a socket warmed under the default one would never be picked up. - A TLS `send()` that the kernel refuses outside a `write()` call (the drain of unsent ciphertext) is reported with the close, as `read EPIPE` / `read ECONNRESET`. Node says `write EPIPE`. `main` does not report it at all. - Once the application has a session over a `net.Socket` (TLS in TLS, http2 `emit("connection")`), a peer that never sends its FIN holds that socket after the TLS socket closed, as on `main`. Node destroys it. Two tests of #38154 are `todo` for this. Closing it any earlier (at its `'finish'`, say) makes the kernel drop what it has not sent yet as soon as the peer's close_notify arrives. - Plaintext that was queued on a socket before it was wrapped (STARTTLS with a backlog) is dropped when the TLS socket is destroyed, or its handshake fails, before the session is accepted. Node drops it too, except on `destroySoon()`. `main` sends it. - Over a stream that is no `net.Socket`, `end()` can still cut what that stream has buffered, and there is no backpressure, both as on `main` (#43877). - `tls.secureContext` (the undocumented door node:tls uses) is not read by a Windows named pipe listener, which builds its context from the options. On `upgradeTLS({ isServer: true })` the options next to it are the policy, as with Node's `SetVerifyMode`. - `selectServerName()` rebuilds the name tree per ClientHello for injected sockets of a server with `addContext()` entries: 0.4 µs for 1 entry, 3.7 µs for 10, 41 µs for 100, against 631–1111 µs for a handshake. #### Not included Left open, because they need a decision or are not TLS: #43877 + #44394 (see "Worth a look" 5; #43874 stays open with them), #38548, #38591 (both shrink who is trusted), #41589 (`verify-full` vs `NODE_TLS_REJECT_UNAUTHORIZED=0`), #37197, #41706, #43216, #33487, #33545, #36707, #32435, #37255, #28691, #40275, #30314 (features), #38120 (needs the BoringSSL fork, as did #33517, which the stale bot has closed since), #38529 (needs a Windows measurement), #34342, #38232, #43089, #44454, #40451, #42710, #44527, #38088, #38093, #41898. #37896, #42054 and #37013 stay open for the halves not taken. `http.createServer({ key, cert })` keeps serving TLS on purpose. One open question: `tls: {}` (an object that names no TLS option) is plain TCP on `Bun.listen` / `Bun.connect`, here and on `main`. It is the same trap as `tls: []`, but changing it changes a Bun default, so it is left alone. ### How did you verify your code works? - Every new test fails on `main` for the stated reason and passes here, except guards that pin existing behavior, each shown to fail when its clause is removed. `node:*` tests also pass on Node v26.3.0; the few that cannot say which Node version has the behavior. - 212 test files that touch TLS, sockets, http, http2, fetch, WebSocket, SQL, Valkey and workers: 6154 pass, 2 fail. Both are seen on `main` too: `serve.test.ts` "root range port" (the box runs as root), and `worker_threads.test.ts` "terminate(): nothing of the worker's runs after the request", which is flaky there and passed in the run below. - 58 of those files the way the ASAN lane runs them (LeakSanitizer + `BUN_JSC_validateExceptionChecks`): 58 files, 48 of them with leak checking, 4132 pass, 3 fail. All three also fail on `main`: `serve.test.ts` "root range port", `node-net.test.ts` "should not leak when connect({path}) fails synchronously on a reused handle" (times out under this environment), `worker_threads.test.ts` "process.exit() with a shell cp in flight" (a `ShellCpTask` leak). - 647 vendored `test-tls-*`, `test-https-*`, `test-net-*`, `test-http2-*`: the only two failures also fail on `main`. - The SNI matcher was diffed against both old matchers: 3 seeds × 1.23 M lookups × 3 registration flavours, every difference in one of the intended classes, TCP and HTTP/3 identical on every lookup. - The headline rows were also driven by hand with scripts against this build, `main` and Node v26.3.0: cleartext https, `crl`, `tls: []` / `{ secureContext }`, the `ca` / `requestCert` matrix, the client certificate on a wrong-name server, late `setSession()`, `destroy()` in `ALPNCallback`, `[rsa, ec]` identities with an intermediate from `ca`, `WebSocket` `checkServerIdentity`, a corrupted record, the Duplex read above, `tls.DEFAULT_CIPHERS`. - The `setSession()` guard was checked against the real `abort()` at 43 handshake states. - `bun run rust:check-all`: 12 of 12 targets. `tsc`, oxlint, source lints, prettier, rustfmt, mordant clean. - usockets' `_Nonnull` is compiled out of debug builds, so 105 of those files were also run on a local release ASAN build with the CI runner's environment (92 with leak checking): 4595 pass, 1 fail, `child_process.test.ts` "spawn reports EPERM after dropping privileges", which cannot pass as root and fails on `main` too. - The close of a TLS socket over another stream ("Worth a look" 5): eleven seeded differential fuzzers, 8,424 scenarios compared, each run on a release ASAN build of this branch, on `main`, on Node v26.3.0 and on the earlier state of the branch. Against `main`: - Data that `main` delivers in full is cut in 5 scenarios, and about 150 that `main` cuts arrive in full. Of the 5, in 2 `main` never notices the peer's close and keeps the socket for good, 2 call `end()` on the middle one of three sessions over an in-memory Duplex, and 1 does the same on Node. - No dead timeout, no silent reset and no uncaught error that `main` does not have (4 uncaught errors fewer). - A socket stays open where `main` closes it in 109, and closes where `main` keeps it in 295. 92 of the 109 do the same on Node or on the earlier state (a `destroy()` that an in-memory Duplex does not show its peer, half-open peers). 14 wait for a peer that paused reading and so does not read the FIN (#42332's backpressure, as in Node); the socket's own timeout fires there. 3 are left: one on a 5 ms timer, two with three sessions over an in-memory Duplex. - The earlier state of the branch cut data in 173 of the 400 scenarios of one of them, where `main` cuts none and this cuts none. - 23 new tests pin what they found. Each earlier attempt at this fix fails the ones that describe it, the earlier state of the branch fails 7, and all pass on Node. - After that change: 999 test files on the release ASAN build (20,246 pass; the 11 files that fail need a database, Docker, DNS or a non-root user, or share a temp directory with a parallel run and pass alone), 61 on the debug build. - TLS over a file descriptor (`openssl.c`, the path of `fetch`, `Bun.serve`, `tls.connect`, `Bun.connect`) got the same treatment after the rebase: seeded differential fuzzers on CI's release build of this branch, on `main` and, for `node:*`, on Node v26.3.0. Every runtime also against itself for the noise floor, injected faults and known bugs of `main` as positive controls, and a difference counts only if it shows in 5 of 5 fresh processes. - `node:tls` over TCP: 11,500 scenarios (one connection with Node as the oracle line by line; 2 to 60 connections beside stalled neighbours; raw peers that break the handshake). HTTPS: about 136,000 runs over `Bun.serve` + `fetch`, `node:https`, `node:http2` and `wss://`, also with the two ends in different runtimes. `Bun.connect` / `Bun.listen` / `upgradeTLS`: 11,500 scenarios and 720 slow connections, with writers driven by what `write()` returns, beside up to 6 stalled, dripping, closing or resetting neighbours, and plain TCP as a second oracle. No crash, hang, duplication, reordering or silent truncation, and no change in time or in connection reuse. - They found six things that `main` does better, none of which any test showed. All are fixed, each with a test that fails on the build before: what the peer sent lost behind a rejected `send()` (23 scenarios, and an early HTTPS response lost with only `EPIPE`), the same silently for a paused reader, `server.close()` never calling back on a half-open server after a ClientHello and a reset (17), `closeAllConnections()` taking 12 s with a stalled client, `end()` losing up to 1.3 of 4 MiB that `write()` had reported while the peer still uploads, and `end()` a little after a stall never closing beside other stalled TLS sockets. The last two fixes also deliver the 1 to 2 MiB that `main` loses there, and close the socket that `main` keeps for good without such neighbours. - All of them again after every fix, on CI's release build of it. That caught one regression of a fix itself (a reader stopped for backpressure lost 86,385 bytes, 1 of 6,000 scenarios), fixed too. On the last build: scenarios that lose data where `main` does not 23 → 2, and Node loses it in both, with the same `EPIPE`; `server.close()` that never calls back 17 → 0; connections held 4 → 0; requests that end in an error only where `main` has a response 6 → 0. With a Node server in another process, a request ends in an error only in 8 and 10 of 1,500 scenarios here, 5 and 3 on `main`, 10 with Node as the client. - `Bun.connect` / `Bun.listen` on the last build against `main`, in scenarios: hangs 0 against 1,031, sockets and fds never released 0 against 965, corrupted data 0 against 345, `abort()` 0 against 26 (`setSession()` after the handshake), writers that never close 0 against 101 of 720 connections. No kind of failure shows here and not on `main`. About a third of the slow connections close later than on `main`, in 1 to 16 s instead of at once, waiting for unsent ciphertext or for the peer's close_notify, and 79 more of them deliver all that `write()` reported. RSS and time with 16 to 256 stalled writers are the same. - What they found that `main` does worse: a `WebSocket` that calls `close()` with sends pending loses messages in 81 of 999 scenarios (0 here), 37 server sockets left open, 10 `server.close()` that never call back, 20 write callbacks that never run. - The kqueue fix cannot be run on Linux. The `connectionListener` count test now says what became of a missing connection, which is how the cause was found (`'tlsClientError'` "socket hang up", then `read ECONNRESET` at the client of the same port, after its `'secureConnect'`). On macOS x64 it failed every attempt of the three builds before the fix and passed at the first attempt of the build with it. - Windows and macOS were only run by CI. Four new tests asserted what only the Linux kernel does (a FIN read ahead of a reset, unread bytes surviving a reset, loopback buffer sizes, `fstat()` on a socket) and now say so per platform. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
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
2 participants
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.
Stacked on #44618.
Problem
write()on a TLS socket over a JS stream fails withERR_SOCKET_CLOSEDwhen the peer closes first. With no'error'listener the process exits 1:error: Socket is closed. Node, 1.4.2 and main deliver it.SSLWrapper(src/uws/lib.rs) answers the peer's close_notify at once, so the socket closes under them.can_end_after_flushloses an http2 403 the same way.Fix
Handlers::write_in_flight), the answer to the close_notify waits. A reply written in the data callback counts.'end'at once, as in node.UpgradedDuplexanswers when its stream is idle.node-tls-duplex-end-verify.test.ts,node-http2-upgrade.test.mts) fail on d42b055 and pass under node v26.3.0.end()retries only from the drain dispatch.Background
SSLWrapperis the TLS engine for non-fd transports, such astls.connect({ socket })(UpgradedDuplex).write: all four owners change. The query costs the other three nothing.Downsides
'end', as in node..text+1,280 bytes, both at 960bb8f.Notes
Repro (TLS over an in-memory Duplex pair, the client's stream completes a write 10 ms later):
error: Socket is closed)write(A); write(B)into a stalled Duplex, then the peer's close_notify: node and this branch deliver 2,097,152 B. d42b055 delivers 1,048,576 B and fails both callbacks.'end' at the close_notify. The close_notify is the peer's EOF. When only the close_notify says that the peer closed (the stream itself stays open), node emits
'end'at once and completes the write. The wrapper callsHandlers::on_peer_close_waitswhen the wait begins, andUpgradedDuplexpasses it to the socket as EOF. Only the answer and the close wait for the write.A reply from the 'data' handler. One chunk carries the peer's last data and its close_notify, and the
'data'handler writes. Node delivers that write. d42b055 and 1.4.2 fail it withERR_SOCKET_CLOSED, also over a stream that completes its writes at once:received_ssl_shutdownwas set before the data callback, so the socket refused the write. For an owner withwrite_in_flight, the wrapper now sets the flag and asks after the data callback. Owners without it keep the old order.A write that completes in the callback that delivers the close_notify.
on_write_donecounts the write as complete at once, and it queues the wake that lets the socket continue for the next tick. A stream that calls the write callback and then pushes the peer's close_notify in the same callback made the wrapper see no write in flight. The socket then failed the completed write, and the one it held behind it, withERR_SOCKET_CLOSED.UpgradedDuplexnow reports a write in flight until that wake ran (wake_pending).The 403.
Http2SecureServeranswers a client without ALPN from its handshake callback withsocket.end("HTTP/1.0 403 ..."). The drain handler of_http2_upgrade.tscompletes that write while the stream still has it,_finalcallsend()on the handle, and the handle closed at once.rawSocket.destroy()then dropped the write.TLS in TLS (
tls.connect({ socket: tlsSocket })). 25 runs ofwrite(8 MiB); write("tail"), then the server'send():The outer socket has
allowHalfOpen: false. It ends itself at the peer's FIN and refuses the parked write (writeAfterFIN). Node's TLS wrap owns the reads of the handle it wraps, so the wrapped socket never ends itself. A fix changes how a wrappednet.Sockettreats its EOF innet.ts. A connectednet.Socketwithout unflushed writes does not take this engine:attachClientTLSadopts its fd.What does not change
write_in_flight: None. Their close is as before.TLSSocketwriteBufferedof 64 B on a direct TLS socket 3,711 and 3,711. The same over a Duplex, without the stream's JS, 4,491 and 4,490.SSLWrapper::receive_dataof one 64 B record over a Duplex, without the socket's JS, 5,110 and 5,107.Handlers<T>grows from 72 to 88 bytes andInner<T>from 296 to 312.Inner<T>stays in the 320-byte bin.Tests
a <client|server> write in flight and the write behind it complete when the peer closes its side first, with and without anend()behind them. Node keeps such a socket half-open until its ownend(). Bun closes it once its writes are out, and the tests check that only for Bun.a <client|server> reports 'end' at the peer's close_notify while its write is in flight.a <client|server> completes the writes of its 'data' handler when the same chunk carries the peer's close_notify.a <client|server> completes the write behind one that its transport completes in the callback that delivers the peer's close_notify.what the peer sends behind its close_notify is not kept while a write is in flight: 128 MiB pushed behind the close_notify. The test keeps 2.3 to 8.3 MiB. Without the clear it keeps 290 MB.a client that negotiated no protocol reads the 403 over a transport that completes its writes a turn later.test-tls-*.jsandtest-https-*.jsfiles oftest/js/node/test/parallel. 243 pass. The other two fail on d42b055 too (test-https-timeout.js,test-tls-client-allow-partial-trust-chain.js).node-tls-connect.test.ts(387 pass, the 4 tests that need the internet fail),node-tls-server.test.ts,node-http2.test.js,node-http2-client-close.test.ts,websocket-proxy.test.ts,proxy.test.ts,renegotiation.test.ts,node-tls-upgrade.test.ts,node-tls-wrapped-socket-close.test.ts,node-tls-raw-end.test.ts,tls-connect-socket-churn.test.ts,node-tls-duplex-close-throw-uaf.test.ts. All pass. The first four ran with--timeout 30000: their child-process tests take about 5 s on a debug build in this container, on d42b055 too.rust-check-allfor bothwindows-msvctriples: 2 ok.Left out
WindowsNamedPipe). It needs a signal from itsStreamingWriterand a run on the Windows lanes.SSLWrapper. After the deferred answer the socket closes. Node keeps it writable when the stream allows half-open.end()deferred bycan_end_after_flushis retried from the drain dispatch. A handle with an http2 native writer attached, or without a drain handler, has no caller that reaches it today.