Skip to content

Commit

Permalink
net,tls: pass a valid socket on tlsClientError
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
daeyeon authored and juanarbol committed Oct 10, 2022
1 parent 9b46943 commit 1b117c2
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 7 deletions.
4 changes: 4 additions & 0 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,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 &&
Expand Down
33 changes: 26 additions & 7 deletions lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -293,6 +294,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');
Expand Down Expand Up @@ -341,6 +355,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.
Expand Down Expand Up @@ -760,15 +775,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);
Expand Down
31 changes: 31 additions & 0 deletions test/internet/test-https-issue-43963.js
Original file line number Diff line number Diff line change
@@ -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();
});

0 comments on commit 1b117c2

Please sign in to comment.