Skip to content

Commit 8ea25a3

Browse files
authored
fix(ext/node): preserve raw socket connect when wrapping TLS (#34093)
Fixes wrapping a still-connecting `net.Socket` with `tls.connect({ socket })`. The TLS wrapper now leaves the raw socket as the TCP handle owner until its pending `connect` event has fired, then transfers ownership to the TLS socket if it was not destroyed by user code. This lets raw `net.connect(..., cb)` callbacks run before TLS takes over, matching Node behavior. Also aligns custom DNS lookup options with Node by not passing `port`, and ignores `test-tls-connect-keepalive-nodelay.js` because it requires Node's unimplemented `internal/net` module.
1 parent 2668e09 commit 8ea25a3

4 files changed

Lines changed: 72 additions & 50 deletions

File tree

ext/node/polyfills/_tls_wrap.js

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -526,7 +526,11 @@ TLSSocket.prototype._wrapHandle = function (wrap, handle) {
526526

527527
// Set ownerSymbol on the parent handle so that connect callbacks
528528
// (which receive the TCP handle, not the TLSWrap) can find the socket.
529-
handle[ownerSymbol] = this;
529+
// If we are wrapping a net.Socket that still needs to emit its connect
530+
// event, keep the raw socket as owner until that event has fired.
531+
if (!(wrap instanceof net.Socket && !wrap.remoteAddress)) {
532+
handle[ownerSymbol] = this;
533+
}
530534

531535
// Proxy methods from the parent TCP handle that callers expect on _handle.
532536
// In Node, TLSWrap is a StreamBase that delegates these to the underlying
@@ -783,15 +787,27 @@ TLSSocket.prototype._init = function (socket, wrap) {
783787

784788
this.connecting = socket.connecting || !socket._handle;
785789
socket.once("connect", () => {
790+
if (this.destroyed) {
791+
return;
792+
}
793+
786794
this.connecting = false;
787795
// If the original socket created its own TCP handle during
788796
// connect() (because it had no handle when we wrapped it),
789797
// re-attach the TLS wrap to the socket's actual TCP handle.
790-
if (ssl && socket._handle) {
798+
if (ssl && socket._handle && ssl._nativeTcpHandle !== socket._handle) {
791799
const nativeHandle = socket._handle;
792-
ssl.attach(nativeHandle);
800+
ssl._attachNativeHandle(nativeHandle);
801+
nativeHandle[ownerSymbol] = this;
802+
} else if (ssl && socket._handle) {
803+
socket._handle[ownerSymbol] = this;
804+
ssl._installNativeOnread?.(socket._handle);
793805
}
794-
this.emit("connect");
806+
nextTick(() => {
807+
if (!this.destroyed) {
808+
this.emit("connect");
809+
}
810+
});
795811
});
796812
}
797813

ext/node/polyfills/internal_binding/tls_wrap.ts

Lines changed: 39 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,40 @@ const { kReadBytesOrError, streamBaseState } = core.loadExtScript(
1111
const kJSStreamHandle = Symbol.for("kJSStreamHandle");
1212
const kOwner = Symbol.for("kJSStreamOwner");
1313

14+
function installNativeOnread(res: TLSWrap, nativeHandle: any) {
15+
nativeHandle.onread = function (
16+
buf: ArrayBuffer | Uint8Array | undefined,
17+
) {
18+
const nread = streamBaseState[kReadBytesOrError];
19+
if (nread > 0 && buf) {
20+
// LibUvStreamWrap passes an ArrayBuffer; convert to Uint8Array for receive()
21+
const data = buf instanceof ArrayBuffer
22+
? new Uint8Array(buf, 0, nread)
23+
: buf.subarray(0, nread);
24+
res.receive(data);
25+
} else if (nread < 0) {
26+
// EOF or error - stop native TCP reads and unref the handle.
27+
// Without this, the libuv handle keeps a ref on the event loop
28+
// and prevents process exit after the TLS connection ends.
29+
nativeHandle.readStop();
30+
nativeHandle.unref();
31+
res.emitEof();
32+
}
33+
};
34+
}
35+
36+
function attachNativeHandle(res: TLSWrap, nativeHandle: any) {
37+
const attachResult = nativeHandle instanceof PipeWrap
38+
? res.attachPipe(nativeHandle)
39+
: res.attach(nativeHandle);
40+
if (attachResult !== 0) {
41+
throw new Error(`TLS wrap attach failed: ${attachResult}`);
42+
}
43+
44+
installNativeOnread(res, nativeHandle);
45+
res._nativeTcpHandle = nativeHandle;
46+
}
47+
1448
/**
1549
* Create a TLSWrap that intercepts an underlying stream handle.
1650
* Mirrors Node's `internalBinding('tls_wrap').wrap(handle, context, isServer)`.
@@ -53,6 +87,10 @@ function wrap(
5387
}
5488

5589
const nativeHandle = handle;
90+
res._attachNativeHandle = (nativeHandle: any) =>
91+
attachNativeHandle(res, nativeHandle);
92+
res._installNativeOnread = (nativeHandle: any) =>
93+
installNativeOnread(res, nativeHandle);
5694

5795
if (nativeHandle[kJSStreamHandle]) {
5896
// JS-backed stream (e.g. JSStreamSocket wrapping a Duplex).
@@ -125,39 +163,7 @@ function wrap(
125163
res._flushEncOut = flushEncOut;
126164
} else {
127165
// Native stream (TCP or Pipe handle).
128-
// attach/attachPipe stores the stream pointer for encrypted writes.
129-
const attachResult = nativeHandle instanceof PipeWrap
130-
? res.attachPipe(nativeHandle)
131-
: res.attach(nativeHandle);
132-
if (attachResult !== 0) {
133-
throw new Error(`TLS wrap attach failed: ${attachResult}`);
134-
}
135-
136-
// Read interception at the JS layer: intercept the TCPWrap's onread
137-
// callback to forward encrypted data from the TCP stream to the TLSWrap
138-
// via receive().
139-
// Note: LibUvStreamWrap's read callback uses (buf) signature with nread
140-
// in streamBaseState, matching onStreamRead in stream_base_commons.ts.
141-
nativeHandle.onread = function (buf: ArrayBuffer | Uint8Array | undefined) {
142-
const nread = streamBaseState[kReadBytesOrError];
143-
if (nread > 0 && buf) {
144-
// LibUvStreamWrap passes an ArrayBuffer; convert to Uint8Array for receive()
145-
const data = buf instanceof ArrayBuffer
146-
? new Uint8Array(buf, 0, nread)
147-
: buf.subarray(0, nread);
148-
res.receive(data);
149-
} else if (nread < 0) {
150-
// EOF or error - stop native TCP reads and unref the handle.
151-
// Without this, the libuv handle keeps a ref on the event loop
152-
// and prevents process exit after the TLS connection ends.
153-
nativeHandle.readStop();
154-
nativeHandle.unref();
155-
res.emitEof();
156-
}
157-
};
158-
159-
// Store the native handle so readStart/readStop can delegate to it.
160-
res._nativeTcpHandle = nativeHandle;
166+
attachNativeHandle(res, nativeHandle);
161167
}
162168

163169
// Store the JS handle reference so Rust can call JS callbacks (onhandshakedone, etc.)

ext/node/polyfills/net.ts

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1047,7 +1047,6 @@ function _lookupAndConnect(self: Socket, options: TcpSocketConnectOptions) {
10471047
family: options.family,
10481048
hints: options.hints || 0,
10491049
all: false,
1050-
port,
10511050
};
10521051

10531052
if (
@@ -1063,6 +1062,16 @@ function _lookupAndConnect(self: Socket, options: TcpSocketConnectOptions) {
10631062
debug("connect: dns options", dnsOpts);
10641063
self._host = host;
10651064
const lookup = options.lookup || dnsLookup;
1065+
const getLookupDnsOpts = () => {
1066+
if (options.lookup === undefined) {
1067+
return { ...dnsOpts, port };
1068+
}
1069+
return {
1070+
family: dnsOpts.family,
1071+
hints: dnsOpts.hints,
1072+
all: dnsOpts.all,
1073+
};
1074+
};
10661075

10671076
if (
10681077
dnsOpts.family !== 4 &&
@@ -1073,19 +1082,14 @@ function _lookupAndConnect(self: Socket, options: TcpSocketConnectOptions) {
10731082
debug("connect: autodetecting");
10741083

10751084
dnsOpts.all = true;
1076-
const lookupOpts = options.lookup === undefined ? dnsOpts : {
1077-
family: dnsOpts.family,
1078-
hints: dnsOpts.hints,
1079-
all: dnsOpts.all,
1080-
};
10811085
defaultTriggerAsyncIdScope(self[asyncIdSymbol], function () {
10821086
_lookupAndConnectMultiple(
10831087
self,
10841088
asyncIdSymbol,
10851089
lookup,
10861090
host,
10871091
options,
1088-
lookupOpts,
1092+
getLookupDnsOpts(),
10891093
port,
10901094
localAddress,
10911095
localPort,
@@ -1097,14 +1101,9 @@ function _lookupAndConnect(self: Socket, options: TcpSocketConnectOptions) {
10971101
}
10981102

10991103
defaultTriggerAsyncIdScope(self[asyncIdSymbol], function () {
1100-
const lookupOpts = options.lookup === undefined ? dnsOpts : {
1101-
family: dnsOpts.family,
1102-
hints: dnsOpts.hints,
1103-
all: dnsOpts.all,
1104-
};
11051104
lookup(
11061105
host,
1107-
lookupOpts,
1106+
getLookupDnsOpts(),
11081107
function emitLookup(
11091108
err: ErrnoException | null,
11101109
ip: string,

tests/node_compat/config.jsonc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3578,6 +3578,7 @@
35783578
"parallel/test-tls-close-event-after-write.js": {},
35793579
"parallel/test-tls-close-notify.js": {},
35803580
"parallel/test-tls-connect-address-family.js": {},
3581+
"parallel/test-tls-connect-given-socket.js": {},
35813582
"parallel/test-tls-connect-hints-option.js": {},
35823583
"parallel/test-tls-connect-hwm-option.js": {},
35833584
"parallel/test-tls-connect-keepalive-nodelay.js": {},

0 commit comments

Comments
 (0)