Repository navigation
node:net: refuse connect() where node's handle refuses it #45346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1388,6 +1388,53 @@ impl<const SSL: bool> NewSocket<SSL> { | |
| socket.close(code); | ||
| } | ||
|
|
||
| /// The errno that node's handle answers to this `connect()` while it owns a socket. `None`: dial. | ||
| pub(crate) fn connect_refusal( | ||
| &self, | ||
| connection: &super::listener::UnixOrHost, | ||
| ) -> Option<sys::SystemErrno> { | ||
| use super::listener::UnixOrHost; | ||
| use sys::SystemErrno as E; | ||
| let socket = self.socket.get(); | ||
| if socket.is_detached() { | ||
| return None; | ||
| } | ||
| // libuv opens an fd only on a handle that has none. | ||
| if matches!(connection, UnixOrHost::Fd(_)) { | ||
| return Some(E::EBUSY); | ||
| } | ||
| let is_tcp = matches!( | ||
| socket.local_address(&mut [0u8; 64]).map(<[u8]>::len), | ||
| Some(4 | 16) | ||
| ); | ||
| if !is_tcp { | ||
| // A pipe handle reports what connect(2) says for the new path: EISCONN when it is a socket. | ||
| return Some(E::EISCONN); | ||
| } | ||
| // Node parses the path as an IPv4 address first: https://github.com/nodejs/node/blob/v26.3.0/src/tcp_wrap.cc#L433-L465 | ||
| if let UnixOrHost::Unix(path) = connection { | ||
| if !matches!( | ||
| bun_core::ip_address::parse_uv_inet_pton(path), | ||
| Some(core::net::IpAddr::V4(_)) | ||
| ) { | ||
| return Some(E::EINVAL); | ||
| } | ||
| } | ||
| if !socket.is_established() { | ||
| // One connect per handle (ConnectEx says EISCONN): https://github.com/nodejs/node/blob/v26.3.0/deps/uv/src/unix/tcp.c#L288-L289 | ||
| return Some(if cfg!(windows) { | ||
| E::EISCONN | ||
| } else { | ||
| E::EALREADY | ||
| }); | ||
| } | ||
| // Linux answers 0 once for a client socket that has not sent its FIN: `connect_finish` swaps it, as before. | ||
| if !self.is_server() && !socket.is_shutdown() { | ||
| return None; | ||
| } | ||
| Some(E::EISCONN) | ||
| } | ||
|
|
||
| /// Discard a still-live native socket so this wrapper can be reused for a | ||
| /// fresh connect. `node:net` permits `socket.connect()` on an | ||
| /// already-connected socket; without this the previous `us_socket_t`'s | ||
|
Comment on lines
+1441
to
1444
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 (optional) Programs that call end() and then connect() on the same net.Socket before 'close' now get an EISCONN error and a destroyed socket; on the base branch the socket redialed. connect_refusal at src/runtime/socket/socket_body.rs:1441-1444 answers EISCONN whenever is_shutdown is set. For end(); connect(ip) in one tick, _final queues endNT (src/js/node/net.ts:2588) ahead of the connect's nextTick (net.ts:3175), so shutdown(2) always lands first. Node differs there: libuv defers shutdown(2) to the poll phase, so connect(2) runs first and emits 'connect' again. Fix: gate the EISCONN answer on the FIN having actually been sent (or defer the shutdown like libuv), and keep the original 'should allow reconnecting after end()' test instead of rewriting it to wait for 'close'. Why this was flaggedTrigger: a client socket calls socket.end() then socket.connect(port, '127.0.0.1') before 'close' (same tick or a few ms later). end() calls _final synchronously, which queues endNT on nextTick (src/js/node/net.ts:2588); endNT calls handle.shutdown() (net.ts:439) which runs shutdown(2) immediately. connect() queues internalConnect on a later nextTick (net.ts:3175), so by the time connect_inner runs the socket is established, client, and is_shutdown, and src/runtime/socket/socket_body.rs:1441-1444 returns EISCONN. internalConnect then destroys the socket with 'connect EISCONN' (net.ts:3430-3431); a program with no 'error' listener exits with an uncaught exception. On the base branch connect_finish called detach_for_reconnect and dialed again, so these programs worked. Node's uv_shutdown only performs shutdown(2) in uv__drain during the next poll phase, after internalConnect's connect(2) has already returned 0 and 'connect' was emitted; Node only answers EISCONN once the FIN was really sent. Verification: _final queues endNT on nextTick (src/js/node/net.ts:2588). endNT calls handle.shutdown() (net.ts:439). The connect's own nextTick (net.ts:3175) was queued after it, so connect_refusal returns Some(E::EISCONN) at src/runtime/socket/socket_body.rs:1436-1441. On the base branch this path went through connect_finish → detach_for_reconnect and emitted 'connect'. |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Callers whose host resolves to two addresses now get an
EALREADY/EISCONNerror and a destroyed socket where both Node and the base branch deliver 'connect'. At src/js/node/net.ts:3475-3477 the multi-address walk destroys the socket with the last error as soon as the handle refuses one attempt, instead of closing that socket silently and dialing the next address as Node's kReinitializeHandle path does. Fix: on a refusal with addresses still left, close the handle's socket without emitting an event and continue the walk, destroying only when the address list is exhausted. Trigger:net.connect(port)with the default host (localhost -> ::1 and 127.0.0.1) called twice while the first dial is in flight (#21524's program).Why this was flagged
A net.Socket whose handle still owns a socket calls connect() with a host that the lookup resolves to more than one address, so lookupAndConnect takes internalConnectMultiple (src/js/node/net.ts:3448). The first attempt is refused by NewSocket::connect_refusal (src/runtime/socket/socket_body.rs:1393) with EALREADY or EISCONN, kConnectTcp returns the errno, the error is pushed and internalConnectMultiple recurses (src/js/node/net.ts:3551). With current > 0 the new check at src/js/node/net.ts:3475 sees ownsSocket(self._handle) true and calls self.destroy(context.errors[context.errors.length - 1]) at src/js/node/net.ts:3476, so no further address is dialed. On the base branch the same recursion reached self[kReinitializeHandle] and the next address was dialed; the socket emitted 'connect'. Node v26 also closes the refused handle's socket in kReinitializeHandle and dials the next address, emitting 'connect'. Users with retry code that calls connect() twice on localhost get an uncaught 'error' and a closed socket after merging.
Verification: On the recursion
current > 0, and the new check at src/js/node/net.ts:3475-3477 destroys the socket with the EISCONN/EALREADY error instead of thekReinitializeHandle(newDetachedSocket(...))on line 3479 that would dial the next address. On the base branch this same call randetach_for_reconnectand dialed the first address, delivering'connect'.