httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: PID table changes (was Re: svn commit: r547987 - in /httpd/httpd/trunk)
Date Thu, 21 Jun 2007 21:22:09 GMT


On 06/21/2007 06:51 PM, Joe Orton wrote:

> 
> 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.

So I assume the patches for the other MPM's will follow.
BTW: Do we have getpgrp / getpgid on all these systems?

> 
> 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)

Thanks for this good catch. At least I missed that.

> 
> 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)

Maybe nitpicking, but I think you missed one kill at about line 310 in
reap_children.

Regards

RĂ¼diger

Mime
View raw message