Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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.
  • Loading branch information
indutny committed Nov 12, 2016
commit 9d603f97feb0a7b4781843ad45df273cf6ce01cc
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"leading to a leak" dot.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

const done = () => {
if (ssl) {
ssl.destroySSL();
if (ssl._secureContext.singleUse) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does .singleUse signify? It's always true as far as I can tell because it's the inversion of options.keepAlive, which is not a documented option and seems to have no test coverage or uses in core.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keepAlive is an option passed by https.Agent, I suspect that it should be tested in our test suite through https module.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand. keepAlive is a layer 7 thing, why would that influence the TLS layer?

ssl._secureContext.context.close();
ssl._secureContext.context = null;
}
}
if (cb)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check typeof cb === 'function'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

cb();
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, what would happen when ssl.destroySSL() gets called synchronously, i.e., outside the callback?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crash, because of StreamBase callbacks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is actually way simpler than this, I'll simplify the code.

Thank you for bringing my attention to this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It crashes if done in the same tick because it may be invoked within the OpenSSL's call stack. If invoked with setImmediate - it won't crash.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... just run the test suite after using setImmediate() and it confirms my initial assumption. Sorry, not going to fix it.

The write callbacks are invoked with UV_ECANCELED and thus things are getting back to tls_wrap.cc at places where it doesn't expect ssl_ to be nullptr. Therefore it is better to free it up after closing the underlying libuv handle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I'm so unhappy with what we have right now (I know that I wrote most of it). I wish we would move to uv_ssl_t.


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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dot. :-)

c.destroyed = false;
});

c.write('hello', common.mustCall((err) => {
assert.equal(err.code, 'ECANCELED');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use assert.strictEqual() please.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

server.close();
}));
}));