httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject [PATCH] pid safety checks for 2.2.x
Date Wed, 27 Jun 2007 17:52:37 GMT
Here's the updated (and simpler) version of my patch which uses 
apr_proc_wait() to determine whether a pid is a valid child.  Simplifies 
the MPM logic a bit since the pid != 0 check is moved into 
ap_mpm_safe_kill().

Tested for both prefork and worker (on Linux) to fix the vulnerability 
using mod_scribble:

http://people.apache.org/~jorton/mod_scribble.c

Index: server/mpm/prefork/prefork.c
===================================================================
--- server/mpm/prefork/prefork.c	(revision 549489)
+++ server/mpm/prefork/prefork.c	(working copy)
@@ -1127,7 +1127,7 @@
         for (index = 0; index < ap_daemons_limit; ++index) {
             if (ap_scoreboard_image->servers[index][0].status != SERVER_DEAD) {
                 /* Ask each child to close its listeners. */
-                kill(MPM_CHILD_PID(index), AP_SIG_GRACEFUL);
+                ap_mpm_safe_kill(MPM_CHILD_PID(index), AP_SIG_GRACEFUL);
                 active_children++;
             }
         }
@@ -1165,12 +1165,10 @@
 
             active_children = 0;
             for (index = 0; index < ap_daemons_limit; ++index) {
-                if (MPM_CHILD_PID(index) != 0) {
-                    if (kill(MPM_CHILD_PID(index), 0) == 0) {
-                            active_children = 1;
-                            /* Having just one child is enough to stay around */
-                            break;
-                    }
+                if (ap_mpm_safe_kill(MPM_CHILD_PID(index), 0) == APR_SUCCESS) {
+                    active_children = 1;
+                    /* Having just one child is enough to stay around */
+                    break;
                 }
             }
         } while (!shutdown_pending && active_children &&
@@ -1222,7 +1220,7 @@
                  * piped loggers, etc. They almost certainly won't handle
                  * it gracefully.
                  */
-                kill(ap_scoreboard_image->parent[index].pid, AP_SIG_GRACEFUL);
+                ap_mpm_safe_kill(ap_scoreboard_image->parent[index].pid, AP_SIG_GRACEFUL);
             }
         }
     }
Index: server/mpm/worker/worker.c
===================================================================
--- server/mpm/worker/worker.c	(revision 549489)
+++ server/mpm/worker/worker.c	(working copy)
@@ -1813,12 +1813,10 @@
 
             active_children = 0;
             for (index = 0; index < ap_daemons_limit; ++index) {
-                if (MPM_CHILD_PID(index) != 0) {
-                    if (kill(MPM_CHILD_PID(index), 0) == 0) {
-                            active_children = 1;
-                            /* Having just one child is enough to stay around */
-                            break;
-                    }
+                if (ap_mpm_safe_kill(MPM_CHILD_PID(index), 0) == APR_SUCCESS) {
+                    active_children = 1;
+                    /* Having just one child is enough to stay around */
+                    break;
                 }
             }
         } while (!shutdown_pending && active_children &&
Index: server/mpm/experimental/event/event.c
===================================================================
--- server/mpm/experimental/event/event.c	(revision 549489)
+++ server/mpm/experimental/event/event.c	(working copy)
@@ -1998,12 +1998,10 @@
 
             active_children = 0;
             for (index = 0; index < ap_daemons_limit; ++index) {
-                if (MPM_CHILD_PID(index) != 0) {
-                    if (kill(MPM_CHILD_PID(index), 0) == 0) {
-                            active_children = 1;
-                            /* Having just one child is enough to stay around */
-                            break;
-                    }
+                if (ap_mpm_safe_kill(MPM_CHILD_PID(index), 0) == APR_SUCCESS) {
+                    active_children = 1;
+                    /* Having just one child is enough to stay around */
+                    break;
                 }
             }
         } while (!shutdown_pending && active_children &&
Index: server/mpm_common.c
===================================================================
--- server/mpm_common.c	(revision 549489)
+++ server/mpm_common.c	(working copy)
@@ -305,6 +305,27 @@
         cur_extra = next;
     }
 }
+
+apr_status_t ap_mpm_safe_kill(pid_t pid, int sig)
+{
+    apr_proc_t proc;
+    apr_status_t waitret;
+
+    /* Ensure the given pid is greater than zero; passing waitpid() a
+     * zero or negative pid has different semantics. */
+    if (pid < 1) {
+        return APR_EINVAL;
+    }
+
+    proc.pid = pid;
+    waitret = apr_proc_wait(&proc, NULL, NULL, APR_NOWAIT);
+    if (waitret == APR_CHILD_NOTDONE) {
+        return kill(pid, sig) ? errno : APR_SUCCESS;
+    }
+    else {
+        return APR_EINVAL;
+    }
+}
 #endif /* AP_MPM_WANT_RECLAIM_CHILD_PROCESSES */
 
 #ifdef AP_MPM_WANT_WAIT_OR_TIMEOUT
Index: include/mpm_common.h
===================================================================
--- include/mpm_common.h	(revision 549489)
+++ include/mpm_common.h	(working copy)
@@ -145,6 +145,17 @@
 #endif
 
 /**
+ * Safely signal an MPM child process, if the process is in the
+ * current process group.  Otherwise fail.
+ * @param pid the process id of a child process to signal
+ * @param sig the signal number to send
+ * @return APR_SUCCESS if signal is sent, otherwise an error as per kill(3)
+ */
+#ifdef AP_MPM_WANT_RECLAIM_CHILD_PROCESSES
+apr_status_t ap_mpm_safe_kill(pid_t pid, int sig);
+#endif
+
+/**
  * Determine if any child process has died.  If no child process died, then
  * this process sleeps for the amount of time specified by the MPM defined
  * macro SCOREBOARD_MAINTENANCE_INTERVAL.

Mime
View raw message