Skip to content

Commit bf25994

Browse files
indutnyMyles Borins
authored and
Myles Borins
committed
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. PR-URL: #9586 Reviewed-By: Ben Noordhuis <[email protected]>
1 parent d1d207b commit bf25994

File tree

2 files changed

+46
-3
lines changed

2 files changed

+46
-3
lines changed

lib/_tls_wrap.js

+20-3
Original file line numberDiff line numberDiff line change
@@ -317,14 +317,31 @@ proxiedMethods.forEach(function(name) {
317317
});
318318

319319
tls_wrap.TLSWrap.prototype.close = function closeProxy(cb) {
320-
if (this.owner)
320+
let ssl;
321+
if (this.owner) {
322+
ssl = this.owner.ssl;
321323
this.owner.ssl = null;
324+
}
325+
326+
// Invoke `destroySSL` on close to clean up possibly pending write requests
327+
// that may self-reference TLSWrap, leading to leak
328+
const done = () => {
329+
if (ssl) {
330+
ssl.destroySSL();
331+
if (ssl._secureContext.singleUse) {
332+
ssl._secureContext.context.close();
333+
ssl._secureContext.context = null;
334+
}
335+
}
336+
if (cb)
337+
cb();
338+
};
322339

323340
if (this._parentWrap && this._parentWrap._handle === this._parent) {
324-
this._parentWrap.once('close', cb);
341+
this._parentWrap.once('close', done);
325342
return this._parentWrap.destroy();
326343
}
327-
return this._parent.close(cb);
344+
return this._parent.close(done);
328345
};
329346

330347
TLSSocket.prototype._wrapHandle = function(wrap) {
+26
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
'use strict';
2+
const common = require('../common');
3+
4+
if (!common.hasCrypto) {
5+
common.skip('missing crypto');
6+
return;
7+
}
8+
9+
const assert = require('assert');
10+
const net = require('net');
11+
const tls = require('tls');
12+
13+
const server = net.createServer(common.mustCall((c) => {
14+
c.destroy();
15+
})).listen(0, common.mustCall(() => {
16+
const c = tls.connect({ port: server.address().port });
17+
c.on('error', () => {
18+
// Otherwise `.write()` callback won't be invoked.
19+
c.destroyed = false;
20+
});
21+
22+
c.write('hello', common.mustCall((err) => {
23+
assert.equal(err.code, 'ECANCELED');
24+
server.close();
25+
}));
26+
}));

0 commit comments

Comments
 (0)