Raise a warning when listening for usafe signals #8601

quibusus · 2016-09-17T09:47:07Z

Checklist

[x ] make -j4 test (UNIX)
[x ] tests and/or benchmarks are included
[doc: note that listening on SIGSEGV & co is unsafe #8410] documentation is changed or added
[ x] commit message follows commit guidelines

Description of change

We might want to emit a warning for signals that users shouldn’t listen on (as listed in the docs warning in #8410)

cjihrig · 2016-09-17T13:41:25Z

I don't think this is something we necessarily need to be doing.

MylesBorins · 2016-09-17T14:07:17Z

We need to be very careful about adding extra warnings, they have proven to be unexpectadly fragile in the past

Fishrock123

Like the idea, impl could use some tweaks

Fishrock123 · 2016-09-17T17:58:48Z

test/parallel/test-process-unsafe-signals.js

+
+});
+
+


remove all but one newline here please

Fishrock123 · 2016-09-17T17:58:56Z

test/parallel/test-process-safe-signals.js

@@ -0,0 +1,12 @@
+'use strict';
+require('../common');
+var assert = require('assert');


Fishrock123 · 2016-09-17T17:59:41Z

test/parallel/test-process-safe-signals.js

+require('../common');
+var assert = require('assert');
+
+process.on('warning', function(warning) {


Please wrap the warning listeners in common.mustCall() in both tests.

Fishrock123 · 2016-09-17T18:01:19Z

lib/internal/process.js

@@ -191,6 +191,11 @@ function setupSignalHandlers() {
  process.on('newListener', function(type, listener) {
    if (isSignal(type) &&
        !signalWraps.hasOwnProperty(type)) {
+
+      if (['SIGBUS', 'SIGFPE', 'SIGSEGV', 'SIGILL'].includes(type)) {
+        process.emitWarning('SIGBUS, SIGFPE, SIGSEGV and SIGILL are not safe');


The message could probably be a bit more descriptive.

e.g.

Listening to X, X, etc signals are not safe and the listener may be called in an infinite loop

or something

addaleax · 2016-09-17T18:37:08Z

We need to be very careful about adding extra warnings, they have proven to be unexpectadly fragile in the past

I’d be pretty okay with this not ending up in v6 or v7. People who are using non-up-to-date versions of signal-exit (6M downloads/month) would get a warning, so giving it a bit of time would probably be nice. (I’m adding the labels but if people’s opinions differ they may feel free to remove it).

Getting told that listening on SIGSEGV is not safe may be a huge debugging help when the problems actually arise, though.

quibusus · 2016-09-18T20:01:18Z

Thank you guys for the feedback, changes are ready for review

cjihrig · 2016-09-18T20:03:57Z

lib/internal/process.js

@@ -191,6 +191,14 @@ function setupSignalHandlers() {
  process.on('newListener', function(type, listener) {
    if (isSignal(type) &&
        !signalWraps.hasOwnProperty(type)) {
+
+      if (['SIGBUS', 'SIGFPE', 'SIGSEGV', 'SIGILL'].includes(type)) {


It may be faster to use a Map or something similar here.

Indeed, indexOf is 5-10x faster in this case

cjihrig · 2016-09-18T20:04:46Z

test/parallel/test-process-safe-signals.js

+
+}, 0));
+
+process.on('SIGINT', () => {


Please write as process.on('SIGINT', () => {});

cjihrig · 2016-09-18T20:05:41Z

test/parallel/test-process-safe-signals.js

+'use strict';
+const common = require('../common');
+
+process.on('warning', common.mustCall((warning) => {


process.on('warning', common.fail);

cjihrig · 2016-09-18T20:06:21Z

test/parallel/test-process-unsafe-signals.js

+               'loop');
+}, 1));
+
+process.on('SIGBUS', () => {


Same comment regarding whitespace/formatting here.

cjihrig · 2016-09-18T20:06:33Z

test/parallel/test-process-unsafe-signals.js

+               'Listening to SIGBUS, SIGFPE, SIGSEGV, SIGILL signals ' +
+               'is not safe and the listener may be called in an infinite ' +
+               'loop');
+}, 1));


No need for the , 1

cjihrig · 2016-09-18T20:06:52Z

test/parallel/test-process-unsafe-signals.js

+const assert = require('assert');
+
+process.on('warning', common.mustCall((warning) => {
+  assert.equal(warning.name, 'Warning');


Please use assert.strictEqual().

bnoordhuis · 2016-09-19T07:54:49Z

Is this actually an issue? If people want to listen for those signals, they presumably have their reasons. If we're printing warnings like this we should rename the project to Nanny.js.

addaleax · 2016-09-19T12:34:27Z

Is this actually an issue? If people want to listen for those signals, they presumably have their reasons.

I don’t know if you count that as “actually an issue”, but yes, signal-exit did that in the past, and an application using it would just hang when receiving a SIGSEGV (which actually occurred with npm). I’m not sure what other modules do that, but I wouldn’t want to remote-debug something like that again, because it’s really unclear what the underlying problem is when looking at the symptoms.

quibusus · 2016-09-19T21:53:59Z

I think this is a good to have feature. It will make using node safer with very little overhead.

Fishrock123 · 2016-09-19T21:58:28Z

So, this only effects on listeners and not once listeners, right?

Is there a way to differentiate?

jasnell

LGTM

jasnell · 2016-09-20T18:45:32Z

Given the increased possibility of breaking things that adding warnings has had in the past, I'm marking this as semver-minor. It would likely be good to have a citgm run on this just in case.

addaleax · 2016-09-24T21:52:24Z

@quibusus Sorry, this seems to have merge conflicts… could you rebase this against master?

quibusus · 2016-09-24T22:23:39Z

@Fishrock123 - all changes are done
@addaleax - rebased onto nodejs/node/master

how about the once listeners mentioned by @Fishrock123 ? should this be covered too ?

vkurchatkin · 2016-09-24T23:39:15Z

-1, either this should be a proper deprecation or no warning at all. There are a lot of potentially unsafe things that we don't warn about, I don't think that we should start

Fishrock123 · 2016-09-24T23:41:11Z

If I understand correctly, this is safe if you use EventEmitter#once? I don't think there is a good way to detect that unfortunately.

addaleax · 2016-09-25T09:29:28Z

If I understand correctly, this is safe if you use EventEmitter#once? I don't think there is a good way to detect that unfortunately.

It isn’t, and the current code does warn when #once is being used, too.

jasnell · 2017-08-24T23:29:59Z

Closing due to lack of forward progress. We can reopen if necessary

nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Sep 17, 2016

Fishrock123 suggested changes Sep 17, 2016

View reviewed changes

addaleax added dont-land-on-v4.x labels Sep 17, 2016

addaleax mentioned this pull request Sep 17, 2016

Raise a warning when listening for usafe signals #8591

Closed

quibusus force-pushed the master branch 2 times, most recently from 9654c2a to 4544667 Compare September 18, 2016 19:52

quibusus changed the title ~~Raise a warning when listening for usafe signals #8591~~ Raise a warning when listening for usafe signals Sep 18, 2016

cjihrig reviewed Sep 18, 2016

View reviewed changes

quibusus force-pushed the master branch from 4544667 to 8c6b95c Compare September 19, 2016 21:51

jasnell approved these changes Sep 20, 2016

View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 20, 2016

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:10

Raise a warning when listening for usafe signals nodejs#8601

ecd5244

quibusus force-pushed the master branch from 8c6b95c to ecd5244 Compare September 24, 2016 22:20

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

jasnell closed this Aug 24, 2017

addaleax mentioned this pull request Jan 28, 2019

SIGSEGV handler causes infinite loop if signal raised from within #25762

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise a warning when listening for usafe signals #8601

Raise a warning when listening for usafe signals #8601

quibusus commented Sep 17, 2016

cjihrig commented Sep 17, 2016

MylesBorins commented Sep 17, 2016

Fishrock123 left a comment

Fishrock123 Sep 17, 2016

Fishrock123 Sep 17, 2016

Fishrock123 Sep 17, 2016

Fishrock123 Sep 17, 2016 •

edited

Loading

addaleax commented Sep 17, 2016

quibusus commented Sep 18, 2016

cjihrig Sep 18, 2016

quibusus Sep 19, 2016

cjihrig Sep 18, 2016

cjihrig Sep 18, 2016

cjihrig Sep 18, 2016

cjihrig Sep 18, 2016

cjihrig Sep 18, 2016

bnoordhuis commented Sep 19, 2016

addaleax commented Sep 19, 2016

quibusus commented Sep 19, 2016

Fishrock123 commented Sep 19, 2016

jasnell left a comment

jasnell commented Sep 20, 2016

addaleax commented Sep 24, 2016

quibusus commented Sep 24, 2016

vkurchatkin commented Sep 24, 2016

Fishrock123 commented Sep 24, 2016

addaleax commented Sep 25, 2016

jasnell commented Aug 24, 2017


		});


		}, 0));

		process.on('SIGINT', () => {

Raise a warning when listening for usafe signals #8601

Raise a warning when listening for usafe signals #8601

Conversation

quibusus commented Sep 17, 2016

Checklist

Description of change

cjihrig commented Sep 17, 2016

MylesBorins commented Sep 17, 2016

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 Sep 17, 2016 • edited Loading

Choose a reason for hiding this comment

addaleax commented Sep 17, 2016

quibusus commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Sep 19, 2016

addaleax commented Sep 19, 2016

quibusus commented Sep 19, 2016

Fishrock123 commented Sep 19, 2016

jasnell left a comment

Choose a reason for hiding this comment

jasnell commented Sep 20, 2016

addaleax commented Sep 24, 2016

quibusus commented Sep 24, 2016

vkurchatkin commented Sep 24, 2016

Fishrock123 commented Sep 24, 2016

addaleax commented Sep 25, 2016

jasnell commented Aug 24, 2017

Fishrock123 Sep 17, 2016 •

edited

Loading