src: fix segfault handling/RegisterSignalHandler #27775
Conversation
This comment has been minimized.
This comment has been minimized.
nodejs-github-bot
added
the
c++
This comment has been minimized.
This comment has been minimized.
| // with ALLOW_CRASHES set. | ||
| if (signal !== null && !process.env.ALLOW_CRASHES) | ||
| continue; | ||
| // reset_handler does not work on FreeBSD. |
There was a problem hiding this comment.
This can be removed when #27780 lands, I think?
There was a problem hiding this comment.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@Trott thanks for re-starting CI, but the failures here are genuine and are not trivially solved – I’m still thinking about a nice solution, if I can’t come up with one then I’ll drop the corresponding parts of this PR… |
Cool. I added the WIP label so I don't indiscriminately restart the CI again. Of course, remove it when appropriate. |
|
Okay, updated with the third commit changed to forbid the @jasnell @bnoordhuis Could you take another look and make sure your reviews still stand? |
This comment has been minimized.
This comment has been minimized.
addaleax
added
author ready
This comment has been minimized.
This comment has been minimized.
This macro is only defined when building Node.js, so addons cannot use it as a way of detecting feature availability.
Without this, we would re-enter the signal handler immediately after re-raising the signal, leading to an infinite loop.
This is not easily implementable, and should be explicitly disallowed.
Ensure coverage for the different combinations of arguments.
|
Landed in 282e2f6...039cfdc |
This macro is only defined when building Node.js, so addons cannot use it as a way of detecting feature availability. PR-URL: #27775 Refs: #27246 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Without this, we would re-enter the signal handler immediately after re-raising the signal, leading to an infinite loop. PR-URL: #27775 Refs: #27246 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
This is not easily implementable, and should be explicitly disallowed. PR-URL: #27775 Refs: #27246 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Ensure coverage for the different combinations of arguments. PR-URL: #27775 Refs: #27246 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
|
@addaleax what was the reason for |
src: do not use posix feature macro in node.h
This macro is only defined when building Node.js, so addons cannot
use it as a way of detecting feature availability.
src: reset SIGSEGV handler before crashing
Without this, we would re-enter the signal handler immediately
after re-raising the signal, leading to an infinite loop.
##### src: implement reset_handler for SIGSEGV handlingOtherwise, this makesRegisterSignalHandler()behave differentlyfor
SIGSEGVthan it does for all other signals.Encoding thereset_handlerbit as part of the function pointer valueis a bit of a hack, and may not work on all platforms.
(This commit can be left out, if people feel like it should be. It can also be replaced by not making thereset_handlerargument part of the public API.)src: forbid reset_handler for SIGSEGV handling
This is not easily implementable, and should be explicitly disallowed.
test: add addon tests for
RegisterSignalHandler()Ensure coverage for the different combinations of arguments.
Refs: #27246
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes