Do not let PHP-FPM children miss SIGTERM, SIGQUIT

sapi/fpm/fpm/fpm_children.c

@@ -404,6 +404,11 @@ int fpm_children_make(struct fpm_worker_pool_s *wp, int in_event_loop, int nb_to
 			return 2;
 		}
 
+		zlog(ZLOG_DEBUG, "blocking signals before child birth");
+		if (0 > fpm_signals_child_block()) {
+			zlog(ZLOG_WARNING, "child may miss signals");
+		}
+
 		pid = fork();
 
 		switch (pid) {
@@ -415,12 +420,16 @@ int fpm_children_make(struct fpm_worker_pool_s *wp, int in_event_loop, int nb_to
 				return 0;
 
 			case -1 :
+				zlog(ZLOG_DEBUG, "unblocking signals");
+				fpm_signals_unblock();
 				zlog(ZLOG_SYSERROR, "fork() failed");
 
 				fpm_resources_discard(child);
 				return 2;
 
 			default :
+				zlog(ZLOG_DEBUG, "unblocking signals, child born");
+				fpm_signals_unblock();
 				child->pid = pid;
 				fpm_clock_get(&child->started);
 				fpm_parent_resources_use(child);

sapi/fpm/fpm/fpm_signals.c

@@ -20,6 +20,7 @@
 
 static int sp[2];
 static sigset_t block_sigset;
+static sigset_t child_block_sigset;
 
 const char *fpm_signal_names[NSIG + 1] = {
 #ifdef SIGHUP
@@ -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,
+			leads to #76601 in such cases */
 		return;
 	}
 
@@ -247,6 +249,10 @@ int fpm_signals_init_child() /* {{{ */
 	}
 
 	zend_signal_init();
+
+	if (0 > fpm_signals_unblock()) {
+		return -1;
+	}
 	return 0;
 }
 /* }}} */
@@ -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) ||
+	    0 > sigaddset(&child_block_sigset, SIGQUIT)) {
+		zlog(ZLOG_SYSERROR, "failed to prepare child signal block mask: sigaddset()");
+		return -1;
+	}
 	return 0;
 }
 /* }}} */
@@ -290,6 +302,16 @@ int fpm_signals_block() /* {{{ */
 }
 /* }}} */
 
+int fpm_signals_child_block() /* {{{ */
+{
+	if (0 > sigprocmask(SIG_BLOCK, &child_block_sigset, NULL)) {
+		zlog(ZLOG_SYSERROR, "failed to block child signals");
+		return -1;
+	}
+	return 0;
+}
+/* }}} */
+
 int fpm_signals_unblock() /* {{{ */
 {
 	/* Ensure that during reload after upgrade all signals are unblocked.

sapi/fpm/fpm/fpm_signals.h

@@ -10,6 +10,7 @@ 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_child_block();
 int fpm_signals_unblock();
 
 extern const char *fpm_signal_names[NSIG + 1];

sapi/fpm/tests/bug76601-reload-child-signals.phpt

@@ -0,0 +1,92 @@
+--TEST--
+FPM: bug76601 children should not ignore signals during concurrent reloads
+--SKIPIF--
+<?php
+include "skipif.inc";
+if (getenv("SKIP_SLOW_TESTS")) die("skip slow test");
+?>
+--FILE--
+<?php
+
+require_once "tester.inc";
+
+$cfg = <<<EOT
+[global]
+error_log = {{FILE:LOG}}
+pid = {{FILE:PID}}
+; some value twice greater than tester->getLogLines() timeout
+process_control_timeout=10
+[unconfined]
+listen = {{ADDR}}
+; spawn children immediately after reload
+pm = static
+pm.max_children = 10
+EOT;
+
+$code = <<<EOT
+<?php
+/* empty */
+EOT;
+
+/*
+ * If a child miss SIGQUIT then reload process should stuck
+ * for at least process_control_timeout that is set greater
+ * than timout in log reading functions.
+ *
+ * Alternative way is to set log_level=debug and filter result of
+ * $tester->getLogLines(2000) for lines containing SIGKILL
+ * 
+ *     [22-Oct-2019 03:28:19.532703] DEBUG: pid 21315, fpm_pctl_kill_all(), line 161: [pool unconfined] sending signal 9 SIGKILL to child 21337
+ *     [22-Oct-2019 03:28:19.533471] DEBUG: pid 21315, fpm_children_bury(), line 259: [pool unconfined] child 21337 exited on signal 9 (SIGKILL) after 1.003055 seconds from start
+ * 
+ * but it has less probability of failure detection. Additionally it requires more
+ * $tester->expectLogNotice() around last reload due to presence of debug messages.
+ */
+
+$tester = new FPM\Tester($cfg, $code);
+$tester->start();
+$tester->expectLogStartNotices();
+
+/* 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: .*' */
+$skipped = $tester->getLogLines(2000);
+
+$tester->signal('USR2');
+$tester->expectLogNotice('Reloading in progress ...');
+/* When a child ignores SIGQUIT, the following expectation fails due to timeout. */
+if (!$tester->expectLogNotice('reloading: .*')) {
+    /* for troubleshooting */
+    echo "Skipped messages\n";
+    echo implode('', $skipped);
+}
+$tester->expectLogNotice('using inherited socket fd=\d+, "127.0.0.1:\d+"');
+$tester->expectLogStartNotices();
+
+$tester->terminate();
+$tester->expectLogTerminatingNotices();
+$tester->close();
+
+?>
+Done
+--EXPECT--
+Reached interval 25000 us with 1000 us steps
+Done
+--CLEAN--
+<?php
+require_once "tester.inc";
+FPM\Tester::clean();
+?>