You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Joe Orton <jo...@redhat.com> on 2003/03/05 17:33:28 UTC
[PATCH] inheriting pipes
Pipes created by the Unix implementation of apr_file_pipe_create are
inherited by default, but you can't make them uninherited without
registering a custom cleanup handler. Any objections to this patch,
which lets apr_file_inherit_unset work on a pipe for Unix? (it looks
like it might already work on Win32).
Alternatively I wonder whether it would be better to not leak pipes by
default, though presumably it was done this way for a reason...
Index: pipe.c
===================================================================
RCS file: /home/cvs/apr/file_io/unix/pipe.c,v
retrieving revision 1.61
diff -u -r1.61 pipe.c
--- pipe.c 7 Jan 2003 00:52:53 -0000 1.61
+++ pipe.c 5 Mar 2003 16:29:11 -0000
@@ -56,6 +56,8 @@
#include "apr_strings.h"
#include "apr_portable.h"
+#include "apr_arch_inherit.h"
+
/* Figure out how to get pipe block/nonblock on BeOS...
* Basically, BONE7 changed things again so that ioctl didn't work,
* but now fcntl does, hence we need to do this extra checking.
@@ -223,6 +225,8 @@
#if APR_HAS_THREADS
(*out)->thlock = NULL;
#endif
+
+ (*in)->flags = (*out)->flags = APR_INHERIT;
apr_pool_cleanup_register((*in)->pool, (void *)(*in), apr_unix_file_cleanup,
apr_pool_cleanup_null);
Re: [PATCH] inheriting pipes
Posted by "Bjoern A. Zeeb" <bz...@lists.zabbadoz.net>.
On Wed, 5 Mar 2003, Joe Orton wrote:
> On Wed, Mar 05, 2003 at 08:03:51PM +0100, Christian Kratzer wrote:
> > Hi,
> >
> > On Wed, 5 Mar 2003, Joe Orton wrote:
> > >
> > > + (*in)->flags = (*out)->flags = APR_INHERIT;
> >
> The APR_INHERIT flag needs to be set, since the current code does allow
> pipes to be inherited by default. If the APR_INHERIT flag is not set,
> apr_file_inherit_unset has no effect. To answer Bill's question as
> well, flags is zero at this point in the function, so a bitop is not
> need, just an assignment.
That's true. But please move the assigment up to the others just for
consistency (#include "inherit.h" left out of patch):
--- ./pipe.c.orig Wed Mar 5 21:46:13 2003
+++ ./pipe.c Wed Mar 5 21:49:13 2003
@@ -204,6 +204,7 @@
(*in)->filedes = filedes[0];
(*in)->is_pipe = 1;
(*in)->fname = NULL;
+ (*in)->flags = APR_INHERIT;
(*in)->buffered = 0;
(*in)->blocking = BLK_ON;
(*in)->timeout = -1;
@@ -217,6 +218,7 @@
(*out)->filedes = filedes[1];
(*out)->is_pipe = 1;
(*out)->fname = NULL;
+ (*out)->flags = APR_INHERIT;
(*out)->buffered = 0;
(*out)->blocking = BLK_ON;
(*out)->timeout = -1;
--
Greetings
Bjoern A. Zeeb bzeeb at Zabbadoz dot NeT
56 69 73 69 74 http://www.zabbadoz.net/
Re: [PATCH] inheriting pipes
Posted by Christian Kratzer <ck...@cksoft.de>.
Hi,
On Wed, 5 Mar 2003, Joe Orton wrote:
[snipp]
> The APR_INHERIT flag needs to be set, since the current code does allow
> pipes to be inherited by default. If the APR_INHERIT flag is not set,
> apr_file_inherit_unset has no effect. To answer Bill's question as
> well, flags is zero at this point in the function, so a bitop is not
> need, just an assignment.
ok.
> > But why not just register apr_unix_file_cleanup for cleanup like in
>
> Indeed, I wonder it was implement this way too. I think the change you
> suggest is a little more intrusive than mine, as currently some callers
> are compensating for the fact that no child_cleanup is registered.
perhaps we can get the opion on some apr developers on this. Imho its
better to spend longer time finding the solution most natural to apr
than finding some hack that achieves the same affect.
I am new to apr myself having looked at it for the first time while
tracing the open file descriptors in apache.
> I also wonder why the code goes to these lengths when on Unix setting
> the CLOEXEC flag would probably suffice.
It looks like apr is trying to emulate CLOEXEC behaviour by calling
the cleanup function in apr_pool_cleanup_for_exec().
This could be so because apr does not expect CLOEXEC to be available
on every platform.
Greetings
Christian
--
CK Software GmbH
Christian Kratzer, Schwarzwaldstr. 31, 71131 Jettingen
Email: ck@cksoft.de
Phone: +49 7452 889-135 Open Software Solutions, Network Security
Fax: +49 7452 889-136 FreeBSD spoken here!
Re: [PATCH] inheriting pipes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 04:53 PM 3/5/2003, Bjoern A. Zeeb wrote:
>On Wed, 5 Mar 2003, William A. Rowe, Jr. wrote:
>
>Hi,
>
>already been to bed but ...
>
>> We should do *either*; if CLOEXEC is supported and can be toggled
>> per our API (_set/_unset) then that can be the preferred method, to protect
>> ourselves from non-apr callers of exec(). (This goes for files, too.)
>
>if it is possible FD_CLOEXEC should really be independed of
>inherit_(un)set. It should be applied to every fd opened by default
>(if available) apart from 012 which seem to already be handled their
>own way. One may have a flag (perhaps also use APR_FILE_NOCLEANUP) to
>supress usage.
>Why ? Because we had seen wrong usage of APR_IMPLEMENT_(UN)INHERIT_SET
>epanded macros at various files in the apache httpd code...
That's apache httpd's problem, not ours. If they don't want those files
inherited they must communicate that fact. All we can do is take it
a step further and set CLOEXEC appropriately.
And in their child_init they should be toggling the handles they wanted
inherited to one level of child process (e.g. files opened in the parent
prior to the httpd parent/child fork()).
We don't redesign APR due to poor httpd implementation, we fix what
can't be implemented in the first place :-) This isn't the case here.
Bill
Re: [PATCH] inheriting pipes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Yes... I know I already responded, now new questions;
At 04:53 PM 3/5/2003, Bjoern A. Zeeb wrote:
>if it is possible FD_CLOEXEC should really be independed of
>inherit_(un)set. It should be applied to every fd opened by default
>(if available) apart from 012 which seem to already be handled their
>own way. One may have a flag (perhaps also use APR_FILE_NOCLEANUP) to
>supress usage.
As I suggested, CLOEXEC isn't independent. It is the way *some* platforms
would prefer to implement inherit_[un]set. Not all, such as TPF, but the vast
majority would have an easier time with it.
>Why ? Because we had seen wrong usage of APR_IMPLEMENT_(UN)INHERIT_SET
>epanded macros at various files in the apache httpd code...
I'm guessing you do not mean wrong usage of the macro
APR_IMPLEMENT_(UN)INHERIT_SET, but rather inaccurate calls
(missing when useful or present when invalid).
In either case your citations would be helpful to the dev@httpd list
so they can start fixing those bugs.
Bill
Re: [PATCH] inheriting pipes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 04:24 PM 3/5/2003, Bjoern A. Zeeb wrote:
>On Wed, 5 Mar 2003, William A. Rowe, Jr. wrote:
>
>> At 01:54 PM 3/5/2003, Joe Orton wrote:
>> >I also wonder why the code goes to these lengths when on Unix setting
>> >the CLOEXEC flag would probably suffice.
>>
>> We should do *either*; if CLOEXEC is supported and can be toggled
>> per our API (_set/_unset) then that can be the preferred method, to protect
>> ourselves from non-apr callers of exec(). (This goes for files, too.)
>
>THANKS !!!
You are welcome.
>Perhaps then also have s.th. like an "alias" apr_pipe_(un)set_inherit to
>apr_file_(un)set_inherit ? It perhaps would make some lines of code
>more clear...
No - because the API functions are not apr_pipe_foo.
Pipes use the apr_file_ api, period. Since on all platforms they take
an apr_file_t, it doesn't matter anyways. If there is any distinction,
that will happen in the _create and _cleanup code as appropriate.
(And that's inherit_[un]set, not visa versa :-)
>< may be ignored >
>Just 'cause I currently saw this: why is it named apr_file_cleanup on
>1/2OS aaeh OS/2 and apr_unix_file_cleanup ? And it seems to be only
>named file_cleanup for win32.
Because you *shouldn't* know the internals of those cleanups. It's entirely
irrelevant internals of APR.
>Functions are also named apr_file_open or apr_file_pipe_create for
>unix. So ich someone touches this perhaps we can also do some cleanup
>for thos cleanup_fns ? Ok, I can hear you till here go *waaah* ;-))))
>If I am right with my limited apache/apr experience this functions
>should be/"are" hidden from users so it should not be real problem ?
Exactly - it should all be transparent to them.
No user should be adding/removing the cleanups - it is up to the
accessors such as apr_file_inherit_[un]set to do such things.
Bill
Re: [PATCH] inheriting pipes
Posted by "Bjoern A. Zeeb" <bz...@lists.zabbadoz.net>.
On Wed, 5 Mar 2003, William A. Rowe, Jr. wrote:
Hi,
already been to bed but ...
> We should do *either*; if CLOEXEC is supported and can be toggled
> per our API (_set/_unset) then that can be the preferred method, to protect
> ourselves from non-apr callers of exec(). (This goes for files, too.)
if it is possible FD_CLOEXEC should really be independed of
inherit_(un)set. It should be applied to every fd opened by default
(if available) apart from 012 which seem to already be handled their
own way. One may have a flag (perhaps also use APR_FILE_NOCLEANUP) to
supress usage.
Why ? Because we had seen wrong usage of APR_IMPLEMENT_(UN)INHERIT_SET
epanded macros at various files in the apache httpd code...
--
G'Night
Bjoern A. Zeeb bzeeb at Zabbadoz dot NeT
56 69 73 69 74 http://www.zabbadoz.net/
Re: [PATCH] inheriting pipes
Posted by "Bjoern A. Zeeb" <bz...@lists.zabbadoz.net>.
On Wed, 5 Mar 2003, William A. Rowe, Jr. wrote:
> At 01:54 PM 3/5/2003, Joe Orton wrote:
> >> But why not just register apr_unix_file_cleanup for cleanup like in
> >
> >Indeed, I wonder it was implement this way too. I think the change you
> >suggest is a little more intrusive than mine, as currently some callers
> >are compensating for the fact that no child_cleanup is registered.
>
> I agree we must register a cleanup so that we don't have to jump through
> hoops later. APR app developers should be able to trust that there is
> already a cleanup registered for such objects they create.
They already do :-((
> >I also wonder why the code goes to these lengths when on Unix setting
> >the CLOEXEC flag would probably suffice.
>
> We should do *either*; if CLOEXEC is supported and can be toggled
> per our API (_set/_unset) then that can be the preferred method, to protect
> ourselves from non-apr callers of exec(). (This goes for files, too.)
THANKS !!!
Perhaps then also have s.th. like an "alias" apr_pipe_(un)set_inherit to
apr_file_(un)set_inherit ? It perhaps would make some lines of code
more clear...
< may be ignored >
Just 'cause I currently saw this: why is it named apr_file_cleanup on
1/2OS aaeh OS/2 and apr_unix_file_cleanup ? And it seems to be only
named file_cleanup for win32.
Functions are also named apr_file_open or apr_file_pipe_create for
unix. So ich someone touches this perhaps we can also do some cleanup
for thos cleanup_fns ? Ok, I can hear you till here go *waaah* ;-))))
If I am right with my limited apache/apr experience this functions
should be/"are" hidden from users so it should not be real problem ?
< /may be ignored >
--
Bjoern A. Zeeb bzeeb at Zabbadoz dot NeT
56 69 73 69 74 http://www.zabbadoz.net/
Re: [PATCH] inheriting pipes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 01:54 PM 3/5/2003, Joe Orton wrote:
>> But why not just register apr_unix_file_cleanup for cleanup like in
>
>Indeed, I wonder it was implement this way too. I think the change you
>suggest is a little more intrusive than mine, as currently some callers
>are compensating for the fact that no child_cleanup is registered.
I agree we must register a cleanup so that we don't have to jump through
hoops later. APR app developers should be able to trust that there is
already a cleanup registered for such objects they create.
>I also wonder why the code goes to these lengths when on Unix setting
>the CLOEXEC flag would probably suffice.
We should do *either*; if CLOEXEC is supported and can be toggled
per our API (_set/_unset) then that can be the preferred method, to protect
ourselves from non-apr callers of exec(). (This goes for files, too.)
>> --- httpd-2.0.44/srclib/apr/file_io/unix/pipe.c.orig Sun Mar 2 00:54:10
>> 2003
>> +++ httpd-2.0.44/srclib/apr/file_io/unix/pipe.c Sun Mar 2 00:54:36 2003
>> @@ -225,9 +225,9 @@
>> #endif
>>
>> apr_pool_cleanup_register((*in)->pool, (void *)(*in), apr_unix_file_cleanup,
>> - apr_pool_cleanup_null);
>> + apr_unix_file_cleanup);
>> apr_pool_cleanup_register((*out)->pool, (void *)(*out), apr_unix_file_cleanup,
>> - apr_pool_cleanup_null);
>> + apr_unix_file_cleanup);
>> return APR_SUCCESS;
>> }
>>
>> would this not be more to the point of getting the descriptors closed on exec.
>>
>> Greetings
>> Christian
>>
>> --
>> CK Software GmbH
>> Christian Kratzer, Schwarzwaldstr. 31, 71131 Jettingen
>> Email: ck@cksoft.de
>> Phone: +49 7452 889-135 Open Software Solutions, Network Security
>> Fax: +49 7452 889-136 FreeBSD spoken here!
>>
Re: [PATCH] inheriting pipes
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Mar 05, 2003 at 08:03:51PM +0100, Christian Kratzer wrote:
> Hi,
>
> On Wed, 5 Mar 2003, Joe Orton wrote:
>
> > Pipes created by the Unix implementation of apr_file_pipe_create are
> > inherited by default, but you can't make them uninherited without
> > registering a custom cleanup handler. Any objections to this patch,
> > which lets apr_file_inherit_unset work on a pipe for Unix? (it looks
> > like it might already work on Win32).
> >
> > Alternatively I wonder whether it would be better to not leak pipes by
> > default, though presumably it was done this way for a reason...
> [snipp]
> > + (*in)->flags = (*out)->flags = APR_INHERIT;
>
> With this you are setting the inherit flag instead of clearing it.
> Should this not rather be the following
>
> + (*in)->flags &= ~APR_INHERIT);
> + (*out)->flags &= ~APR_INHERIT);
>
> which would clear APR_INHERIT in both flags.
The APR_INHERIT flag needs to be set, since the current code does allow
pipes to be inherited by default. If the APR_INHERIT flag is not set,
apr_file_inherit_unset has no effect. To answer Bill's question as
well, flags is zero at this point in the function, so a bitop is not
need, just an assignment.
> But why not just register apr_unix_file_cleanup for cleanup like in
Indeed, I wonder it was implement this way too. I think the change you
suggest is a little more intrusive than mine, as currently some callers
are compensating for the fact that no child_cleanup is registered.
I also wonder why the code goes to these lengths when on Unix setting
the CLOEXEC flag would probably suffice.
> --- httpd-2.0.44/srclib/apr/file_io/unix/pipe.c.orig Sun Mar 2 00:54:10
> 2003
> +++ httpd-2.0.44/srclib/apr/file_io/unix/pipe.c Sun Mar 2 00:54:36 2003
> @@ -225,9 +225,9 @@
> #endif
>
> apr_pool_cleanup_register((*in)->pool, (void *)(*in), apr_unix_file_cleanup,
> - apr_pool_cleanup_null);
> + apr_unix_file_cleanup);
> apr_pool_cleanup_register((*out)->pool, (void *)(*out), apr_unix_file_cleanup,
> - apr_pool_cleanup_null);
> + apr_unix_file_cleanup);
> return APR_SUCCESS;
> }
>
> would this not be more to the point of getting the descriptors closed on exec.
>
> Greetings
> Christian
>
> --
> CK Software GmbH
> Christian Kratzer, Schwarzwaldstr. 31, 71131 Jettingen
> Email: ck@cksoft.de
> Phone: +49 7452 889-135 Open Software Solutions, Network Security
> Fax: +49 7452 889-136 FreeBSD spoken here!
>
Re: [PATCH] inheriting pipes
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 10:33 AM 3/5/2003, Joe Orton wrote:
>Pipes created by the Unix implementation of apr_file_pipe_create are
>inherited by default, but you can't make them uninherited without
>registering a custom cleanup handler. Any objections to this patch,
>which lets apr_file_inherit_unset work on a pipe for Unix? (it looks
>like it might already work on Win32).
> (*in)->flags = (*out)->flags = APR_INHERIT;
Didn't you mean
(*in)->flags |= APR_INHERIT;
(*out)->flags |= APR_INHERIT;
??? You don't want to corrupt their values. Yes - your patch looks
100% spot-on, +1. If those pipes are created inherited, we need to
track the current 'setting' correctly.
Bill
Re: [PATCH] inheriting pipes
Posted by Christian Kratzer <ck...@cksoft.de>.
Hi,
On Wed, 5 Mar 2003, Joe Orton wrote:
> Pipes created by the Unix implementation of apr_file_pipe_create are
> inherited by default, but you can't make them uninherited without
> registering a custom cleanup handler. Any objections to this patch,
> which lets apr_file_inherit_unset work on a pipe for Unix? (it looks
> like it might already work on Win32).
>
> Alternatively I wonder whether it would be better to not leak pipes by
> default, though presumably it was done this way for a reason...
[snipp]
> + (*in)->flags = (*out)->flags = APR_INHERIT;
With this you are setting the inherit flag instead of clearing it.
Should this not rather be the following
+ (*in)->flags &= ~APR_INHERIT);
+ (*out)->flags &= ~APR_INHERIT);
which would clear APR_INHERIT in both flags.
But why not just register apr_unix_file_cleanup for cleanup like in
--- httpd-2.0.44/srclib/apr/file_io/unix/pipe.c.orig Sun Mar 2 00:54:10
2003
+++ httpd-2.0.44/srclib/apr/file_io/unix/pipe.c Sun Mar 2 00:54:36 2003
@@ -225,9 +225,9 @@
#endif
apr_pool_cleanup_register((*in)->pool, (void *)(*in), apr_unix_file_cleanup,
- apr_pool_cleanup_null);
+ apr_unix_file_cleanup);
apr_pool_cleanup_register((*out)->pool, (void *)(*out), apr_unix_file_cleanup,
- apr_pool_cleanup_null);
+ apr_unix_file_cleanup);
return APR_SUCCESS;
}
would this not be more to the point of getting the descriptors closed on exec.
Greetings
Christian
--
CK Software GmbH
Christian Kratzer, Schwarzwaldstr. 31, 71131 Jettingen
Email: ck@cksoft.de
Phone: +49 7452 889-135 Open Software Solutions, Network Security
Fax: +49 7452 889-136 FreeBSD spoken here!