From 3bfdd1aadf6019ecba3579099fd3bbf54caeb8ac Mon Sep 17 00:00:00 2001
From: Daeyeon Jeong <daeyeon.dev@gmail.com>
Date: Mon, 1 Aug 2022 15:37:45 +0900
Subject: [PATCH] net,tls: pass a valid socket on `tlsClientError`

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: https://github.com/nodejs/node/pull/44021
Fixes: https://github.com/nodejs/node/issues/43963
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
---
 lib/_tls_wrap.js                        |  4 +++
 lib/net.js                              | 33 +++++++++++++++++++------
 test/internet/test-https-issue-43963.js | 31 +++++++++++++++++++++++
 3 files changed, 61 insertions(+), 7 deletions(-)
 create mode 100644 test/internet/test-https-issue-43963.js

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 50157e4fcd3e9d..11ff14d725b36b 100644
--- 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
index a1be06346b28a5..2d4894a06e5da1 100644
--- 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
new file mode 100644
index 00000000000000..0d5a6109145d1b
--- /dev/null
+++ 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();
+});