Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Errors-To: <wrowe@rowe-clan.net>
Message-Id: <5.2.0.9.2.20030316201616.01f99178@pop3.rowe-clan.net>
Date: Sun, 16 Mar 2003 20:37:30 -0600
To: "Bjoern A. Zeeb" <bz@zabbadoz.net>
From: "William A. Rowe, Jr." <wrowe@rowe-clan.net>
Subject: Re: discussion on fd leak problematic
Cc: "Bjoern A. Zeeb" <bz@zabbadoz.net>, dev@apr.apache.org,
        Christian Kratzer <ck@cksoft.de>, Steve G <linux_4ever@yahoo.com>,
        jorton@redhat.com
In-Reply-To: <Pine.BSF.4.53.0303142234000.45658@e0-0.zab2.int.zabbadoz.n
 et>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"

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.

Thank you for the thorough description and history!!!

Comments on APR inline [snipping the httpd parts and removing httpd
from the reply-to-all];  can we all agree on the following points so that
httpd, svn and other implementors have an easy time from here on out?

>The main question here is from what I can see:
>
>is it possible to use FD_CLOEXEC somewhere in the API of
>APR_IMPLEMENT_INHERIT_(UN)SET so that we might have some double checks for closing file descriptors on execs.

I strongly believe this is a correct change... if inherit_unset is called
we must set the FD_CLOEXEC bit, if inherit_set is called it must
be cleared.  The other patch you identified, we must properly toggle
the internal APR_INHERIT flag so it tracks the created apr_file_t and
all successive changes (through dup, dup2, inherit_[un]set) with that
flag bit.  Any other behavior is a bug.

>Another question from me is if it would be possible to always register
>a child chlean_fn not apr_pool_cleanup_null for pipes - resp. s.th. like
>in my diff[9].

Agreed, too, that this is a correct change ... with the absolute exception
of any apr_file_dup2 calls to associate stdin/stdout/stderr handles that
are passed to the child.  Those must not be closed on exec(), and it
seems redundant to force the user to toggle these.

We should *presume* uninherited, allow the user to toggle inherited,
which maps solves the 80/20 in terms of programmer/developer effort.

Finally, I believe that the apr_proc_create code should be responsible
to set all of the dup2 handles to inherited.

I see an interesting race where two threads are each apr_proc_create()ing
children at the same time, and both set up their handles as inherited.
Although this isn't yet a safe design (therefore httpd has mod_cgid) it is
possibly worth mutexing the apr_proc_create around this potential issue.

>Last point on this would be that developers that use APR need to somehow
>recognnize that they can change the behaviour for pipes like they can
>for files with apr_file_inherit_(un)set. not having apr_pipe_inherit_(un)set
>seems to also cause irritations amog apr/httpd-developers.

Hmmm, no.  That's a bit of confusion - a pipe is an apr_file_t* pointer, so
it must use the apr_file_FOO() API, and should use the same cleanups
since they both clean up apr_file_t objects which point to file handles
(on all platforms.)  

I can't think of a platform that supports pipes and implements them as
something other than fd or file HANDLE objects.  

I do believe it's worth documenting under apr_file_pipe_create that the
created pipe is an apr_file_t that is supported by all apr_file_foo() fns,
including apr_file_inherit_[un]set.

>For open there is the (yet) unused(?) flag . Can some
>have a look at this and either document it as yet unused or remove it
>from the code or correct me if I am wrong. [file_io/unix/open.c]

Agreed - minimally document it as @deprecated @bug Unimplemented.
But I believe that was a special case, mostly for working around our *own*
processes stdin/out/err which shouldn't be closed prior to exit();

Bill