net,tls: pass a valid socket on tlsClientError

daeyeon · web-flow · commit 7f7a899fa5f3 · 2022-08-01T07:37:45.000+01:00
On the 'tlsClientError' event, the `tlsSocket` instance is passed as `closed` status. Thus, users can't get information such as `remote address`, `remoteFamily`, and so on. This adds a flag to close a socket after emitting an `error` event. Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com PR-URL: #44021 Fixes: #43963 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
@@ -423,6 +423,10 @@ function onerror(err) {
   if (!owner._secureEstablished) {
     // When handshake fails control is not yet released,
     // so self._tlsError will return null instead of actual error
+
+    // Set closing the socket after emitting an event since the socket needs to
+    // be accessible when the `tlsClientError` event is emmited.
+    owner._closeAfterHandlingError = true;
     owner.destroy(err);
   } else if (owner._tlsOptions?.isServer &&
              owner._rejectUnauthorized &&
diff --git a/lib/net.js b/lib/net.js
@@ -102,6 +102,7 @@ const {
   uvExceptionWithHostPort,
 } = require('internal/errors');
 const { isUint8Array } = require('internal/util/types');
+const { queueMicrotask } = require('internal/process/task_queues');
 const {
   validateAbortSignal,
   validateFunction,
@@ -289,6 +290,19 @@ function initSocketHandle(self) {
   }
 }
 
+function closeSocketHandle(self, isException, isCleanupPending = false) {
+  if (self._handle) {
+    self._handle.close(() => {
+      debug('emit close');
+      self.emit('close', isException);
+      if (isCleanupPending) {
+        self._handle.onread = noop;
+        self._handle = null;
+        self._sockname = null;
+      }
+    });
+  }
+}
 
 const kBytesRead = Symbol('kBytesRead');
 const kBytesWritten = Symbol('kBytesWritten');
@@ -337,6 +351,7 @@ function Socket(options) {
   this[kBuffer] = null;
   this[kBufferCb] = null;
   this[kBufferGen] = null;
+  this._closeAfterHandlingError = false;
 
   if (typeof options === 'number')
     options = { fd: options }; // Legacy interface.
@@ -755,15 +770,19 @@ Socket.prototype._destroy = function(exception, cb) {
       });
       if (err)
         this.emit('error', errnoException(err, 'reset'));
+    } else if (this._closeAfterHandlingError) {
+      // Enqueue closing the socket as a microtask, so that the socket can be
+      // accessible when an `error` event is handled in the `next tick queue`.
+      queueMicrotask(() => closeSocketHandle(this, isException, true));
     } else {
-      this._handle.close(() => {
-        debug('emit close');
-        this.emit('close', isException);
-      });
+      closeSocketHandle(this, isException);
+    }
+
+    if (!this._closeAfterHandlingError) {
+      this._handle.onread = noop;
+      this._handle = null;
+      this._sockname = null;
     }
-    this._handle.onread = noop;
-    this._handle = null;
-    this._sockname = null;
     cb(exception);
   } else {
     cb(exception);
diff --git a/test/internet/test-https-issue-43963.js b/test/internet/test-https-issue-43963.js
@@ -0,0 +1,31 @@
+'use strict';
+const common = require('../common');
+const https = require('node:https');
+const assert = require('node:assert');
+
+const server = https.createServer();
+
+server.on(
+  'tlsClientError',
+  common.mustCall((exception, tlsSocket) => {
+    assert.strictEqual(exception !== undefined, true);
+    assert.strictEqual(Object.keys(tlsSocket.address()).length !== 0, true);
+    assert.strictEqual(tlsSocket.localAddress !== undefined, true);
+    assert.strictEqual(tlsSocket.localPort !== undefined, true);
+    assert.strictEqual(tlsSocket.remoteAddress !== undefined, true);
+    assert.strictEqual(tlsSocket.remoteFamily !== undefined, true);
+    assert.strictEqual(tlsSocket.remotePort !== undefined, true);
+  }),
+);
+
+server.listen(0, () => {
+  const req = https.request({
+    hostname: '127.0.0.1',
+    port: server.address().port,
+  });
+  req.on(
+    'error',
+    common.mustCall(() => server.close()),
+  );
+  req.end();
+});
