Skip to content

Commit

Permalink
tls: fix leak of WriteWrap+TLSWrap combination
Browse files Browse the repository at this point in the history
Writing data to TLSWrap instance during handshake will result in it
being queued in `write_item_queue_`. This queue won't get cleared up
until the end of the handshake.

Technically, it gets cleared on `~TLSWrap` invocation, however this
won't ever happen because every `WriteWrap` holds a reference to the
`TLSWrap` through JS object, meaning that they are doomed to be alive
for eternity.

To breach this dreadful contract a knight shall embark from the
`close` function to kill the dragon of memory leak with his magic
spear of `destroySSL`.

`destroySSL` cleans up `write_item_queue_` and frees `SSL` structure,
both are good for memory usage.

PR-URL: nodejs#9586
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
indutny committed Nov 15, 2016
1 parent a804627 commit 478fabf
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 3 deletions.
23 changes: 20 additions & 3 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -305,14 +305,31 @@ proxiedMethods.forEach(function(name) {
});

tls_wrap.TLSWrap.prototype.close = function close(cb) {
if (this.owner)
let ssl;
if (this.owner) {
ssl = this.owner.ssl;
this.owner.ssl = null;
}

// Invoke `destroySSL` on close to clean up possibly pending write requests
// that may self-reference TLSWrap, leading to leak
const done = () => {
if (ssl) {
ssl.destroySSL();
if (ssl._secureContext.singleUse) {
ssl._secureContext.context.close();
ssl._secureContext.context = null;
}
}
if (cb)
cb();
};

if (this._parentWrap && this._parentWrap._handle === this._parent) {
this._parentWrap.once('close', cb);
this._parentWrap.once('close', done);
return this._parentWrap.destroy();
}
return this._parent.close(cb);
return this._parent.close(done);
};

TLSSocket.prototype._wrapHandle = function(wrap) {
Expand Down
26 changes: 26 additions & 0 deletions test/parallel/test-tls-writewrap-leak.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
'use strict';
const common = require('../common');

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

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

const server = net.createServer(common.mustCall((c) => {
c.destroy();
})).listen(0, common.mustCall(() => {
const c = tls.connect({ port: server.address().port });
c.on('error', () => {
// Otherwise `.write()` callback won't be invoked.
c.destroyed = false;
});

c.write('hello', common.mustCall((err) => {
assert.equal(err.code, 'ECANCELED');
server.close();
}));
}));

0 comments on commit 478fabf

Please sign in to comment.