src: fix segfault handling/RegisterSignalHandler #27775

addaleax · 2019-05-20T01:40:44Z

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 handling~~

Otherwise, this makes RegisterSignalHandler() behave differently
for SIGSEGV than it does for all other signals.

Encoding the reset_handler bit as part of the function pointer value
is 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 the reset_handler argument 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), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

src/node.cc

test/addons/register-signal-handler/binding.cc

bnoordhuis · 2019-05-20T11:31:30Z

test/addons/register-signal-handler/test.js

+    // with ALLOW_CRASHES set.
+    if (signal !== null && !process.env.ALLOW_CRASHES)
+      continue;
+    // reset_handler does not work on FreeBSD.


This can be removed when #27780 lands, I think?

Correct, although I would feel more comfortable with (partially) backporting #27246 and this PR to v12.x (without enabling the WASM handler), while I wouldn’t necessarily do that with #27780 because it removes support for a platform, even if we have already officially stopped supporting it…

addaleax · 2019-05-30T21:37:25Z

@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…

Trott · 2019-06-01T10:09:42Z

@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.

addaleax · 2019-06-09T20:32:34Z

Okay, updated with the third commit changed to forbid the SIGSEGV + reset_handler = true combination – I don’t see a good, cross-platform way to implement that, and fixing the issue here is actually pretty important, so I don’t want the PR lying around unused any longer.

@jasnell @bnoordhuis Could you take another look and make sure your reviews still stand?

Trott · 2019-06-13T03:06:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23849/

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.

addaleax · 2019-06-14T17:29:23Z

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

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 <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This is not easily implementable, and should be explicitly disallowed. PR-URL: #27775 Refs: #27246 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Ensure coverage for the different combinations of arguments. PR-URL: #27775 Refs: #27246 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

targos · 2019-07-02T07:02:44Z

@addaleax what was the reason for dont-land-on-v12.x?