net: ignore stale lookup callbacks after reconnect

A DNS lookup started before Socket.destroy() can complete after the
same Socket has been reconnected. Since reconnecting sets `connecting`
back to true, the stale lookup callback can act on the new connection
lifecycle and start another connect on its handle.

Track the socket connection generation across destroy boundaries and
ignore stale lookup and autoSelectFamily callbacks from an earlier
generation.

Signed-off-by: Hamed Elaraby <qef0@hotmail.com>

Author: wehamed

lib/net.js

@@ -376,6 +376,13 @@ function closeSocketHandle(self, isException, isCleanupPending = false) {
 const kBytesRead = Symbol('kBytesRead');
 const kBytesWritten = Symbol('kBytesWritten');
 const kSetTOS = Symbol('kSetTOS');
+// Generation token for a Socket's connection lifecycle. Async work started by
+// a connect() attempt (e.g. a DNS lookup) captures the current generation; the
+// generation is advanced once per lifecycle, in _destroy, so stale async work
+// from a destroyed lifecycle cannot drive a newer lifecycle established by a
+// later connect() (e.g. reconnect after 'close'). Multiple connect() calls
+// within one undestroyed lifecycle share a generation.
+const kConnectGeneration = Symbol('kConnectGeneration');
 // Marks a Socket whose handle is an adopted, already-bound BoundSocket.
 const kBoundSource = Symbol('kBoundSource');
 
@@ -1100,6 +1107,9 @@ Socket.prototype._destroy = function(exception, cb) {
   debug('destroy');
 
   this.connecting = false;
+  // The connection lifecycle ends here; invalidate async work (e.g. a DNS
+  // lookup) still in flight so it cannot drive a newer lifecycle.
+  this[kConnectGeneration] = (this[kConnectGeneration] || 0) + 1;
 
   for (let s = this; s !== null; s = s._parent) {
     clearTimeout(s[kTimeout]);
@@ -1415,6 +1425,13 @@ function internalConnectMultiple(context, canceled) {
   clearTimeout(context[kTimeout]);
   const self = context.socket;
 
+  // The connection lifecycle that created this context may have been destroyed
+  // and a newer connect() established; a stale context must not drive the new
+  // lifecycle's handle.
+  if (self[kConnectGeneration] !== context.connectGeneration) {
+    return;
+  }
+
   // We were requested to abort. Stop all operations
   if (self._aborted) {
     return;
@@ -1778,14 +1795,18 @@ function lookupAndConnect(self, options) {
     return;
   }
 
+  const connectGeneration = self[kConnectGeneration];
+
   defaultTriggerAsyncIdScope(self[async_id_symbol], function() {
     lookup(host, dnsopts, function emitLookup(err, ip, addressType) {
       self.emit('lookup', err, ip, addressType, host);
 
       // It's possible we were destroyed while looking this up.
       // XXX it would be great if we could cancel the promise returned by
       // the look up.
-      if (!self.connecting) return;
+      // A stale callback may also arrive after the attempt was destroyed and a
+      // newer connect() attempt was started; it must not drive the new attempt.
+      if (!self.connecting || self[kConnectGeneration] !== connectGeneration) return;
 
       if (err) {
         // net.createConnection() creates a net.Socket object and immediately
@@ -1815,12 +1836,16 @@ function lookupAndConnect(self, options) {
 function lookupAndConnectMultiple(
   self, async_id_symbol, lookup, host, options, dnsopts, port, localAddress, localPort, timeout,
 ) {
+  const connectGeneration = self[kConnectGeneration];
+
   defaultTriggerAsyncIdScope(self[async_id_symbol], function emitLookup() {
     lookup(host, dnsopts, function emitLookup(err, addresses) {
       // It's possible we were destroyed while looking this up.
       // XXX it would be great if we could cancel the promise returned by
       // the look up.
-      if (!self.connecting) {
+      // A stale callback may also arrive after the attempt was destroyed and a
+      // newer connect() attempt was started; it must not drive the new attempt.
+      if (!self.connecting || self[kConnectGeneration] !== connectGeneration) {
         return;
       } else if (err) {
         self.emit('lookup', err, undefined, undefined, host);
@@ -1843,7 +1868,7 @@ function lookupAndConnectMultiple(
         const { address: ip, family: addressType } = address;
         self.emit('lookup', err, ip, addressType, host);
         // It's possible we were destroyed while looking this up.
-        if (!self.connecting) {
+        if (!self.connecting || self[kConnectGeneration] !== connectGeneration) {
           return;
         }
         if (isIP(ip) && (addressType === 4 || addressType === 6)) {
@@ -1914,6 +1939,7 @@ function lookupAndConnectMultiple(
         socket: self,
         addresses: toAttempt,
         current: 0,
+        connectGeneration,
         port,
         localPort,
         timeout,
@@ -2087,6 +2113,14 @@ function afterConnectMultiple(context, current, status, handle, req, readable, w
 
   const self = context.socket;
 
+  // The connection lifecycle that created this context may have been destroyed
+  // and a newer connect() established; drop the stale completion, closing the
+  // completed handle so it cannot leak.
+  if (self[kConnectGeneration] !== context.connectGeneration) {
+    handle.close();
+    return;
+  }
+
   // Some error occurred, add to the list of exceptions
   if (status !== 0) {
     const ex = createConnectionError(req, status);
@@ -2115,6 +2149,14 @@ function afterConnectMultiple(context, current, status, handle, req, readable, w
 
 function internalConnectMultipleTimeout(context, req, handle) {
   debug('connect/multiple: connection to %s:%s timed out', req.address, req.port);
+
+  // The connection lifecycle that created this context may have been destroyed
+  // and a newer connect() established; drop the stale timeout so it cannot
+  // emit a spurious connectionAttemptTimeout or drive further attempts.
+  if (context.socket[kConnectGeneration] !== context.connectGeneration) {
+    return;
+  }
+
   context.socket.emit('connectionAttemptTimeout', req.address, req.port, req.addressType);
 
   req.oncomplete = undefined;

test/parallel/test-net-connect-immediate-destroy-reconnect.js

@@ -0,0 +1,114 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const net = require('net');
+
+// Regression test for https://github.com/nodejs/node/issues/55519.
+//
+// Destroying a socket while its DNS lookup is still in flight and then
+// reconnecting on 'close' must not let the stale lookup callback from the
+// destroyed attempt drive internalConnect on the new attempt's handle. Doing
+// so would issue a second connect on the same handle and fail the connection
+// with EALREADY (EINVAL on Windows).
+//
+// The flow is driven deterministically: the custom lookup captures the
+// callbacks of both attempts and the stale (attempt 1) callback is invoked
+// before the callback of the current (attempt 2) attempt.
+
+const cases = [
+  { autoSelectFamily: false },
+  { autoSelectFamily: true },
+];
+
+function runCase(options, done) {
+  let connected = false;
+  let accepted = false;
+
+  const watchdog = setTimeout(() => {
+    console.error(`test-case timed out: ${JSON.stringify(options)}`);
+    process.exit(1);
+  }, 10_000);
+
+  const socket = new net.Socket();
+
+  const finish = common.mustCall(() => {
+    clearTimeout(watchdog);
+    socket.destroy();
+    server.close();
+    done();
+  });
+
+  // Teardown only after both the client has connected and the server has
+  // accepted the connection, so the accept callback is never dropped by an
+  // early server.close().
+  const server = net.createServer(common.mustCall(() => {
+    accepted = true;
+    if (connected) {
+      finish();
+    }
+  }));
+
+  server.listen(0, common.localhostIPv4, common.mustCall(() => {
+    const port = server.address().port;
+    const lookupCalls = [];
+    let reconnected = false;
+    let connectionAttempts = 0;
+
+    function lookup(host, dnsopts, cb) {
+      lookupCalls.push({ dnsopts, cb });
+    }
+
+    socket.on('connectionAttempt', () => {
+      connectionAttempts++;
+    });
+
+    socket.on('connect', common.mustCall(() => {
+      // Only the current (second) attempt may have connected: the stale
+      // callback must not have started a connect on the new attempt's handle.
+      assert.strictEqual(connectionAttempts, 1);
+      connected = true;
+      if (accepted) {
+        finish();
+      }
+    }));
+
+    socket.on('error', common.mustNotCall());
+
+    socket.on('close', common.mustCallAtLeast(() => {
+      if (reconnected) {
+        return;
+      }
+      reconnected = true;
+      // Start a new connection attempt; the lookup callback of the first
+      // attempt is still pending.
+      socket.connect({ host: 'host.example', port, lookup, ...options });
+      assert.strictEqual(lookupCalls.length, 2);
+
+      const fire = (call) => {
+        if (call.dnsopts.all === true) {
+          call.cb(null, [{ address: common.localhostIPv4, family: 4 }]);
+        } else {
+          call.cb(null, common.localhostIPv4, 4);
+        }
+      };
+      // Invoke the stale callback first, then the current attempt's callback.
+      // The stale callback must be ignored.
+      fire(lookupCalls[0]);
+      fire(lookupCalls[1]);
+    }));
+
+    socket.connect({ host: 'host.example', port, lookup, ...options });
+    socket.destroy();
+  }));
+}
+
+let index = 0;
+function next() {
+  if (index >= cases.length) {
+    return;
+  }
+  runCase(cases[index++], next);
+}
+
+next();