Prevent use after free in fpm_event_epoll_wait

epoll event backend does not guarantee that child input/output events
are reported before SIGCHILD due to finished worker. While a bunch of
events received by epoll is being processed, child-related structures
may be removed before dispatching of an I/O event for the same child.
The result may be attempt to access to memory region allocated for
another purpose, segfault of the master process, and unavailable web
sites.

Postpone processing of SIGCHILD events till other events in the same
bunch are processed.

Fix Bug #62418 php-fpm master process crashes
Fix Bug #65398 Race condition between SIGCHLD and child stdout/stderr event leads to segfault
Fix Bug #75112 php-fpm crashing, hard to reproduce
Fix Bug #77114 php-fpm master segfaults in fpm_event_epoll_wait/fpm_event_fire
Fix Bug #77185 Use-after-free in FPM master event handling

diff --git a/NEWS b/NEWS
index 721773e4b884af885ce459d4b9efbc5cf2a92dd9..63389a0b6b35ff10c0ae47403504f88445a8c1b7 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -6,6 +6,10 @@ PHP                                                                        NEWS
   . Fixed bug #77946 (Bad cURL resources returned by curl_multi_info_read()).
     (Abyr Valg)
 
+- FPM:
+  . Fixed bug #77185 (Use-after-free in FPM master event handling).
+    (Maksim Nikulin)
+
 - Standard:
   . Fixed bug #69100 (Bus error from stream_copy_to_stream (file -> SSL stream)
     with invalid length). (Nikita)

diff --git a/sapi/fpm/fpm/fpm_events.c b/sapi/fpm/fpm/fpm_events.c
index 1750fab103a2781021d69adeb24ad0971c29fed0..81db9f8867a65a19f3bb4a487c5f03fbf21b8612 100644 (file)
--- a/sapi/fpm/fpm/fpm_events.c
+++ b/sapi/fpm/fpm/fpm_events.c
@@ -34,6 +34,7 @@
 #define fpm_event_set_timeout(ev, now) timeradd(&(now), &(ev)->frequency, &(ev)->timeout);
 
 static void fpm_event_cleanup(int which, void *arg);
+static void fpm_postponed_children_bury(struct fpm_event_s *ev, short which, void *arg);
 static void fpm_got_signal(struct fpm_event_s *ev, short which, void *arg);
 static struct fpm_event_s *fpm_event_queue_isset(struct fpm_event_queue_s *queue, struct fpm_event_s *ev);
 static int fpm_event_queue_add(struct fpm_event_queue_s **queue, struct fpm_event_s *ev);
@@ -43,6 +44,7 @@ static void fpm_event_queue_destroy(struct fpm_event_queue_s **queue);
 static struct fpm_event_module_s *module;
 static struct fpm_event_queue_s *fpm_event_queue_timer = NULL;
 static struct fpm_event_queue_s *fpm_event_queue_fd = NULL;
+static struct fpm_event_s children_bury_timer;
 
 static void fpm_event_cleanup(int which, void *arg) /* {{{ */
 {
@@ -51,6 +53,12 @@ static void fpm_event_cleanup(int which, void *arg) /* {{{ */
 }
 /* }}} */
 
+static void fpm_postponed_children_bury(struct fpm_event_s *ev, short which, void *arg) /* {{{ */
+{
+       fpm_children_bury();
+}
+/* }}} */
+
 static void fpm_got_signal(struct fpm_event_s *ev, short which, void *arg) /* {{{ */
 {
        char c;
@@ -72,7 +80,12 @@ static void fpm_got_signal(struct fpm_event_s *ev, short which, void *arg) /* {{
                switch (c) {
                        case 'C' :                  /* SIGCHLD */
                                zlog(ZLOG_DEBUG, "received SIGCHLD");
-                               fpm_children_bury();
+                               /* epoll_wait() may report signal fd before read events for a finished child
+                                * in the same bunch of events. Prevent immediate free of the child structure
+                                * and so the fpm_event_s instance. Otherwise use after free happens during
+                                * attemp to process following read event. */
+                               fpm_event_set_timer(&children_bury_timer, 0, &fpm_postponed_children_bury, NULL);
+                               fpm_event_add(&children_bury_timer, 0);
                                break;
                        case 'I' :                  /* SIGINT  */
                                zlog(ZLOG_DEBUG, "received SIGINT");

diff --git a/sapi/fpm/tests/bug77185-reload-robustness.phpt b/sapi/fpm/tests/bug77185-reload-robustness.phpt
new file mode 100644 (file)
index 0000000..3780176
--- /dev/null
+++ b/sapi/fpm/tests/bug77185-reload-robustness.phpt
@@ -0,0 +1,46 @@
+--TEST--
+FPM: bug77185 - Reload robustness
+--SKIPIF--
+<?php include "skipif.inc"; ?>
+--FILE--
+<?php
+
+require_once "tester.inc";
+
+$workers = 10;
+$loops   = 10;
+
+$cfg = <<<EOT
+[global]
+error_log = {{FILE:LOG}}
+pid = {{FILE:PID}}
+[unconfined]
+listen = {{ADDR}}
+pm = static
+pm.max_children = $workers
+catch_workers_output = true
+EOT;
+
+$tester = new FPM\Tester($cfg);
+$tester->start();
+$tester->expectLogStartNotices();
+for ($i = 0; $i < $loops; $i++) {
+       $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->terminate();
+$tester->expectLogTerminatingNotices();
+$tester->close();
+
+?>
+Done
+--EXPECT--
+Done
+--CLEAN--
+<?php
+require_once "tester.inc";
+FPM\Tester::clean();
+?>