-
Notifications
You must be signed in to change notification settings - Fork 30k
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.
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't quite understand. |
||
ssl._secureContext.context.close(); | ||
ssl._secureContext.context = null; | ||
} | ||
} | ||
if (cb) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack. |
||
cb(); | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious, what would happen when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Crash, because of StreamBase callbacks. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh... just run the test suite after using The write callbacks are invoked with UV_ECANCELED and thus things are getting back to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
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) { | ||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack. |
||
server.close(); | ||
})); | ||
})); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.