Do not let PHP-FPM children miss SIGTERM, SIGQUIT #4836
Conversation
Postpone signal delivery while spawning children. Prevent the following case: - Reload (reexec) is in progress. - New master is forking to start enough children for pools where `pm` is not `on-demand`. - Another `SIGUSR2` is received by the master process. - Master process switches to reloading state. - Some child has not set its own signal handlers. - `SIGQUIT` and `SIGTERM` sent by master process are caught by signal handler set by master process and so they are ignored. - A child is running, it has no reason to finish Before pull request php#4465 this scenario could cause deadlock, however with 0ed6c37 reload finishes after `SIGKILL`. Use sigprocmask() around fork() to avoid race of delivery signal to children and setting of own signal handlers. Fixes bug #76601
|
I am unsure that it is possible to create a test with high enough probability of failure without these changes since #4465 is merged. Maybe fpm log could be checked for |
|
I have added a test that relies on relation of |
sapi/fpm/fpm/fpm_signals.c
Outdated
| @@ -276,6 +282,12 @@ int fpm_signals_init_mask(int *signum_array, size_t size) /* {{{ */ | |||
| return -1; | |||
| } | |||
| } | |||
| memcpy(&child_block_sigset, &block_sigset, sizeof(block_sigset)); | |||
There was a problem hiding this comment.
I think this is technically undefined (or at least not recommended for portability) as sigset_t should be used only with specified operations. I would prefer to set it in the loop above instead - using the same operations as for block_sigset.
| @@ -276,6 +282,12 @@ int fpm_signals_init_mask(int *signum_array, size_t size) /* {{{ */ | |||
| return -1; | |||
| } | |||
| } | |||
| memcpy(&child_block_sigset, &block_sigset, sizeof(block_sigset)); | |||
| if (0 > sigaddset(&child_block_sigset, SIGTERM) || | |||
There was a problem hiding this comment.
What's the actually the reason specify one part (block_sigset) from parameter provided in fpm_main.c and another part (child_block_sigset extra signals) specified directly in this function? I think we should either have everything in the parameters (meaning extra child signals to block array paramter) or just have everything defined in fpm_signals which might make more sense.
sapi/fpm/fpm/fpm_signals.c
Outdated
| @@ -167,7 +168,8 @@ static void sig_handler(int signo) /* {{{ */ | |||
|
|
|||
| if (fpm_globals.parent_pid != getpid()) { | |||
| /* prevent a signal race condition when child process | |||
| have not set up it's own signal handler yet */ | |||
| do not set up it's own sigprocmask for some reason, | |||
There was a problem hiding this comment.
few typos (grammar mistakes) in here. Maybe something like:
prevent a signal race condition if a child process does not set
its own sigprocmask up which leads to #76601.
Avoid list of signals to block in `fpm_main.c`. It had some sense before explicit signals for children control were added to `fpm_signals_init_mask()`.
There is no guarantee that it could not cause undefined behavior.
|
Merged as e37bd5d . Thanks! |
Follow up of #4471.
Postpone signal delivery while spawning children.
Prevent the following case:
where
pmis noton-demand.SIGUSR2is received by the master process.SIGQUITandSIGTERMsent by master process are caughtby signal handler set by master process and so they are ignored.
Before pull request #4465 this scenario could cause deadlock,
however with 0ed6c37 reload finishes after
SIGKILL.Use sigprocmask() around fork() to avoid race of delivery signal to children
and setting of own signal handlers.
Fixes bug #76601