From f96a86cac583675d10f601316c6ec4803bb851cc Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 6 Dec 2017 05:29:29 +0100 Subject: [PATCH] tls: unconsume stream on destroy When the TLS stream is destroyed for whatever reason, we should unset all callbacks on the underlying transport stream. PR-URL: https://github.com/nodejs/node/pull/17478 Fixes: https://github.com/nodejs/node/issues/17475 Reviewed-By: Fedor Indutny Reviewed-By: Jan Krems Reviewed-By: Matteo Collina Reviewed-By: James M Snell --- src/tls_wrap.cc | 21 +++++++++++-- ...test-tls-transport-destroy-after-own-gc.js | 30 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-tls-transport-destroy-after-own-gc.js diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 3b899ea12d501d..3f5ed2c57580ff 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -101,6 +101,19 @@ TLSWrap::~TLSWrap() { #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB sni_context_.Reset(); #endif // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB + + // See test/parallel/test-tls-transport-destroy-after-own-gc.js: + // If this TLSWrap is garbage collected, we cannot allow callbacks to be + // called on this stream. + + if (stream_ == nullptr) + return; + stream_->set_destruct_cb({ nullptr, nullptr }); + stream_->set_after_write_cb({ nullptr, nullptr }); + stream_->set_alloc_cb({ nullptr, nullptr }); + stream_->set_read_cb({ nullptr, nullptr }); + stream_->set_destruct_cb({ nullptr, nullptr }); + stream_->Unconsume(); } @@ -564,12 +577,16 @@ uint32_t TLSWrap::UpdateWriteQueueSize(uint32_t write_queue_size) { int TLSWrap::ReadStart() { - return stream_->ReadStart(); + if (stream_ != nullptr) + return stream_->ReadStart(); + return 0; } int TLSWrap::ReadStop() { - return stream_->ReadStop(); + if (stream_ != nullptr) + return stream_->ReadStop(); + return 0; } diff --git a/test/parallel/test-tls-transport-destroy-after-own-gc.js b/test/parallel/test-tls-transport-destroy-after-own-gc.js new file mode 100644 index 00000000000000..46f630982af643 --- /dev/null +++ b/test/parallel/test-tls-transport-destroy-after-own-gc.js @@ -0,0 +1,30 @@ +// Flags: --expose-gc +'use strict'; + +// Regression test for https://github.com/nodejs/node/issues/17475 +// Unfortunately, this tests only "works" reliably when checked with valgrind or +// a similar tool. + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const { TLSSocket } = require('tls'); +const makeDuplexPair = require('../common/duplexpair'); + +let { clientSide } = makeDuplexPair(); + +let clientTLS = new TLSSocket(clientSide, { isServer: false }); +// eslint-disable-next-line no-unused-vars +let clientTLSHandle = clientTLS._handle; + +setImmediate(() => { + clientTLS = null; + global.gc(); + clientTLSHandle = null; + global.gc(); + setImmediate(() => { + clientSide = null; + global.gc(); + }); +});