apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: discussion on fd leak problematic
Date Mon, 17 Mar 2003 03:11:41 GMT
At 04:36 PM 3/14/2003, Bjoern A. Zeeb wrote:
>this is a summary for further discussion on the fd leak problematic in
>httpd-2.0 and related apr (inherit) code.

Again, thanks.  This post focuses on the httpd aspects, presuming
all of your issues in APR are addressed by that list, and all that httpd
must do is to follow good design for the *expected* behavior of APR :-)

Comments inline.

>The Problem has existed for a along time in httpd code but came not up
>before 2.0.40 because of another bug in apr which has been fixed before
>2.0.40 release[3].

Yes - we've revised the exec/inherit code several times - each time
'improving' it but therefore impacting httpd's behavior.

>- From then on APR_IMPLEMENT_INHERIT_SET and APR_IMPLEMENT_INHERIT_UNSET
>worked the way they should but they had already been ""misused"" the wrong
>way for unknown reasons in httpd code. See cvs commits around the versions
>of diff in [4].

Correct, in part.  Consider three models;

  -D ONE_PROCESS (-x) 
    There is no parent - so we have no interest in propogating any
    handles except stdin/out/err for CGI and things such as pipe loggers.

  Unix Prefork/Worker
    Here the parent fork()s the child workers - without exec().  
    FD_CLOEXEC would work throughout httpd - even if a 3rd party 
    module such as php were to call exec() or system() itself.  
    That is a *huge* argument for apr to back our inherit_[un]set 
    preference with FD_CLOEXEC.  Same inherited handles as for 
    -D ONE_PROCESS above, most will not need to be inherited.

  Win32 (others?) MPM
    Here we should be setting many handles as inherited in the parent
    process, which will call apr_proc_create to launch the child workers.
    There is a huge caviat here... the parent must pass those handles
    to the child through a currently undefined mechanism, and the child
    must receive those handles and then reset them with inherit_unset()!

Now, where did my incorrect commits come in?  I didn't recognize that,
of course, fork() won't close any handles or invoke the cleanups.  The
child worker processes *are* the same 'process', as the parent, in a
manner of speaking.

>HTTPD
>- - - - - - - - - - - - - - - - - - - - -
>
>because of the ""missuse"" of apr_file_inherit_set and not registered
>child cleanup_fn we have various fd leaks for pipes and logs.
>
>After my initial patch [1][10] Joe Orton came up with another patch
>to dev@httpd[7]. My problem with this is:
>is it good enough to simply remove the apr_file_inherit_set() calls
>or explicitly use apr_file_inherit_unset() so that we can be sure
>that there is always a child cleanup_fn registered.

First we must remove those calls, with a COMMENT to the effect
that this code is needed to properly implement multiple child 
processes under non-fork()ing platforms.  We don't respect those
handles or provide the mechanics today - so we can comment out
the code for today and re-enable, sometime in the near future, with
the correct mechanics, respecting APR_HAS_FORK. 

>In apr there still is the flag APR_FILE_NOCLEANUP that might prevent
>registering a child cleanup_fn as the deault for now is. This flag seems
>to be unused at the moment for open() calls.

I don't think this affects httpd today - I believe that flag was for very, very
persistent handles such as stdin/out/err

>Another thing I lately discovered after some discussion with Steve Grubb
>on lseek()ing on open file descriptors was that for me only error logs
>had been readable but not access logs. I think we do not need to open
>error_logs for reading. 

I agree...

>A fix for at least one place has been posted to
>dev@httpd[11]. There might be other places where files are opened with
>more priviledges than needed. Please look at this.

+1 to this patch.  Agreed more research is required, and 3rd party
authors should be sensitive to this issue.

Bill



Mime
View raw message