httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: [PATCH] pid safety checks for 2.2.x
Date Thu, 28 Jun 2007 11:56:16 GMT
On Wed, Jun 27, 2007 at 09:38:10PM +0200, Ruediger Pluem wrote:
> > +    /* Ensure the given pid is greater than zero; passing waitpid() a
> > +     * zero or negative pid has different semantics. */
> 
> Ok, it seems as I am trying to become the king of all nitpickers :-):
> 
> Style of comment.

Happy to oblige but I'm not sure what the nit is - just my poor grammar 
or the slightly inappropriate reference to waitpid()?

> > +    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;
> 
> Hm. Wouldn't it make sense to log this in the case
> 
> waitret != APR_CHILD_DONE
> 
> as in the PID table patches?
> This could give the admin a hint that something is rotten on his box.

I guess this is worthwhile.

The problem is that waitpid() does not distinguish between "child 
already reaped" (ignorable error) and "child not in process group" 
(something bad) so that will mean some unnecessary log spam in some 
cases.  

The log spam is avoidable using getpgid() so I've resurrected that on 
platforms where available: (which will be 99%)

So, final comments on this?  If there's consensus that this is the 
approach to take I'll revert the pidtable stuff out of trunk, commit 
this there, and propose the backport.

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,64 @@
         cur_extra = next;
     }
 }
+
+/* Before sending the signal to the pid this function verifies that
+ * the pid is a member of the current process group; either using
+ * apr_proc_wait(), where waitpid() guarantees to fail for non-child
+ * processes; or by using getpgid() directly, if available. */
+apr_status_t ap_mpm_safe_kill(pid_t pid, int sig)
+{
+#ifndef HAVE_GETPGID
+    apr_proc_t proc;
+    apr_status_t rv;
+    apr_exit_why_e why;
+    int status;
+
+    if (pid < 1) {
+        return APR_EINVAL;
+    }
+
+    proc.pid = pid;
+    rv = apr_proc_wait(&proc, &status, &why, APR_NOWAIT);
+    if (rv == APR_CHILD_DONE) {
+#ifdef AP_MPM_WANT_PROCESS_CHILD_STATUS
+        /* The child already died - log the termination status if
+         * necessary: */
+        ap_process_child_status(&proc, why, status);
+#endif
+        return APR_EINVAL;
+    }
+    else if (rv != APR_CHILD_NOTDONE) {
+        /* The child is already dead and reaped, or was a bogus pid -
+         * log this either way. */
+        ap_log_error(APLOG_MARK, APLOG_NOTICE, rv, ap_server_conf,
+                     "cannot send signal %d to pid %ld (non-child or "
+                     "already dead)", sig, (long)pid);
+        return APR_EINVAL;
+    }
+#else
+    int pg;
+
+    if (pid < 1) {
+        return APR_EINVAL;
+    }
+
+    pg = getpgid(pid);    
+    if (pg == -1) {
+        /* Process already dead... */
+        return errno;
+    }
+
+    if (pg != getpgrp()) {
+        ap_log_error(APLOG_MARK, APLOG_NOTICE, 0, ap_server_conf,
+                     "refusing to send signal %d to pid %ld outside "
+                     "process group", sig, (long)pid);
+        return APR_EINVAL;
+    }
+#endif        
+
+    return kill(pid, sig) ? errno : APR_SUCCESS;
+}
 #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.
Index: configure.in
===================================================================
--- configure.in	(revision 549489)
+++ configure.in	(working copy)
@@ -392,6 +392,7 @@
 bindprocessor \
 prctl \
 timegm \
+getpgid
 )
 
 dnl confirm that a void pointer is large enough to store a long integer


Mime
View raw message