Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit bf25994

Browse filesBrowse files
indutnyMyles Borins
authored andcommitted
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 <info@bnoordhuis.nl>
1 parent d1d207b commit bf25994
Copy full SHA for bf25994

File tree

Expand file treeCollapse file tree

2 files changed

+46
-3
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+46
-3
lines changed
Open diff view settings
Collapse file

‎lib/_tls_wrap.js‎

Copy file name to clipboardExpand all lines: lib/_tls_wrap.js
+20-3Lines changed: 20 additions & 3 deletions
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) {
Collapse file
+26Lines changed: 26 additions & 0 deletions
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.