Skip to content

Commit

Permalink
tls: fix segfault on destroy after partial read
Browse files Browse the repository at this point in the history
OnRead() calls into JS land which can result in the SSL context object
being destroyed on return.  Check that `ssl_ != nullptr` afterwards.

Fixes: #11885
PR-URL: #11898
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
bnoordhuis authored and italoacasas committed Mar 21, 2017
1 parent 0c00b65 commit c626734
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,12 @@ void TLSWrap::ClearOut() {
memcpy(buf.base, current, avail);
OnRead(avail, &buf);

// Caveat emptor: OnRead() calls into JS land which can result in
// the SSL context object being destroyed. We have to carefully
// check that ssl_ != nullptr afterwards.
if (ssl_ == nullptr)
return;

read -= avail;
current += avail;
}
Expand Down
36 changes: 36 additions & 0 deletions test/parallel/test-tls-socket-destroy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
'use strict';

const common = require('../common');

if (!common.hasCrypto) {
common.skip('missing crypto');
return;
}

const fs = require('fs');
const net = require('net');
const tls = require('tls');

const key = fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem');
const cert = fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem');
const secureContext = tls.createSecureContext({ key, cert });

const server = net.createServer(common.mustCall((conn) => {
const options = { isServer: true, secureContext, server };
const socket = new tls.TLSSocket(conn, options);
socket.once('data', common.mustCall(() => {
socket._destroySSL(); // Should not crash.
server.close();
}));
}));

server.listen(0, function() {
const options = {
port: this.address().port,
rejectUnauthorized: false,
};
tls.connect(options, function() {
this.write('*'.repeat(1 << 20)); // Write more data than fits in a frame.
this.on('error', this.destroy); // Server closes connection on us.
});
});

0 comments on commit c626734

Please sign in to comment.