src: fix TLSWrap lifetime bug in ALPN callback #49635
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Retrieve the TLSWrap from the SSL object, not SSL_CTX. A SSL_CTX object is the parent of zero or more SSL objects. TLSWrap is a wrapper around SSL, SecureContext around SSL_CTX. Node.js normally uses a SecureContext per TLSWrap but it is possible to use a SecureContext object more than once. It was therefore possible for an ALPN callback to use the wrong (possibly already freed) TLSWrap object. Having said that, while the bug is clear once you see it, I'm not able to trigger it (and hence no test, not for lack of trying.) None of the bug reporters were able to reliably reproduce it either so the stars probably need to align just right in order to hit it. Fixes: nodejs#47207
|
Review requested:
|
nodejs-github-bot
added
c++
lpinca
approved these changes
Sep 14, 2023
JungMinu
approved these changes
Sep 20, 2023
|
This presumably fixes the crash at the following code location: [pc=0x0000000000e69847, sp=0x00007ffd3c6aecb0] in node::crypto::(anonymous namespace)::SelectALPNCallback(ssl_st*, unsigned char const**, unsigned char*, unsigned char const*, unsigned int, void*)+0x87 We can confirm this is currently present on node v20.7.0 and node v18.18.0 The issue is causing 1 of about 2M HTTP request to segfault the server. Once fix is available in v20, we will be able to confirm within a day of production usage if it addresses the core problem. |
|
We can confirm that this patch addresses the crashes in node 20.8.0. |
|
Is there anything blocking this PR from being merged? It would be great to see this fix included in the next v18 / v20 release. |
github-actions
bot
removed
the
request-ci
richardlau
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 1643adf |
This was referenced Oct 5, 2023
24 tasks
|
We were also seeing this issue in v18. Upgraded to v21 nightly and no segfaults in production for last 12 hrs where we usually see 1 or 2. Would be really good to get this in a v18 or v20 release. |
targos
pushed a commit
that referenced
this pull request
Oct 28, 2023
Retrieve the TLSWrap from the SSL object, not SSL_CTX. A SSL_CTX object is the parent of zero or more SSL objects. TLSWrap is a wrapper around SSL, SecureContext around SSL_CTX. Node.js normally uses a SecureContext per TLSWrap but it is possible to use a SecureContext object more than once. It was therefore possible for an ALPN callback to use the wrong (possibly already freed) TLSWrap object. Having said that, while the bug is clear once you see it, I'm not able to trigger it (and hence no test, not for lack of trying.) None of the bug reporters were able to reliably reproduce it either so the stars probably need to align just right in order to hit it. Fixes: #47207 PR-URL: #49635 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
alexfernandez
pushed a commit
to alexfernandez/node
that referenced
this pull request
Nov 1, 2023
Retrieve the TLSWrap from the SSL object, not SSL_CTX. A SSL_CTX object is the parent of zero or more SSL objects. TLSWrap is a wrapper around SSL, SecureContext around SSL_CTX. Node.js normally uses a SecureContext per TLSWrap but it is possible to use a SecureContext object more than once. It was therefore possible for an ALPN callback to use the wrong (possibly already freed) TLSWrap object. Having said that, while the bug is clear once you see it, I'm not able to trigger it (and hence no test, not for lack of trying.) None of the bug reporters were able to reliably reproduce it either so the stars probably need to align just right in order to hit it. Fixes: nodejs#47207 PR-URL: nodejs#49635 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
targos
pushed a commit
that referenced
this pull request
Nov 11, 2023
Retrieve the TLSWrap from the SSL object, not SSL_CTX. A SSL_CTX object is the parent of zero or more SSL objects. TLSWrap is a wrapper around SSL, SecureContext around SSL_CTX. Node.js normally uses a SecureContext per TLSWrap but it is possible to use a SecureContext object more than once. It was therefore possible for an ALPN callback to use the wrong (possibly already freed) TLSWrap object. Having said that, while the bug is clear once you see it, I'm not able to trigger it (and hence no test, not for lack of trying.) None of the bug reporters were able to reliably reproduce it either so the stars probably need to align just right in order to hit it. Fixes: #47207 PR-URL: #49635 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
nromito
added a commit
to nromito/js2bin
that referenced
this pull request
Nov 16, 2023
debadree25
pushed a commit
to debadree25/node
that referenced
this pull request
Apr 15, 2024
Retrieve the TLSWrap from the SSL object, not SSL_CTX. A SSL_CTX object is the parent of zero or more SSL objects. TLSWrap is a wrapper around SSL, SecureContext around SSL_CTX. Node.js normally uses a SecureContext per TLSWrap but it is possible to use a SecureContext object more than once. It was therefore possible for an ALPN callback to use the wrong (possibly already freed) TLSWrap object. Having said that, while the bug is clear once you see it, I'm not able to trigger it (and hence no test, not for lack of trying.) None of the bug reporters were able to reliably reproduce it either so the stars probably need to align just right in order to hit it. Fixes: nodejs#47207 PR-URL: nodejs#49635 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
sercher
added a commit
to sercher/graaljs
that referenced
this pull request
Apr 25, 2024
Retrieve the TLSWrap from the SSL object, not SSL_CTX. A SSL_CTX object is the parent of zero or more SSL objects. TLSWrap is a wrapper around SSL, SecureContext around SSL_CTX. Node.js normally uses a SecureContext per TLSWrap but it is possible to use a SecureContext object more than once. It was therefore possible for an ALPN callback to use the wrong (possibly already freed) TLSWrap object. Having said that, while the bug is clear once you see it, I'm not able to trigger it (and hence no test, not for lack of trying.) None of the bug reporters were able to reliably reproduce it either so the stars probably need to align just right in order to hit it. Fixes: nodejs/node#47207 PR-URL: nodejs/node#49635 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
sercher
added a commit
to sercher/graaljs
that referenced
this pull request
Apr 25, 2024
Retrieve the TLSWrap from the SSL object, not SSL_CTX. A SSL_CTX object is the parent of zero or more SSL objects. TLSWrap is a wrapper around SSL, SecureContext around SSL_CTX. Node.js normally uses a SecureContext per TLSWrap but it is possible to use a SecureContext object more than once. It was therefore possible for an ALPN callback to use the wrong (possibly already freed) TLSWrap object. Having said that, while the bug is clear once you see it, I'm not able to trigger it (and hence no test, not for lack of trying.) None of the bug reporters were able to reliably reproduce it either so the stars probably need to align just right in order to hit it. Fixes: nodejs/node#47207 PR-URL: nodejs/node#49635 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Retrieve the TLSWrap from the SSL object, not SSL_CTX.
A SSL_CTX object is the parent of zero or more SSL objects. TLSWrap is a wrapper around SSL, SecureContext around SSL_CTX.
Node.js normally uses a SecureContext per TLSWrap but it is possible to use a SecureContext object more than once.
It was therefore possible for an ALPN callback to use the wrong (possibly already freed) TLSWrap object.
Having said that, while the bug is clear once you see it, I'm not able to trigger it (and hence no test, not for lack of trying.)
None of the bug reporters were able to reliably reproduce it either so the stars probably need to align just right in order to hit it.
Fixes: #47207