New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: note that listening on SIGSEGV & co is unsafe #8410

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@addaleax
Member

addaleax commented Sep 5, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Note that trying to listen for some signals using process.on() is unsafe in the process docs.

doc: note that listening on SIGSEGV & co is unsafe
Note that trying to listen for some signals using `process.on()`
is unsafe in the `process` docs.

addaleax added a commit to addaleax/signal-exit that referenced this pull request Sep 5, 2016

Do not listen on SIGBUS, SIGFPE, SIGSEGV and SIGILL
Listening for one of these signals from JS will make the process
enter an infinite loop when encountering them naturally
because the underlying problem is not resolved while the signal
handler is being scheduled.

Ref: npm/npm#13782
Ref: nodejs/node#8410
@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Sep 5, 2016

Member

LGTM

Member

bnoordhuis commented Sep 5, 2016

LGTM

Show outdated Hide outdated doc/api/process.md
@@ -422,6 +422,9 @@ It is important to take note of the following:
* `SIGKILL` cannot have a listener installed, it will unconditionally terminate
Node.js on all platforms.
* `SIGSTOP` cannot have a listener installed.
* `SIGBUS`, `SIGFPE`, `SIGSEGV` and `SIGILL`, when not raised artificially,
inherently leave the process in a state from which it is not safe to
attempt to call JS listeners.

This comment has been minimized.

@Fishrock123

Fishrock123 Sep 5, 2016

Member

Does core not call listeners then...?

@Fishrock123

Fishrock123 Sep 5, 2016

Member

Does core not call listeners then...?

This comment has been minimized.

@addaleax

addaleax Sep 5, 2016

Member

libuv schedules them to run, but the control flow returns from the signal handler before actually calling anything. As far as I have been able to tell, that means that the offending code will just be executed again, triggering the signal again, etc. in an infinite loop.

@addaleax

addaleax Sep 5, 2016

Member

libuv schedules them to run, but the control flow returns from the signal handler before actually calling anything. As far as I have been able to tell, that means that the offending code will just be executed again, triggering the signal again, etc. in an infinite loop.

This comment has been minimized.

@addaleax

addaleax Sep 5, 2016

Member

@Fishrock123 sorry, occurs to me that I might have misread your question – node does try to call the listeners in the usual way, and that works perfectly when the signal is artificially generated, as in process.kill(process.pid, 'SIGSEGV').

@addaleax

addaleax Sep 5, 2016

Member

@Fishrock123 sorry, occurs to me that I might have misread your question – node does try to call the listeners in the usual way, and that works perfectly when the signal is artificially generated, as in process.kill(process.pid, 'SIGSEGV').

This comment has been minimized.

@Fishrock123

Fishrock123 Sep 5, 2016

Member

and that works perfectly when the signal is artificially generated,

Does that imply that we should not allow these signals to trigger when they are not artificial?

@Fishrock123

Fishrock123 Sep 5, 2016

Member

and that works perfectly when the signal is artificially generated,

Does that imply that we should not allow these signals to trigger when they are not artificial?

This comment has been minimized.

@addaleax

addaleax Sep 5, 2016

Member

Does that imply that we should not allow these signals to trigger when they are not artificial?

We might be able to do that… libuv would need to set SA_SIGINFO, which comes with this warning in the Linux manpage:

Use of these latter values in sa_flags may be less portable in applications intended for older UNIX implementations.

I am not sure what practical use cases for manually triggering SIGSEGV there are, though.

@addaleax

addaleax Sep 5, 2016

Member

Does that imply that we should not allow these signals to trigger when they are not artificial?

We might be able to do that… libuv would need to set SA_SIGINFO, which comes with this warning in the Linux manpage:

Use of these latter values in sa_flags may be less portable in applications intended for older UNIX implementations.

I am not sure what practical use cases for manually triggering SIGSEGV there are, though.

This comment has been minimized.

@Fishrock123

Fishrock123 Sep 6, 2016

Member

@addaleax I mean, should these listeners even function if it is not safe?

@Fishrock123

Fishrock123 Sep 6, 2016

Member

@addaleax I mean, should these listeners even function if it is not safe?

This comment has been minimized.

@addaleax

addaleax Sep 6, 2016

Member

@Fishrock123 I would say no, but it’s probably not something worth the trouble of removing…

@addaleax

addaleax Sep 6, 2016

Member

@Fishrock123 I would say no, but it’s probably not something worth the trouble of removing…

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Sep 5, 2016

Contributor

LGTM

Contributor

cjihrig commented Sep 5, 2016

LGTM

Show outdated Hide outdated doc/api/process.md
@@ -422,6 +422,9 @@ It is important to take note of the following:
* `SIGKILL` cannot have a listener installed, it will unconditionally terminate
Node.js on all platforms.
* `SIGSTOP` cannot have a listener installed.
* `SIGBUS`, `SIGFPE`, `SIGSEGV` and `SIGILL`, when not raised artificially,

This comment has been minimized.

@jasnell

jasnell Sep 7, 2016

Member

raised artificially is a bit obscure. I'm not sure the average reader will know that that means.

@jasnell

jasnell Sep 7, 2016

Member

raised artificially is a bit obscure. I'm not sure the average reader will know that that means.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 7, 2016

Member

While this is ok in general, I think it would be beneficial to describe the risk a bit more.

Member

jasnell commented Sep 7, 2016

While this is ok in general, I think it would be beneficial to describe the risk a bit more.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Sep 7, 2016

Member

It seems fine to me as is, really. You could s/raised artificially/sent explicitly/ but that's not even much of an improvement, IMO.

Member

bnoordhuis commented Sep 7, 2016

It seems fine to me as is, really. You could s/raised artificially/sent explicitly/ but that's not even much of an improvement, IMO.

addaleax added some commits Sep 8, 2016

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Sep 8, 2016

Member

I’ve added a bit more explanation on what might happen and a reference to the kill(2) manpage, hopefully that clears things up a bit… idk, the language around signals is usually based on raise as a verb.

Member

addaleax commented Sep 8, 2016

I’ve added a bit more explanation on what might happen and a reference to the kill(2) manpage, hopefully that clears things up a bit… idk, the language around signals is usually based on raise as a verb.

bcoe added a commit to tapjs/signal-exit that referenced this pull request Sep 8, 2016

fix: do not listen on SIGBUS, SIGFPE, SIGSEGV and SIGILL (#40)
Listening for one of these signals from JS will make the process
enter an infinite loop when encountering them naturally
because the underlying problem is not resolved while the signal
handler is being scheduled.

Ref: npm/npm#13782
Ref: nodejs/node#8410
@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Sep 8, 2016

Member

seems fine to me then

Member

Fishrock123 commented Sep 8, 2016

seems fine to me then

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 8, 2016

Member

LGTM. I appreciate the tweaks.

Member

jasnell commented Sep 8, 2016

LGTM. I appreciate the tweaks.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Sep 9, 2016

Contributor

LGTM

Contributor

cjihrig commented Sep 9, 2016

LGTM

jasnell added a commit that referenced this pull request Sep 12, 2016

doc: note that listening on SIGSEGV & co is unsafe
Note that trying to listen for some signals using `process.on()`
is unsafe in the `process` docs.

PR-URL: #8410
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 12, 2016

Member

Landed in 3207ea4

Member

jasnell commented Sep 12, 2016

Landed in 3207ea4

@jasnell jasnell closed this Sep 12, 2016

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

doc: note that listening on SIGSEGV & co is unsafe
Note that trying to listen for some signals using `process.on()`
is unsafe in the `process` docs.

PR-URL: #8410
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@AdriVanHoudt AdriVanHoudt referenced this pull request Sep 17, 2016

Closed

internal: changed var to const in linkedlist #8609

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment