Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: fix leak of WriteWrap+TLSWrap combination #9586

Closed
wants to merge 3 commits into from
Closed

tls: fix leak of WriteWrap+TLSWrap combination #9586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d603f9
tls: fix leak of WriteWrap+TLSWrap combination
indutny Nov 12, 2016
e6869b8
fixes
indutny Nov 14, 2016
717f9bf
fix
indutny Nov 15, 2016
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
23 changes: 20 additions & 3 deletions lib/_tls_wrap.js
View file
Open in desktop
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 a leak.
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 (typeof cb === 'function')
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
View file
Open in desktop
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.deepEqual(err.code, 'ECANCELED');
server.close();
}));
}));