From 97ea9f05d4a6e59a95ee82b30136a3f86290799e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 20 May 2019 01:55:27 +0200 Subject: [PATCH 1/4] 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/node.h | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/node.h b/src/node.h index d8f7e3bc57503e..65997fbc7b9e87 100644 --- a/src/node.h +++ b/src/node.h @@ -66,9 +66,11 @@ #include -#ifdef __POSIX__ +// We cannot use __POSIX__ in this header because that's only defined when +// building Node.js. +#ifndef _WIN32 #include -#endif // __POSIX__ +#endif // _WIN32 #define NODE_MAKE_VERSION(major, minor, patch) \ ((major) * 0x1000 + (minor) * 0x100 + (patch)) @@ -812,7 +814,7 @@ class NODE_EXTERN AsyncResource { async_context async_context_; }; -#ifdef __POSIX__ +#ifndef _WIN32 // Register a signal handler without interrupting // any handlers that node itself needs. NODE_EXTERN @@ -821,7 +823,7 @@ void RegisterSignalHandler(int signal, siginfo_t* info, void* ucontext), bool reset_handler = false); -#endif // __POSIX__ +#endif // _WIN32 } // namespace node From 543b8385126662c1e454d4d5e6847fa7180133e3 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 20 May 2019 01:57:17 +0200 Subject: [PATCH 2/4] 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/node.cc | 6 ++++++ src/node_process_methods.cc | 7 +++++++ test/abort/test-signal-handler.js | 23 +++++++++++++++++++++++ 3 files changed, 36 insertions(+) create mode 100644 test/abort/test-signal-handler.js diff --git a/src/node.cc b/src/node.cc index 388a9ea4486a88..f4a953a7c38abf 100644 --- a/src/node.cc +++ b/src/node.cc @@ -487,6 +487,12 @@ void TrapWebAssemblyOrContinue(int signo, siginfo_t* info, void* ucontext) { 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); } diff --git a/src/node_process_methods.cc b/src/node_process_methods.cc index a9ddd70bcbe2d6..912ac9bec23960 100644 --- a/src/node_process_methods.cc +++ b/src/node_process_methods.cc @@ -63,6 +63,12 @@ static void Abort(const FunctionCallbackInfo& args) { Abort(); } +// For internal testing only, not exposed to userland. +static void CauseSegfault(const FunctionCallbackInfo& args) { + // This should crash hard all platforms. + *static_cast(nullptr) = nullptr; +} + static void Chdir(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); CHECK(env->owns_process_state()); @@ -405,6 +411,7 @@ static void InitializeProcessMethods(Local 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); } diff --git a/test/abort/test-signal-handler.js b/test/abort/test-signal-handler.js new file mode 100644 index 00000000000000..f68e6881f6e5b6 --- /dev/null +++ b/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}`); +} From cdd38f43e3dce5486f24c5a5d201c4a13bc82f74 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 20 May 2019 02:03:48 +0200 Subject: [PATCH 3/4] src: forbid reset_handler for SIGSEGV handling This is not easily implementable, and should be explicitly disallowed. --- src/node.cc | 14 ++++++++------ src/node.h | 8 ++++++-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/node.cc b/src/node.cc index f4a953a7c38abf..99e6f74b472fdc 100644 --- a/src/node.cc +++ b/src/node.cc @@ -477,13 +477,15 @@ 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 - previous_sigsegv_action; +static std::atomic 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 { @@ -502,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; } diff --git a/src/node.h b/src/node.h index 65997fbc7b9e87..049690d92fb9e4 100644 --- a/src/node.h +++ b/src/node.h @@ -815,8 +815,12 @@ class NODE_EXTERN AsyncResource { }; #ifndef _WIN32 -// Register a signal handler without interrupting -// any handlers that node itself needs. +// 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, From 5dd1210f5c942bda4c120b3c952ba7e86de02841 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 20 May 2019 02:05:20 +0200 Subject: [PATCH 4/4] test: add addon tests for `RegisterSignalHandler()` Ensure coverage for the different combinations of arguments. --- .../test-addon-register-signal-handler.js | 7 +++ .../addons/register-signal-handler/binding.cc | 36 ++++++++++++ .../register-signal-handler/binding.gyp | 9 +++ test/addons/register-signal-handler/test.js | 56 +++++++++++++++++++ 4 files changed, 108 insertions(+) create mode 100644 test/abort/test-addon-register-signal-handler.js create mode 100644 test/addons/register-signal-handler/binding.cc create mode 100644 test/addons/register-signal-handler/binding.gyp create mode 100644 test/addons/register-signal-handler/test.js diff --git a/test/abort/test-addon-register-signal-handler.js b/test/abort/test-addon-register-signal-handler.js new file mode 100644 index 00000000000000..63ce4e1dbe38c5 --- /dev/null +++ b/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'); diff --git a/test/addons/register-signal-handler/binding.cc b/test/addons/register-signal-handler/binding.cc new file mode 100644 index 00000000000000..01580578d159c4 --- /dev/null +++ b/test/addons/register-signal-handler/binding.cc @@ -0,0 +1,36 @@ +#ifndef _WIN32 +#include +#include +#include +#include +#include + +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& args) { + assert(args[0]->IsInt32()); + assert(args[1]->IsBoolean()); + + int32_t signo = args[0].As()->Value(); + bool reset_handler = args[1].As()->Value(); + + node::RegisterSignalHandler(signo, Handler, reset_handler); +} + +NODE_MODULE_INIT() { + NODE_SET_METHOD(exports, "registerSignalHandler", RegisterSignalHandler); +} + +#endif // _WIN32 diff --git a/test/addons/register-signal-handler/binding.gyp b/test/addons/register-signal-handler/binding.gyp new file mode 100644 index 00000000000000..55fbe7050f18e4 --- /dev/null +++ b/test/addons/register-signal-handler/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'sources': [ 'binding.cc' ], + 'includes': ['../common.gypi'], + } + ] +} diff --git a/test/addons/register-signal-handler/test.js b/test/addons/register-signal-handler/test.js new file mode 100644 index 00000000000000..f56bb2f411024c --- /dev/null +++ b/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); + } +}