-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
segfault in TLS #1696
Comments
When loading session, OCSP response, SNI, always check that the `self._handle` is present. If it is not - the socket was closed - and we should emit the error instead of throwing an uncaught exception. Fix: nodejs/node-v0.x-archive#8780 Fix: nodejs#1696
May I ask you to give a try to the following patch #1702 ? |
@indutny gladly and thanks for the quick response. Applied the patch on io.js master (4e2f999), still getting segfault:
|
By the way, as I was testing the patch npm seemed to fail installing four different modules.
|
* Destroy `SSL*` and friends on a next tick to make sure that we are not doing it in one of the OpenSSL callbacks * Add more checks to the C++ methods that might be invoked during destructor's pending queue cleanup Fix: nodejs/node-v0.x-archive#8780 Fix: nodejs#1696
@indutny I've tested the changes on 2.1.0 as well, still segfaults:
Any way I can help with more info? |
@EricTheOne I commented about it on other issue. I really need a test case that reproduces the problem. It doesn't look like I fully understand the problem atm. |
When loading session, OCSP response, SNI, always check that the `self._handle` is present. If it is not - the socket was closed - and we should emit the error instead of throwing an uncaught exception. Fix: nodejs/node-v0.x-archive#8780 Fix: nodejs#1696
* Destroy `SSL*` and friends on a next tick to make sure that we are not doing it in one of the OpenSSL callbacks * Add more checks to the C++ methods that might be invoked during destructor's pending queue cleanup Fix: nodejs/node-v0.x-archive#8780 Fix: nodejs#1696
When loading session, OCSP response, SNI, always check that the `self._handle` is present. If it is not - the socket was closed - and we should emit the error instead of throwing an uncaught exception. Fix: nodejs/node-v0.x-archive#8780 Fix: nodejs#1696
* Destroy `SSL*` and friends on a next tick to make sure that we are not doing it in one of the OpenSSL callbacks * Add more checks to the C++ methods that might be invoked during destructor's pending queue cleanup Fix: nodejs/node-v0.x-archive#8780 Fix: nodejs#1696
When loading session, OCSP response, SNI, always check that the `self._handle` is present. If it is not - the socket was closed - and we should emit the error instead of throwing an uncaught exception. Fix: nodejs/node-v0.x-archive#8780 Fix: #1696 PR-URL: #1702 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
* Destroy `SSL*` and friends on a next tick to make sure that we are not doing it in one of the OpenSSL callbacks * Add more checks to the C++ methods that might be invoked during destructor's pending queue cleanup Fix: nodejs/node-v0.x-archive#8780 Fix: #1696 PR-URL: #1702 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@EricTheOne PTAL at #1910, I think it should fix the issue for you ;) |
Queued write requests should be invoked on handle close, otherwise the "consumer" might be already destroyed when the write callbacks of the "consumed" handle will be invoked. Fix: nodejs#1696
Queued write requests should be invoked on handle close, otherwise the "consumer" might be already destroyed when the write callbacks of the "consumed" handle will be invoked. Same applies to the shutdown requests. Make sure to "move" away socket from server to not break the `connections` counter in `net.js`. Otherwise it might not call `close` callback, or call it too early. Fix: #1696 PR-URL: #1910 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Ala #1910 (comment), @EricTheOne did that patch fix this issue? :) |
Queued write requests should be invoked on handle close, otherwise the "consumer" might be already destroyed when the write callbacks of the "consumed" handle will be invoked. Same applies to the shutdown requests. Make sure to "move" away socket from server to not break the `connections` counter in `net.js`. Otherwise it might not call `close` callback, or call it too early. Fix: nodejs#1696 PR-URL: nodejs#1910 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
I'm going to assume it did. Please re-open if it didn't. |
Reproduces 100% of the time within a minute of running the server. Server opens multiple TLS client connections (as a client).
I'll gladly help with testing a fix or sharing some of the backtrace data, but cannot share server code or specific scenario.
Please see nodejs/node-v0.x-archive#8780, I'm not 100% sure but I think it may be the same issue. I'm unable to reproduce it in node 0.12.2 (server is running fine, the segfault doesn't happen).
Backtrace (commit 7693705):
dmesg:
segfault at 30 ip 000000000069eed0 sp 00007ffe6b7e41a8 error 4 in iojs[400000+d0e000]
Although nodejs/node-v0.x-archive#8780 contains a suggested fix, I don't see that it was applied in node.js 0.12.2, so either something else fixed it or the situation is different.
Naively applying the fix suggested in 8780 on io.js 2.0.1 (18d457b) doesn't seem to help. Instead I'm failing on an assertion.
"Fix" (tls_wrap.cc):
Backtrace with fix applied:
The text was updated successfully, but these errors were encountered: