From d943cb73115d081c7b1720e2934bba0d3ee8262e Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 31 Oct 2016 16:48:14 -0600 Subject: [PATCH 1/2] stream_base,tls_wrap: notify on destruct The TLSWrap constructor is passed a StreamBase* which it stores as TLSWrap::stream_, and is used to receive/send data along the pipeline (e.g. tls -> tcp). Problem is the lifetime of the instance that stream_ points to is independent of the lifetime of the TLSWrap instance. So it's possible for stream_ to be delete'd while the TLSWrap instance is still alive, allowing potential access to a then invalid pointer. Fix by having the StreamBase destructor null out TLSWrap::stream_; allowing all TLSWrap methods that rely on stream_ to do a check to see if it's available. While the test provided is fixed by this commit, it was also previously fixed by 478fabf. Regardless, leave the test in for better testing. PR-URL: https://github.com/nodejs/node/pull/11947 --- src/stream_base.h | 9 +++- src/tls_wrap.cc | 7 ++++ src/tls_wrap.h | 3 ++ .../test-tls-retain-handle-no-abort.js | 42 +++++++++++++++++++ 4 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-tls-retain-handle-no-abort.js diff --git a/src/stream_base.h b/src/stream_base.h index faddee88c82786..35929750bfbc54 100644 --- a/src/stream_base.h +++ b/src/stream_base.h @@ -146,10 +146,14 @@ class StreamResource { const uv_buf_t* buf, uv_handle_type pending, void* ctx); + typedef void (*DestructCb)(void* ctx); StreamResource() : bytes_read_(0) { } - virtual ~StreamResource() = default; + virtual ~StreamResource() { + if (!destruct_cb_.is_empty()) + destruct_cb_.fn(destruct_cb_.ctx); + } virtual int DoShutdown(ShutdownWrap* req_wrap) = 0; virtual int DoTryWrite(uv_buf_t** bufs, size_t* count); @@ -186,15 +190,18 @@ class StreamResource { inline void set_alloc_cb(Callback c) { alloc_cb_ = c; } inline void set_read_cb(Callback c) { read_cb_ = c; } + inline void set_destruct_cb(Callback c) { destruct_cb_ = c; } inline Callback after_write_cb() { return after_write_cb_; } inline Callback alloc_cb() { return alloc_cb_; } inline Callback read_cb() { return read_cb_; } + inline Callback destruct_cb() { return destruct_cb_; } private: Callback after_write_cb_; Callback alloc_cb_; Callback read_cb_; + Callback destruct_cb_; uint64_t bytes_read_; friend class StreamBase; diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index d21976c7df309e..3dad65011ff85b 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -85,6 +85,7 @@ TLSWrap::TLSWrap(Environment* env, stream_->set_after_write_cb({ OnAfterWriteImpl, this }); stream_->set_alloc_cb({ OnAllocImpl, this }); stream_->set_read_cb({ OnReadImpl, this }); + stream_->set_destruct_cb({ OnDestructImpl, this }); set_alloc_cb({ OnAllocSelf, this }); set_read_cb({ OnReadSelf, this }); @@ -687,6 +688,12 @@ void TLSWrap::OnReadImpl(ssize_t nread, } +void TLSWrap::OnDestructImpl(void* ctx) { + TLSWrap* wrap = static_cast(ctx); + wrap->clear_stream(); +} + + void TLSWrap::OnAllocSelf(size_t suggested_size, uv_buf_t* buf, void* ctx) { buf->base = node::Malloc(suggested_size); buf->len = suggested_size; diff --git a/src/tls_wrap.h b/src/tls_wrap.h index d6c4b62493d10b..f1d53f3e2f5da1 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -75,6 +75,8 @@ class TLSWrap : public AsyncWrap, size_t self_size() const override { return sizeof(*this); } + void clear_stream() { stream_ = nullptr; } + protected: static const int kClearOutChunkSize = 16384; @@ -142,6 +144,7 @@ class TLSWrap : public AsyncWrap, const uv_buf_t* buf, uv_handle_type pending, void* ctx); + static void OnDestructImpl(void* ctx); void DoRead(ssize_t nread, const uv_buf_t* buf, uv_handle_type pending); diff --git a/test/parallel/test-tls-retain-handle-no-abort.js b/test/parallel/test-tls-retain-handle-no-abort.js new file mode 100644 index 00000000000000..43b3709fd5f85b --- /dev/null +++ b/test/parallel/test-tls-retain-handle-no-abort.js @@ -0,0 +1,42 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); + +if (!common.hasCrypto) { + common.skip('missing crypto'); + return; +} +const tls = require('tls'); +const fs = require('fs'); +const util = require('util'); + +const sent = 'hello world'; +const serverOptions = { + isServer: true, + key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'), + cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem') +}; + +let ssl = null; + +process.on('exit', function() { + assert.ok(ssl !== null); + // If the internal pointer to stream_ isn't cleared properly then this + // will abort. + util.inspect(ssl); +}); + +const server = tls.createServer(serverOptions, function(s) { + s.on('data', function() { }); + s.on('end', function() { + server.close(); + s.destroy(); + }); +}).listen(0, function() { + const c = new tls.TLSSocket(); + ssl = c.ssl; + c.connect(this.address().port, function() { + c.end(sent); + }); +}); From ab676157bdabd343fc4b733d8ef1b601f7dcaafb Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Mon, 20 Mar 2017 16:26:10 -0600 Subject: [PATCH 2/2] Partial revert "tls: keep track of stream that is closed" This partually reverts commit 4cdb0e89d8daf7e1371c3b8d3f057940aa327d4a. A nullptr check in TSLWrap::IsAlive() and the added test were left. PR-URL: https://github.com/nodejs/node/pull/11947 --- lib/_tls_wrap.js | 6 ------ src/tls_wrap.cc | 9 --------- src/tls_wrap.h | 1 - 3 files changed, 16 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 288f82e05b3d12..511ff3557a6e23 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -399,12 +399,6 @@ TLSSocket.prototype._wrapHandle = function(wrap) { res = null; }); - if (wrap) { - wrap.on('close', function() { - res.onStreamClose(); - }); - } - return res; }; diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 3dad65011ff85b..6f2d0e4c16d576 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -815,14 +815,6 @@ void TLSWrap::EnableSessionCallbacks( } -void TLSWrap::OnStreamClose(const FunctionCallbackInfo& args) { - TLSWrap* wrap; - ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); - - wrap->stream_ = nullptr; -} - - void TLSWrap::DestroySSL(const FunctionCallbackInfo& args) { TLSWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); @@ -953,7 +945,6 @@ void TLSWrap::Initialize(Local target, env->SetProtoMethod(t, "enableSessionCallbacks", EnableSessionCallbacks); env->SetProtoMethod(t, "destroySSL", DestroySSL); env->SetProtoMethod(t, "enableCertCb", EnableCertCb); - env->SetProtoMethod(t, "onStreamClose", OnStreamClose); StreamBase::AddMethods(env, t, StreamBase::kFlagHasWritev); SSLWrap::AddMethods(env, t); diff --git a/src/tls_wrap.h b/src/tls_wrap.h index f1d53f3e2f5da1..19633ea8667fff 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -161,7 +161,6 @@ class TLSWrap : public AsyncWrap, static void EnableCertCb( const v8::FunctionCallbackInfo& args); static void DestroySSL(const v8::FunctionCallbackInfo& args); - static void OnStreamClose(const v8::FunctionCallbackInfo& args); #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB static void GetServername(const v8::FunctionCallbackInfo& args);