unix: reset signal disposition before execve() #1376
Conversation
|
Would it make sense adding an option so the user can decide what behavior it prefers (just in case there are people relying in the current behavior)? |
|
Yes. I was going to propose that if people weren't on board with this. |
|
+1. We've been carrying a similar patch in Julia for years. |
|
@libuv/collaborators Can I get a review? Your opinion on whether or not to add a flag is welcome too. (I'm currently leaning towards not adding a flag but happy to do it if everyone feels it's a good idea.) |
| } | ||
|
|
||
| /* Reset signal mask. */ | ||
| sigemptyset(&set); |
There was a problem hiding this comment.
should we check for sigemptyset() return value?
There was a problem hiding this comment.
Wups, sorry Santi, missed your comment. I don't mind adding that but it's probably academic, I'm not aware of any platforms where sigemptyset() can actually fail. What would it even mean?
There was a problem hiding this comment.
No problem, I was also surprised when looking at the manual that it could fail
Signal dispositions are inherited by child processes. Libuv itself does not touch them (if you don't use uv_signal_start(), that is) but the embedder might and probably does in the case of SIGPIPE. Reset the disposition for signals 1-31 to their defaults right before execve'ing into the new process. Fixes: nodejs/node#13662 PR-URL: libuv#1376 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Like the previous commit, except now the signal mask is reset instead of the signal disposition. This does open a race window where blocked signals can get delivered in the interval between the pthread_sigmask() call and the execve() call (and may end up terminating the process) but that cannot be helped; the same caveat applies to the previous commit. Fixes: nodejs/node#13662 PR-URL: libuv#1376 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
|
Rebase + new CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/308/ - all green and landed in d0a27ba...11563e1. Just to be on the safe side I did a local integration test with node.js and all was green there too. |
Signal dispositions are inherited by child processes. Libuv itself
does not touch them (if you don't use uv_signal_start(), that is)
but the embedder might and probably does in the case of SIGPIPE.
Reset the disposition for signals 1-31 to their defaults right before
execve'ing into the new process.
But caveat emptor:
Fixes: nodejs/node#13662
CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/294/