From a4ca83cc3332f9b1f3a14687e1baa5f9d9f5d89d Mon Sep 17 00:00:00 2001 From: Maksim Nikulin Date: Wed, 24 Jul 2019 16:50:57 +0700 Subject: [PATCH 1/3] Block signals during fpm master initialization Fix PHP-FPM failure in the case of concurrent reload attempts. Postpone signal delivery to the fpm master process till proper signal handlers are set. Prevent the following case: - Running master process receives `SIGUSR2` and performs `execvp()`. - Another `SIGUSR2` is arrived before signal handlers are set. - Master process dies. - Requests to the HTTP server handled by PHP-fpm can not be served any more. Block some signals using `sigprocmask()` before `execvp()` and early in the `main()` function. Unblock signals as soon as proper handlers are set. Fixes bug #74083 --- sapi/fpm/fpm/fpm_main.c | 13 +++++++++ sapi/fpm/fpm/fpm_process_ctl.c | 4 +++ sapi/fpm/fpm/fpm_signals.c | 53 ++++++++++++++++++++++++++++++++++ sapi/fpm/fpm/fpm_signals.h | 3 ++ 4 files changed, 73 insertions(+) diff --git a/sapi/fpm/fpm/fpm_main.c b/sapi/fpm/fpm/fpm_main.c index f0cc3a07a485c..e70aca7de3502 100644 --- a/sapi/fpm/fpm/fpm_main.c +++ b/sapi/fpm/fpm/fpm_main.c @@ -109,6 +109,10 @@ int __riscosify_control = __RISCOSIFY_STRICT_UNIX_SPECS; #include "fpm_log.h" #include "zlog.h" +#if HAVE_SIGNAL_H +# include "fpm_signals.h" +#endif + #ifndef PHP_WIN32 /* XXX this will need to change later when threaded fastcgi is implemented. shane */ struct sigaction act, old_term, old_quit, old_int; @@ -1604,6 +1608,15 @@ int main(int argc, char *argv[]) closes it. in apache|apxs mode apache does that for us! thies@thieso.net 20000419 */ + + /* Subset of signals from fpm_signals_init_main() to avoid unexpected death during early init + or during reload just after execvp() or fork */ + int init_signal_array[] = { SIGUSR1, SIGUSR2, SIGCHLD }; + if (0 > fpm_signals_init_mask(init_signal_array, sizeof(init_signal_array)/sizeof(init_signal_array[0])) || + 0 > fpm_signals_block()) { + zlog(ZLOG_WARNING, "Could die in the case of too early reload signal"); + } + zlog(ZLOG_DEBUG, "Blocked some signals"); #endif #endif diff --git a/sapi/fpm/fpm/fpm_process_ctl.c b/sapi/fpm/fpm/fpm_process_ctl.c index f99ec241c74eb..a9136c7465efc 100644 --- a/sapi/fpm/fpm/fpm_process_ctl.c +++ b/sapi/fpm/fpm/fpm_process_ctl.c @@ -78,6 +78,10 @@ static void fpm_pctl_exit() /* {{{ */ static void fpm_pctl_exec() /* {{{ */ { + zlog(ZLOG_DEBUG, "Blocking some signals before reexec"); + if (0 > fpm_signals_block()) { + zlog(ZLOG_WARNING, "concurrent reloads may be unstable"); + } zlog(ZLOG_NOTICE, "reloading: execvp(\"%s\", {\"%s\"" "%s%s%s" "%s%s%s" "%s%s%s" "%s%s%s" "%s%s%s" diff --git a/sapi/fpm/fpm/fpm_signals.c b/sapi/fpm/fpm/fpm_signals.c index 4c9f9664f65b7..ab9ab1d813ff1 100644 --- a/sapi/fpm/fpm/fpm_signals.c +++ b/sapi/fpm/fpm/fpm_signals.c @@ -20,6 +20,7 @@ #include "zlog.h" static int sp[2]; +static sigset_t block_sigset; const char *fpm_signal_names[NSIG + 1] = { #ifdef SIGHUP @@ -211,6 +212,11 @@ int fpm_signals_init_main() /* {{{ */ zlog(ZLOG_SYSERROR, "failed to init signals: sigaction()"); return -1; } + + zlog(ZLOG_DEBUG, "Unblocking all signals"); + if (0 > fpm_signals_unblock()) { + return -1; + } return 0; } /* }}} */ @@ -251,3 +257,50 @@ int fpm_signals_get_fd() /* {{{ */ return sp[0]; } /* }}} */ + +int fpm_signals_init_mask(int *signum_array, size_t size) /* {{{ */ +{ + size_t i = 0; + if (0 > sigemptyset(&block_sigset)) { + zlog(ZLOG_SYSERROR, "failed to prepare signal block mask: sigemptyset()"); + return -1; + } + for (i = 0; i < size; ++i) { + int sig_i = signum_array[i]; + if (0 > sigaddset(&block_sigset, sig_i)) { + if (sig_i <= NSIG && fpm_signal_names[sig_i] != NULL) { + zlog(ZLOG_SYSERROR, "failed to prepare signal block mask: sigaddset(%s)", + fpm_signal_names[sig_i]); + } else { + zlog(ZLOG_SYSERROR, "failed to prepare signal block mask: sigaddset(%d)", sig_i); + } + return -1; + } + } + return 0; +} +/* }}} */ + +int fpm_signals_block() /* {{{ */ +{ + if (0 > sigprocmask(SIG_BLOCK, &block_sigset, NULL)) { + zlog(ZLOG_SYSERROR, "failed to block signals"); + return -1; + } + return 0; +} +/* }}} */ + +int fpm_signals_unblock() /* {{{ */ +{ + /* Ensure that during reload after upgrade all signals are unblocked. + block_sigset could have different value before execve() */ + sigset_t all_signals; + sigfillset(&all_signals); + if (0 > sigprocmask(SIG_UNBLOCK, &all_signals, NULL)) { + zlog(ZLOG_SYSERROR, "failed to unblock signals"); + return -1; + } + return 0; +} +/* }}} */ diff --git a/sapi/fpm/fpm/fpm_signals.h b/sapi/fpm/fpm/fpm_signals.h index 759a4e316ca96..a18460f00ba30 100644 --- a/sapi/fpm/fpm/fpm_signals.h +++ b/sapi/fpm/fpm/fpm_signals.h @@ -9,6 +9,9 @@ int fpm_signals_init_main(); int fpm_signals_init_child(); int fpm_signals_get_fd(); +int fpm_signals_init_mask(int *signum_array, size_t size); +int fpm_signals_block(); +int fpm_signals_unblock(); extern const char *fpm_signal_names[NSIG + 1]; From 1cbea1947c88c414d2a85491c9273d00da803cad Mon Sep 17 00:00:00 2001 From: Maksim Nikulin Date: Thu, 25 Jul 2019 11:41:36 +0700 Subject: [PATCH 2/3] Add (slow) test for fpm concurrent reloads #74083 --- .../fpm/tests/bug74083-concurrent-reload.phpt | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 sapi/fpm/tests/bug74083-concurrent-reload.phpt diff --git a/sapi/fpm/tests/bug74083-concurrent-reload.phpt b/sapi/fpm/tests/bug74083-concurrent-reload.phpt new file mode 100644 index 0000000000000..532c3b10bf8e1 --- /dev/null +++ b/sapi/fpm/tests/bug74083-concurrent-reload.phpt @@ -0,0 +1,76 @@ +--TEST-- +Concurrent reload signals should not kill PHP-FPM master process. (Bug: #74083) +--SKIPIF-- + +--FILE-- +start(); +$tester->expectLogStartNotices(); +$tester->ping('{{ADDR}}'); + +/* Vary interval between concurrent reload requests + since performance of test instance is not known in advance */ +$max_interval = 25000; +$step = 1000; +$pid = $tester->getPid(); +for ($interval = 0; $interval < $max_interval; $interval += $step) { + exec("kill -USR2 $pid", $out, $killExitCode); + if ($killExitCode) { + echo "ERROR: master process is dead\n"; + break; + } + usleep($interval); +} +echo "Reached interval $interval us with $step us steps\n"; +$tester->expectLogNotice('Reloading in progress ...'); +/* Consume mix of 'Reloading in progress ...' and 'reloading: .*' */ +$tester->getLogLines(2000); + +$tester->signal('USR2'); +$tester->expectLogNotice('Reloading in progress ...'); +$tester->expectLogNotice('reloading: .*'); +$tester->expectLogNotice('using inherited socket fd=\d+, "127.0.0.1:\d+"'); +$tester->expectLogStartNotices(); +$tester->ping('{{ADDR}}'); + +$tester->terminate(); +$tester->expectLogTerminatingNotices(); +$tester->close(); + +?> +Done +--EXPECT-- +Reached interval 25000 us with 1000 us steps +Done +--CLEAN-- + From 8948a33c8fb9b3b9c46bce85572af878012cc1af Mon Sep 17 00:00:00 2001 From: Maksim Nikulin Date: Thu, 25 Jul 2019 13:15:35 +0700 Subject: [PATCH 3/3] Skip fpm bug #74083 test on Windows Have not expected side effects of `include`. --- sapi/fpm/tests/bug74083-concurrent-reload.phpt | 1 + 1 file changed, 1 insertion(+) diff --git a/sapi/fpm/tests/bug74083-concurrent-reload.phpt b/sapi/fpm/tests/bug74083-concurrent-reload.phpt index 532c3b10bf8e1..8b4b690e96201 100644 --- a/sapi/fpm/tests/bug74083-concurrent-reload.phpt +++ b/sapi/fpm/tests/bug74083-concurrent-reload.phpt @@ -2,6 +2,7 @@ Concurrent reload signals should not kill PHP-FPM master process. (Bug: #74083) --SKIPIF-- --FILE--