Skip to content

Commit

Permalink
stream_base,tls_wrap: notify on destruct
Browse files Browse the repository at this point in the history
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: #11947
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
  • Loading branch information
trevnorris authored and MylesBorins committed Apr 19, 2017
1 parent 99749dc commit ff1d61c
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 1 deletion.
9 changes: 8 additions & 1 deletion src/stream_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,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);
Expand Down Expand Up @@ -175,15 +179,18 @@ class StreamResource {

inline void set_alloc_cb(Callback<AllocCb> c) { alloc_cb_ = c; }
inline void set_read_cb(Callback<ReadCb> c) { read_cb_ = c; }
inline void set_destruct_cb(Callback<DestructCb> c) { destruct_cb_ = c; }

inline Callback<AfterWriteCb> after_write_cb() { return after_write_cb_; }
inline Callback<AllocCb> alloc_cb() { return alloc_cb_; }
inline Callback<ReadCb> read_cb() { return read_cb_; }
inline Callback<DestructCb> destruct_cb() { return destruct_cb_; }

private:
Callback<AfterWriteCb> after_write_cb_;
Callback<AllocCb> alloc_cb_;
Callback<ReadCb> read_cb_;
Callback<DestructCb> destruct_cb_;
uint64_t bytes_read_;

friend class StreamBase;
Expand Down
7 changes: 7 additions & 0 deletions src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,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 });
Expand Down Expand Up @@ -661,6 +662,12 @@ void TLSWrap::OnReadImpl(ssize_t nread,
}


void TLSWrap::OnDestructImpl(void* ctx) {
TLSWrap* wrap = static_cast<TLSWrap*>(ctx);
wrap->clear_stream();
}


void TLSWrap::OnAllocSelf(size_t suggested_size, uv_buf_t* buf, void* ctx) {
buf->base = static_cast<char*>(node::Malloc(suggested_size));
CHECK_NE(buf->base, nullptr);
Expand Down
3 changes: 3 additions & 0 deletions src/tls_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,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;

Expand Down Expand Up @@ -119,6 +121,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);

Expand Down
42 changes: 42 additions & 0 deletions test/parallel/test-tls-retain-handle-no-abort.js
Original file line number Diff line number Diff line change
@@ -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);
});
});

0 comments on commit ff1d61c

Please sign in to comment.