On Sat, Jun 16, 2007 at 09:29:25PM -0000, Jim Jagielski wrote:
> Author: jim
> Date: Sat Jun 16 14:29:24 2007
> New Revision: 547987
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=547987
> Log:
> PID table impl: parent process keeps a local table store of
> Apache child process PIDs and uses that to check validity
> of what's in the scoreboard.
Firstly my sincere apologies to Jim for bringing this up after such
considerable work was put in already - I've had a busy month with a week
out for a holiday :(
Secondly: I think this approach is unnecessarily complex. I think it's
sufficient to simply check whether the target process is in the right
process group before sending a signal, i.e. getpgid(pid) == getpgrp().
This ensures that the parent will only kill things it created.
It is reasonable to assume that the parent's process group holds exactly
the set of processes which is safe to kill - prefork relies on that
being so when handling SIGHUP already.
Patch below is PoC.
Thirdly: an implementation problem which I think needs addressing
regardless:
+ if (ap_in_pid_table(MPM_CHILD_PID(index))) {
+ if (kill(MPM_CHILD_PID(index), 0) == 0) {
and similar has a potential race; both calls could result in the pid
being read back from the shm segment, and hence the malicious child
could change from a "good" pid to a "bad" one in between. I don't think
it's guaranteed that even using a temporary variable is safe; an
out-of-line function call passed MPM_CHILD_PID(index) is best. (as in
the ap_mpm_safe_kill() function in my patch)
Here's the patch (against 2.2.x):
Index: server/mpm/prefork/mpm.h
===================================================================
--- server/mpm/prefork/mpm.h (revision 549489)
+++ server/mpm/prefork/mpm.h (working copy)
@@ -53,6 +53,7 @@
#define AP_MPM_USES_POD 1
#define MPM_CHILD_PID(i) (ap_scoreboard_image->parent[i].pid)
#define MPM_NOTE_CHILD_KILLED(i) (MPM_CHILD_PID(i) = 0)
+#define MPM_VALID_PID(p) (getpgid(p) == getpgrp())
#define MPM_ACCEPT_FUNC unixd_accept
extern int ap_threads_per_child;
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++;
}
}
@@ -1166,7 +1166,7 @@
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) {
+ if (ap_mpm_safe_kill(MPM_CHILD_PID(index), 0) == 0) {
active_children = 1;
/* Having just one child is enough to stay around
*/
break;
@@ -1222,7 +1222,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/mpm.h
===================================================================
--- server/mpm/worker/mpm.h (revision 549489)
+++ server/mpm/worker/mpm.h (working copy)
@@ -52,6 +52,7 @@
#define MPM_CHILD_PID(i) (ap_scoreboard_image->parent[i].pid)
#define MPM_NOTE_CHILD_KILLED(i) (MPM_CHILD_PID(i) = 0)
#define MPM_ACCEPT_FUNC unixd_accept
+#define MPM_VALID_PID(p) (getpgid(p) == getpgrp())
extern int ap_threads_per_child;
extern int ap_max_daemons_limit;
Index: server/mpm/worker/worker.c
===================================================================
--- server/mpm/worker/worker.c (revision 549489)
+++ server/mpm/worker/worker.c (working copy)
@@ -1814,7 +1814,7 @@
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) {
+ if (ap_mpm_safe_kill(MPM_CHILD_PID(index), 0) == 0) {
active_children = 1;
/* Having just one child is enough to stay around
*/
break;
Index: server/mpm_common.c
===================================================================
--- server/mpm_common.c (revision 549489)
+++ server/mpm_common.c (working copy)
@@ -126,6 +126,10 @@
apr_proc_t proc;
apr_status_t waitret;
+ if (!MPM_VALID_PID(pid)) {
+ return 1;
+ }
+
proc.pid = pid;
waitret = apr_proc_wait(&proc, NULL, NULL, APR_NOWAIT);
if (waitret != APR_CHILD_NOTDONE) {
@@ -305,6 +309,16 @@
cur_extra = next;
}
}
+
+apr_status_t ap_mpm_safe_kill(pid_t pid, int sig)
+{
+ if (MPM_VALID_PID(pid)) {
+ 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.
