From 9d603f97feb0a7b4781843ad45df273cf6ce01cc Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sat, 12 Nov 2016 16:08:59 -0500 Subject: [PATCH 1/3] tls: fix leak of WriteWrap+TLSWrap combination 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. --- lib/_tls_wrap.js | 23 ++++++++++++++++++--- test/parallel/test-tls-writewrap-leak.js | 26 ++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-tls-writewrap-leak.js diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 478fd20188196d..cd793cb34f0a54 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -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) { diff --git a/test/parallel/test-tls-writewrap-leak.js b/test/parallel/test-tls-writewrap-leak.js new file mode 100644 index 00000000000000..4386efdc935f21 --- /dev/null +++ b/test/parallel/test-tls-writewrap-leak.js @@ -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(); + })); +})); From e6869b87035949da59bbd2d0a9df44cc8f33f39f Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sun, 13 Nov 2016 22:06:10 -0500 Subject: [PATCH 2/3] fixes --- lib/_tls_wrap.js | 2 +- test/parallel/test-tls-writewrap-leak.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index cd793cb34f0a54..8d9c0ea6d16e88 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -321,7 +321,7 @@ tls_wrap.TLSWrap.prototype.close = function close(cb) { ssl._secureContext.context = null; } } - if (cb) + if (typeof cb === 'function') cb(); }; diff --git a/test/parallel/test-tls-writewrap-leak.js b/test/parallel/test-tls-writewrap-leak.js index 4386efdc935f21..10dfefac81582e 100644 --- a/test/parallel/test-tls-writewrap-leak.js +++ b/test/parallel/test-tls-writewrap-leak.js @@ -20,7 +20,7 @@ const server = net.createServer(common.mustCall((c) => { }); c.write('hello', common.mustCall((err) => { - assert.equal(err.code, 'ECANCELED'); + assert.deepEqual(err.code, 'ECANCELED'); server.close(); })); })); From 717f9bf28f2f333fffa50d91a15e0d3e87736219 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Tue, 15 Nov 2016 04:25:46 -0500 Subject: [PATCH 3/3] fix --- lib/_tls_wrap.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 8d9c0ea6d16e88..fc95e79f0d1c4c 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -312,7 +312,7 @@ tls_wrap.TLSWrap.prototype.close = function close(cb) { } // Invoke `destroySSL` on close to clean up possibly pending write requests - // that may self-reference TLSWrap, leading to leak + // that may self-reference TLSWrap, leading to a leak. const done = () => { if (ssl) { ssl.destroySSL();