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);
+ }
+}