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: #17478
Fixes: #17475
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Author: addaleax

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;
 }
 
 

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();
+  });
+});