src: fix segfault handling/RegisterSignalHandler

src/node.cc

@@ -477,16 +477,24 @@ void LoadEnvironment(Environment* env) {
   USE(StartMainThreadExecution(env));
 }
 
+#ifdef __POSIX__
+typedef void (*sigaction_cb)(int signo, siginfo_t* info, void* ucontext);
+#endif
 #if NODE_USE_V8_WASM_TRAP_HANDLER
-static std::atomic<void (*)(int signo, siginfo_t* info, void* ucontext)>
-    previous_sigsegv_action;
+static std::atomic<sigaction_cb> previous_sigsegv_action;
 
 void TrapWebAssemblyOrContinue(int signo, siginfo_t* info, void* ucontext) {
   if (!v8::TryHandleWebAssemblyTrapPosix(signo, info, ucontext)) {
-    auto prev = previous_sigsegv_action.load();
+    sigaction_cb prev = previous_sigsegv_action.load();
     if (prev != nullptr) {
       prev(signo, info, ucontext);
     } else {
+      // Reset to the default signal handler, i.e. cause a hard crash.
+      struct sigaction sa;
+      memset(&sa, 0, sizeof(sa));
+      sa.sa_handler = SIG_DFL;
+      CHECK_EQ(sigaction(signo, &sa, nullptr), 0);
+
       ResetStdio();
       raise(signo);
     }
@@ -496,13 +504,13 @@ void TrapWebAssemblyOrContinue(int signo, siginfo_t* info, void* ucontext) {
 
 #ifdef __POSIX__
 void RegisterSignalHandler(int signal,
-                           void (*handler)(int signal,
-                                           siginfo_t* info,
-                                           void* ucontext),
+                           sigaction_cb handler,
                            bool reset_handler) {
+  CHECK_NOT_NULL(handler);
 #if NODE_USE_V8_WASM_TRAP_HANDLER
   if (signal == SIGSEGV) {
     CHECK(previous_sigsegv_action.is_lock_free());
+    CHECK(!reset_handler);
     previous_sigsegv_action.store(handler);
     return;
   }

src/node.h

@@ -66,9 +66,11 @@
 
 #include <memory>
 
-#ifdef __POSIX__
+// We cannot use __POSIX__ in this header because that's only defined when
+// building Node.js.
+#ifndef _WIN32
 #include <signal.h>
-#endif  // __POSIX__
+#endif  // _WIN32
 
 #define NODE_MAKE_VERSION(major, minor, patch)                                \
   ((major) * 0x1000 + (minor) * 0x100 + (patch))
@@ -812,16 +814,20 @@ class NODE_EXTERN AsyncResource {
   async_context async_context_;
 };
 
-#ifdef __POSIX__
-// Register a signal handler without interrupting
-// any handlers that node itself needs.
+#ifndef _WIN32
+// Register a signal handler without interrupting any handlers that node
+// itself needs. This does override handlers registered through
+// process.on('SIG...', function() { ... }). The `reset_handler` flag indicates
+// whether the signal handler for the given signal should be reset to its
+// default value before executing the handler (i.e. it works like SA_RESETHAND).
+// The `reset_handler` flag is invalid when `signal` is SIGSEGV.
 NODE_EXTERN
 void RegisterSignalHandler(int signal,
                            void (*handler)(int signal,
                                            siginfo_t* info,
                                            void* ucontext),
                            bool reset_handler = false);
-#endif  // __POSIX__
+#endif  // _WIN32
 
 }  // namespace node
 

src/node_process_methods.cc

@@ -63,6 +63,12 @@ static void Abort(const FunctionCallbackInfo<Value>& args) {
   Abort();
 }
 
+// For internal testing only, not exposed to userland.
+static void CauseSegfault(const FunctionCallbackInfo<Value>& args) {
+  // This should crash hard all platforms.
+  *static_cast<void**>(nullptr) = nullptr;
+}
+
 static void Chdir(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   CHECK(env->owns_process_state());
@@ -405,6 +411,7 @@ static void InitializeProcessMethods(Local<Object> target,
     env->SetMethod(target, "_debugProcess", DebugProcess);
     env->SetMethod(target, "_debugEnd", DebugEnd);
     env->SetMethod(target, "abort", Abort);
+    env->SetMethod(target, "causeSegfault", CauseSegfault);
     env->SetMethod(target, "chdir", Chdir);
   }
 

test/abort/test-addon-register-signal-handler.js

@@ -0,0 +1,7 @@
+'use strict';
+require('../common');
+
+// This is a sibling test to test/addons/register-signal-handler/
+
+process.env.ALLOW_CRASHES = true;
+require('../addons/register-signal-handler/test');

test/abort/test-signal-handler.js

@@ -0,0 +1,23 @@
+'use strict';
+const common = require('../common');
+if (common.isWindows)
+  common.skip('No signals on Window');
+
+const assert = require('assert');
+const { spawnSync } = require('child_process');
+
+// Test that a hard crash does not cause an endless loop.
+
+if (process.argv[2] === 'child') {
+  const { internalBinding } = require('internal/test/binding');
+  const { causeSegfault } = internalBinding('process_methods');
+
+  causeSegfault();
+} else {
+  const child = spawnSync(process.execPath,
+                          ['--expose-internals', __filename, 'child'],
+                          { stdio: 'inherit' });
+  // FreeBSD and macOS use SIGILL for the kind of crash we're causing here.
+  assert(child.signal === 'SIGSEGV' || child.signal === 'SIGILL',
+         `child.signal = ${child.signal}`);
+}

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

@@ -0,0 +1,36 @@
+#ifndef _WIN32
+#include <node.h>
+#include <v8.h>
+#include <uv.h>
+#include <assert.h>
+#include <unistd.h>
+
+using v8::Boolean;
+using v8::FunctionCallbackInfo;
+using v8::Int32;
+using v8::Value;
+
+void Handler(int signo, siginfo_t* siginfo, void* ucontext) {
+  char signo_char = signo;
+  int written;
+  do {
+    written = write(1, &signo_char, 1);  // write() is signal-safe.
+  } while (written == -1 && errno == EINTR);
+  assert(written == 1);
+}
+
+void RegisterSignalHandler(const FunctionCallbackInfo<Value>& args) {
+  assert(args[0]->IsInt32());
+  assert(args[1]->IsBoolean());
+
+  int32_t signo = args[0].As<Int32>()->Value();
+  bool reset_handler = args[1].As<Boolean>()->Value();
+
+  node::RegisterSignalHandler(signo, Handler, reset_handler);
+}
+
+NODE_MODULE_INIT() {
+  NODE_SET_METHOD(exports, "registerSignalHandler", RegisterSignalHandler);
+}
+
+#endif  // _WIN32

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

@@ -0,0 +1,9 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'sources': [ 'binding.cc' ],
+      'includes': ['../common.gypi'],
+    }
+  ]
+}

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

@@ -0,0 +1,56 @@
+'use strict';
+const common = require('../../common');
+if (common.isWindows)
+  common.skip('No RegisterSignalHandler() on Windows');
+
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const { signals } = require('os').constants;
+const { spawnSync } = require('child_process');
+
+const bindingPath = path.resolve(
+  __dirname, 'build', common.buildType, 'binding.node');
+
+if (!fs.existsSync(bindingPath))
+  common.skip('binding not built yet');
+
+const binding = require(bindingPath);
+
+if (process.argv[2] === 'child') {
+  const signo = +process.argv[3];
+  const reset = process.argv[4] === 'reset';
+  const count = +process.argv[5];
+
+  binding.registerSignalHandler(signo, reset);
+  for (let i = 0; i < count; i++)
+    process.kill(process.pid, signo);
+  return;
+}
+
+for (const raiseSignal of [ 'SIGABRT', 'SIGSEGV' ]) {
+  const signo = signals[raiseSignal];
+  for (const { reset, count, stderr, code, signal } of [
+    { reset: true, count: 1, stderr: [signo], code: 0, signal: null },
+    { reset: true, count: 2, stderr: [signo], code: null, signal: raiseSignal },
+    { reset: false, count: 1, stderr: [signo], code: 0, signal: null },
+    { reset: false, count: 2, stderr: [signo, signo], code: 0, signal: null }
+  ]) {
+    // We do not want to generate core files when running this test as an
+    // addon test. We require this file as an abort test as well, though,
+    // with ALLOW_CRASHES set.
+    if (signal !== null && !process.env.ALLOW_CRASHES)
+      continue;
+    // reset_handler does not work with SIGSEGV.
+    if (reset && signo === signals.SIGSEGV)
+      continue;
+
+    const args = [__filename, 'child', signo, reset ? 'reset' : '', count];
+    console.log(`Running: node ${args.join(' ')}`);
+    const result = spawnSync(
+      process.execPath, args, { stdio: ['inherit', 'pipe', 'inherit'] });
+    assert.strictEqual(result.status, code);
+    assert.strictEqual(result.signal, signal);
+    assert.deepStrictEqual([...result.stdout], stderr);
+  }
+}