You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2003/03/18 00:50:42 UTC
[Patch] for discussion of FD_CLOEXEC and APR_INHERIT
To reassure you all, it looks like unix/pipe.c does things correctly - it's everything
else that's goofy ;-)
Here are the patches to the file schema ... one more change to apr_arch_inherit
is still required for socket fd's, and there are socket fd code changes, but this
should get the discussion started...
Premise: the default is uninherited handles, backed by FD_CLOEXEC where that
symbol is defined, unless APR_INHERIT is passed to apr_file_open()'s flags or
the user calls apr_file_inherit_set.
Pipes are automatically inherited, and must be apr_file_inherit_unset() when
desired. Obviously only one side of the handle is changed, usually.
Liberal comments sprinkled below - I'll post the complete patch either tomorrow
morning or perhaps tonight to include socket code patches. Feedback and comments
would be very greatly appreciated!!!
Bill
--- include/arch/unix/apr_arch_inherit.h 6 Jan 2003 23:44:26 -0000 1.1
+++ include/arch/unix/apr_arch_inherit.h 17 Mar 2003 22:58:31 -0000
@@ -59,6 +59,46 @@
#define APR_INHERIT (1 << 24) /* Must not conflict with other bits */
+#ifdef FD_CLOEXEC
+
+#define APR_IMPLEMENT_INHERIT_SET(name, flag, pool, cleanup) \
+apr_status_t apr_##name##_inherit_set(apr_##name##_t *the##name) \
+{ \
+ if (!(the##name->flag & APR_INHERIT)) { \
+ int ffd = fcntl(the##name->filedes, F_GETFD, 0); \
+ if (ffd >= 0) \
+ ffd = fcntl(the##name->filedes, F_SETFD, ffd & ~FD_CLOEXEC);\
+ /* if (ffd < 0) \
+ * XXX: What to do in this case? No good ideas. \
+ */ \
+ the##name->flag |= APR_INHERIT; \
+ apr_pool_child_cleanup_set(the##name->pool, \
+ (void *)the##name, \
+ cleanup, apr_pool_cleanup_null); \
+ } \
+ return APR_SUCCESS; \
+}
+
+#define APR_IMPLEMENT_INHERIT_UNSET(name, flag, pool, cleanup) \
+apr_status_t apr_##name##_inherit_unset(apr_##name##_t *the##name) \
+{ \
+ if (the##name->flag & APR_INHERIT) { \
+ int ffd = fcntl(the##name->filedes, F_GETFD, 0); \
+ if (ffd >= 0) \
+ ffd = fcntl(the##name->filedes, F_SETFD, ffd | FD_CLOEXEC); \
+ /* if (ffd < 0) \
+ * XXX: What to do in this case? No good ideas. \
+ */ \
+ the##name->flag &= ~APR_INHERIT; \
+ apr_pool_child_cleanup_set(the##name->pool, \
+ (void *)the##name, \
+ cleanup, cleanup); \
+ } \
+ return APR_SUCCESS; \
+}
+
+#else
+
#define APR_IMPLEMENT_INHERIT_SET(name, flag, pool, cleanup) \
apr_status_t apr_##name##_inherit_set(apr_##name##_t *the##name) \
{ \
# The patch above provides that we toggle the state of FD_CLOEXEC. It might
# be overkill, since most platforms support only a single bit in GETFD/SETFD,
# but I was being cautious.
@@ -69,11 +109,6 @@
cleanup, apr_pool_cleanup_null); \
} \
return APR_SUCCESS; \
-} \
-/* Deprecated */ \
-void apr_##name##_set_inherit(apr_##name##_t *the##name) \
-{ \
- apr_##name##_inherit_set(the##name); \
}
#define APR_IMPLEMENT_INHERIT_UNSET(name, flag, pool, cleanup) \
# This macro had to move below to escape from the #ifdef FD_CLOEXEC block...
@@ -86,7 +121,16 @@
cleanup, cleanup); \
} \
return APR_SUCCESS; \
-} \
+}
+
+#endif
+
+/* Deprecated */ \
+void apr_##name##_set_inherit(apr_##name##_t *the##name) \
+{ \
+ apr_##name##_inherit_set(the##name); \
+}
+
/* Deprecated */ \
void apr_##name##_unset_inherit(apr_##name##_t *the##name) \
{ \
--- file_io/unix/open.c 6 Mar 2003 09:24:17 -0000 1.110
+++ file_io/unix/open.c 17 Mar 2003 22:58:30 -0000
@@ -149,11 +149,33 @@
#endif
if (perm == APR_OS_DEFAULT) {
- fd = open(fname, oflags, 0666);
+ perm = 0x666;
}
- else {
- fd = open(fname, oflags, apr_unix_perms2mode(perm));
- }
+
+ fd = open(fname, oflags, apr_unix_perms2mode(perm));
+
+#ifdef FD_CLOEXEC
+ /* If we are registering a cleanup - we match the FD_CLOEXEC
+ * state to our cleanup state. - meaning that either flags
+ * APR_FILE_NOCLEANUP or APR_INHERIT will inhibit CLOEXEC.
+ */
+ if ((fd >= 0) && !(flag & (APR_FILE_NOCLEANUP | APR_INHERIT))) {
+ /* XXX: in multithreaded applications, there is a race
+ * between open() above and setting the FD_CLOEXEC bit.
+ */
+ int ffd = fcntl(fd, F_GETFD, 0);
+ if (ffd >= 0) {
+ ffd = fcntl(fd, F_SETFD, ffd | FD_CLOEXEC);
+ }
+ if (ffd < 0) {
+ /* XXX: What to do in this case? No good ideas; one option:
+ * close(fd);
+ * fd = -1;
+ */
+ }
+ }
+#endif
+
if (fd < 0) {
return errno;
}
# The above is rearrangement it becomes simpler to follow the flow
# of open -> fdset, so that the initial state of CLOEXEC is initialized
# correctly based on the settings of APR_FILE_NOCLEANUP and
# APR_INHERIT. I don't have a good answer to the failure case though.
@@ -191,7 +213,10 @@
if (!(flag & APR_FILE_NOCLEANUP)) {
apr_pool_cleanup_register((*new)->pool, (void *)(*new),
- apr_unix_file_cleanup, apr_unix_file_cleanup);
+ apr_unix_file_cleanup,
+ (flag & APR_INHERIT)
+ ? apr_unix_file_cleanup
+ : apr_pool_cleanup_null);
}
return APR_SUCCESS;
}
# Here we need to respect the APR_INHERIT bit when registering the cleanup.
--- file_io/unix/mktemp.c 7 Jan 2003 00:52:53 -0000 1.27
+++ file_io/unix/mktemp.c 17 Mar 2003 22:58:30 -0000
@@ -225,6 +225,13 @@
if (fd == -1) {
return errno;
}
+ /* XXX: We must reset several flags values as passed-in, since
+ * mkstemp didn't subscribe to our preference flags.
+ *
+ * We either have to unset the flags, or fix up the fd and other
+ * xthread and inherit bits appropriately. Since gettemp() above
+ * calls apr_file_open, our flags are respected in that code path.
+ */
apr_os_file_put(fp, &fd, flags, p);
(*fp)->fname = apr_pstrdup(p, template);
# And I plan to add some comments about the sorry state of mktemp/mkstemp().
# I don't have an immediate plan - it looks like we need some heavyweight code
# to deal with all of the flags and permissions issues.
# Now for the uglier patches that fix some existing bugs...
--- file_io/unix/filedup.c 7 Jan 2003 00:52:53 -0000 1.53
+++ file_io/unix/filedup.c 17 Mar 2003 22:58:30 -0000
@@ -64,28 +64,29 @@
{
int rv;
- if ((*new_file) == NULL) {
- if (which_dup == 1) {
- (*new_file) = (apr_file_t *)apr_pcalloc(p, sizeof(apr_file_t));
- if ((*new_file) == NULL) {
- return APR_ENOMEM;
- }
- (*new_file)->pool = p;
- } else {
- /* We can't dup2 unless we have a valid new_file */
+ if (which_dup == 2) {
+ if (*new_file == NULL) {
return APR_EINVAL;
}
- }
-
- if (which_dup == 2) {
+ apr_pool_cleanup_kill((*new_file)->pool, *new_file,
+ apr_unix_file_cleanup);
rv = dup2(old_file->filedes, (*new_file)->filedes);
} else {
- rv = ((*new_file)->filedes = dup(old_file->filedes));
+ rv = dup(old_file->filedes);
}
if (rv == -1)
return errno;
-
+
+ if ((*new_file) == NULL) {
+ (*new_file) = (apr_file_t *)apr_pcalloc(p, sizeof(apr_file_t));
+ (*new_file)->pool = p;
+ }
+
+ if (which_dup != 2) {
+ (*new_file)->filedes = rv;
+ }
+
(*new_file)->fname = apr_pstrdup(p, old_file->fname);
(*new_file)->buffered = old_file->buffered;
# The code above is mainly geared to allow the reader to follow the
# code flow. But it does break out some special cases (with netware)
# that correspond to changes below in the two public entry points below.
@@ -112,10 +113,35 @@
/* make sure unget behavior is consistent */
(*new_file)->ungetchar = old_file->ungetchar;
- /* apr_file_dup() clears the inherit attribute, user must call
- * apr_file_inherit_set() again on the dupped handle, as necessary.
+ /* apr_file_dup() clears the inherit attribute except for fd's 0..2
+ * so the user must call apr_file_inherit_set() again on the dupped
+ * handle, when desired.
*/
- (*new_file)->flags = old_file->flags & ~APR_INHERIT;
+ if ((*new_file)->filedes <= 2) {
+ /* Default the stdin/out/err fd's to inherited */
+ (*new_file)->flags = old_file->flags | APR_INHERIT;
+ }
+ else {
+ (*new_file)->flags = old_file->flags & ~APR_INHERIT;
+ }
+
+ apr_pool_cleanup_register((*new_file)->pool, (void *)(*new_file),
+ apr_unix_file_cleanup,
+ (old_file->flags & APR_INHERIT)
+ ? apr_pool_cleanup_null
+ : apr_unix_file_cleanup);
+
+#ifdef FD_CLOEXEC
+ int ffd = fcntl((*new_file)->filedes, F_GETFD, 0);
+ if ((ffd >= 0) && (old_file->flags & APR_INHERIT)) {
+ ffd = fcntl((*new_file)->filedes, F_SETFD, ffd | FD_CLOEXEC);
+ }
+ /* if (ffd < 0)
+ * XXX: What to do in this case? No good ideas.
+ * returning an error implies we have to go
+ * back and close the original handle.
+ */
+#endif
return APR_SUCCESS;
}
# This code is set up to properly register our cleanups based on the
# the fd 0..2 (stdin/out/err) v.s. other handles. But the old code was
# simply broken - a closed file handle passed to apr_file_dup (e.g.
# one of the standard handles) would have resulted in no cleanup
# registered at all.
@@ -123,25 +149,19 @@
APR_DECLARE(apr_status_t) apr_file_dup(apr_file_t **new_file,
apr_file_t *old_file, apr_pool_t *p)
{
- apr_status_t rv;
-
- rv = _file_dup(new_file, old_file, p, 1);
- if (rv != APR_SUCCESS)
- return rv;
-
- /* we do this here as we don't want to double register an existing
- * apr_file_t for cleanup
- */
- apr_pool_cleanup_register((*new_file)->pool, (void *)(*new_file),
- apr_unix_file_cleanup, apr_unix_file_cleanup);
- return rv;
-
+ return _file_dup(new_file, old_file, p, 1);
}
APR_DECLARE(apr_status_t) apr_file_dup2(apr_file_t *new_file,
apr_file_t *old_file, apr_pool_t *p)
{
#ifdef NETWARE
+ /* XXX Netware must special-case any dup2(stdin/out/err,...)
+ * as provided by apr_file_open_std[in|out|err].
+ * At least we pre-close the target file...
+ */
+ apr_pool_cleanup_run(new_file->pool, new_file,
+ apr_unix_file_cleanup);
return _file_dup(&new_file, old_file, p, 1);
#else
return _file_dup(&new_file, old_file, p, 2);
# The changes in the front of _file_dup made all of this code
# simpler to follow, I hope.
@@ -177,7 +200,9 @@
if (!(old_file->flags & APR_FILE_NOCLEANUP)) {
apr_pool_cleanup_register(p, (void *)(*new_file),
apr_unix_file_cleanup,
- apr_unix_file_cleanup);
+ ((*new_file)->flags & APR_INHERIT)
+ ? apr_pool_cleanup_null
+ : apr_unix_file_cleanup);
}
old_file->filedes = -1;
# and once more, we set up the pool cleanup based on the setting
# of the inherit bit.
Re: [Patch rev 3] FD_CLOEXEC and APR_INHERIT
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 09:53 AM 3/20/2003, you wrote:
>On Tue, Mar 18, 2003 at 11:18:12PM -0600, William Rowe wrote:
>> Thanks to the feedback from Joe and Bjoern, I have committed a ton and 1/2
>> of bug fixes to apr's open.c and dupfile.c.
>>
>> After those changes, this patch is much easier to read. Please take a look
>> and comment on anything you see that might be amiss.
>
>It has two compile failures on Unix, so can I presume you haven't tested
>this in httpd on a Unix platform?
ACK.
I'm in the process of testing the current trees and have already received
confirmation from Bjoern that with the apr fixes *already* in the apr tree,
plus the patch you reposted on dev@httpd leaves us no further open fd's
beyond 0,1,2 when invoking CGI scripts.
That confirmation (I've asked him to fw: to the httpd list) gives me enough
confidence to call 0.9.2 ready - subject to tagging, testing, tarring and
the final +1's for release.
Of course Netware and OS2 should also be reviewed for the correct
behavior - but we've already got the 80/20 problem licked, and I'm unaware
of any mass-vhosters using Netware or OS2 to invoke untrusted CGIs.
If Netware or OS2 have any inheritance bug fixes, we can pick those up
on the word of those platform maintainers later tonight/tommorow morning.
Bill
Re: [Patch rev 3] FD_CLOEXEC and APR_INHERIT
Posted by "Bjoern A. Zeeb" <bz...@lists.zabbadoz.net>.
On Thu, 20 Mar 2003, Joe Orton wrote:
> On Tue, Mar 18, 2003 at 11:18:12PM -0600, William Rowe wrote:
> > Thanks to the feedback from Joe and Bjoern, I have committed a ton and 1/2
> > of bug fixes to apr's open.c and dupfile.c.
> >
> > After those changes, this patch is much easier to read. Please take a look
> > and comment on anything you see that might be amiss.
>
> It has two compile failures on Unix, so can I presume you haven't tested
> this in httpd on a Unix platform?
I just tested CVS co of apr, apr-util and httpd-2.0 on FreeBSD and it
compiled and worked fine.
--
Bjoern A. Zeeb bzeeb at Zabbadoz dot NeT
56 69 73 69 74 http://www.zabbadoz.net/
Re: [Patch rev 3] FD_CLOEXEC and APR_INHERIT
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Tue, Mar 18, 2003 at 11:18:12PM -0600, William Rowe wrote:
> Thanks to the feedback from Joe and Bjoern, I have committed a ton and 1/2
> of bug fixes to apr's open.c and dupfile.c.
>
> After those changes, this patch is much easier to read. Please take a look
> and comment on anything you see that might be amiss.
It has two compile failures on Unix, so can I presume you haven't tested
this in httpd on a Unix platform?
joe
Re: [Patch rev 3 - attached] FD_CLOEXEC and APR_INHERIT
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 11:18 PM 3/18/2003, William A. Rowe, Jr. wrote:
>Thanks to the feedback from Joe and Bjoern, I have committed a ton and 1/2
>of bug fixes to apr's open.c and dupfile.c.
>
>After those changes, this patch is much easier to read. Please take a look
>and comment on anything you see that might be amiss.
I swear the cat must have walked across the keyboard... I never clicked
send that *I* remember. Anyways, patch is attached.
[Patch rev 3] FD_CLOEXEC and APR_INHERIT
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Thanks to the feedback from Joe and Bjoern, I have committed a ton and 1/2
of bug fixes to apr's open.c and dupfile.c.
After those changes, this patch is much easier to read. Please take a look
and comment on anything you see that might be amiss.
If there are ANY remaining bugs in the current implementation of APR_INHERIT
(ignoring the non-apr fork()/exec() issues that this patch addresses now) they
should be fixed immediately.
I reserve my extreme opinions for Win32 matters, I won't express any opinion
on wether or not this patch should be in 0.9.2. I offer it for completeness.
Provided the developer is using apr_proc_create, today they should end up
with no unintended handles in Unix child processes from our apr_file_t or
apr_socket_t entities. If this is not true, scream loudly.
I'm actually concerned about inheritance of the various semaphores, mutexes,
locks and shm's. I'm sure I can count on Bjoern to let us know if we still have
work to do in those 'other departments'.
Oh, this patch has one flaw, and I'm hoping someone suggests an easy solve.
I do bother testing the APR_FILES_AS_SOCKETS to avoid trying to toggle
a value which would be quite invalid. However, that's only partially effective
since we use the same implement logic.
Perhaps we should we always provide both APR_IMPLEMENT_INHERIT_UNSET
and an alternate APR_IMPLEMENT_INHERIT_UNSET_CLOEXEC, and leave the
decision to use the _CLOEXEC flavor to open.c and sockets.c to decide?
Bill
Re: [Patch] for discussion of FD_CLOEXEC and APR_INHERIT
Posted by "Bjoern A. Zeeb" <bz...@lists.zabbadoz.net>.
On Mon, 17 Mar 2003, William A. Rowe, Jr. wrote:
Hi,
just skipped through; too late for everything more.
> --- file_io/unix/open.c 6 Mar 2003 09:24:17 -0000 1.110
> +++ file_io/unix/open.c 17 Mar 2003 22:58:30 -0000
...
> @@ -191,7 +213,10 @@
>
> if (!(flag & APR_FILE_NOCLEANUP)) {
> apr_pool_cleanup_register((*new)->pool, (void *)(*new),
> - apr_unix_file_cleanup, apr_unix_file_cleanup);
> + apr_unix_file_cleanup,
> + (flag & APR_INHERIT)
> + ? apr_unix_file_cleanup
> + : apr_pool_cleanup_null);
This should be ... ?
+ (flag & APR_INHERIT)
+ ? apr_pool_cleanup_null
+ : apr_unix_file_cleanup);
--
Bjoern A. Zeeb bzeeb at Zabbadoz dot NeT
56 69 73 69 74 http://www.zabbadoz.net/
tech support excuse #427:
Firmware update in the coffee machine
Re: [Patch] redux; discussion of FD_CLOEXEC and APR_INHERIT
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Tue, Mar 18, 2003 at 09:56:04PM +0000, Bjoern A. Zeeb wrote:
> On Tue, 18 Mar 2003, Joe Orton wrote:
> > 2. otherwise: even when you set CLOEXEC, the arbitrary binary which
> > can be fork/exec from an untrusted PHP script can then just use
> > ptrace() to arrange for any httpd child to run arbitrary code anyway,
> > thereby gaining access to all the fds you went to so much effort to
> > avoid leaking.
>
> But if I call an ordinary CGI script (be it shell,perl,C, php as CGI...)
> I may run it through suexec and it will be run under a completly different
> unprivileged uid and gid. It will not be able to access httpd
> resources if my file, etc. permissions are set up correctly, ...
> Same will for sure almost be true for cgid/mod_cgi.
You're confusing two issues: the fd leaks to CGI scripts in recent
versions of httpd-2.0 are caused entirely by inappropriate calls to
apr_file_inherit_set in httpd.
Adding CLOEXEC support to APR makes absolutely no difference to that.
Regards,
joe
Re: [Patch] redux; discussion of FD_CLOEXEC and APR_INHERIT
Posted by "Bjoern A. Zeeb" <bz...@lists.zabbadoz.net>.
On Tue, 18 Mar 2003, Joe Orton wrote:
Hi,
> "security risk" is that PHP will let you fork/exec any executable
we are not talking about mod_php here - at least I do not.
> 2. otherwise: even when you set CLOEXEC, the arbitrary binary which
> can be fork/exec from an untrusted PHP script can then just use
> ptrace() to arrange for any httpd child to run arbitrary code anyway,
> thereby gaining access to all the fds you went to so much effort to
> avoid leaking.
But if I call an ordinary CGI script (be it shell,perl,C, php as CGI...)
I may run it through suexec and it will be run under a completly different
unprivileged uid and gid. It will not be able to access httpd
resources if my file, etc. permissions are set up correctly, ...
Same will for sure almost be true for cgid/mod_cgi.
A CLOEXEC and correct handling from within APR for those exec()ing
cases will prevent those CGIs from inheriting a lot of file
descriptors that
a) _are_ a security risk (else the cgi perhaps wouldn't be able to
access these resources - see above) and
b) those fds eat a lot of per process resources so a CGI may run into
set process limits (set/getrlimit(2)) and
c) closing these descriptors in a wrapper like suexec would be doable
but is _very_ expensive as one may have to call a close(2) on all
possible open file descriptors apart from 012 what means looping
from 3 to the rlim_cur/rlim_max value of getrlimit(RLIMIT_NOFILE, &)
So APR can help me a lot as the internals already have a "list" of
open file descriptors and need to only close them thus elimination a),
b) and c).
Just one point on this.
--
Bjoern A. Zeeb bzeeb at Zabbadoz dot NeT
56 69 73 69 74 http://www.zabbadoz.net/
Re: [Patch] redux; discussion of FD_CLOEXEC and APR_INHERIT
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Tue, Mar 18, 2003 at 12:37:16PM -0600, William Rowe wrote:
> At 04:03 AM 3/18/2003, Joe Orton wrote:
> >On Mon, Mar 17, 2003 at 11:59:42PM -0600, William Rowe wrote:
> >...
> >> If Brad or Brian are available - I will need your eyes on the Unix
> >> patches - and I don't want to go weeks before we release 0.9.2.
> >> If we can address these issues on those platforms by Wed that
> >> would be *really* terrific!
> >
> >So why not release 0.9.2 yesterday and add this in afterwards? This is a
> >pretty significant change in behaviour.
>
> Because the existing code is a security risk to some applications, and
> the security team has reinforced that the ASF shouldn't be dropping
> code that has known issues. See Bjoern's earlier posts for additional,
> very valid arguments.
What "security risk" is not setting CLOEXEC fixing for APR
applications? The argument I've seen that not setting CLOEXEC is a
"security risk" is that PHP will let you fork/exec any executable
which then inherits all the fds an httpd child has open.
1. if you care about that, turn on PHP safe mode, and don't let
script authors fork/exec anything at all.
2. otherwise: even when you set CLOEXEC, the arbitrary binary which
can be fork/exec from an untrusted PHP script can then just use
ptrace() to arrange for any httpd child to run arbitrary code anyway,
thereby gaining access to all the fds you went to so much effort to
avoid leaking.
I think this issue is getting over-hyped.
joe
Re: [Patch] redux; discussion of FD_CLOEXEC and APR_INHERIT
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 04:03 AM 3/18/2003, Joe Orton wrote:
>On Mon, Mar 17, 2003 at 11:59:42PM -0600, William Rowe wrote:
>...
>> If Brad or Brian are available - I will need your eyes on the Unix
>> patches - and I don't want to go weeks before we release 0.9.2.
>> If we can address these issues on those platforms by Wed that
>> would be *really* terrific!
>
>So why not release 0.9.2 yesterday and add this in afterwards? This is a
>pretty significant change in behaviour.
Because the existing code is a security risk to some applications, and
the security team has reinforced that the ASF shouldn't be dropping
code that has known issues. See Bjoern's earlier posts for additional,
very valid arguments.
I'm offering to help with OS2/Netware - but the extra eyes are necessary
(as your list of issues pointed out - I'm adopting all of the changes and
will point out some confusion in a bit.)
Regarding confusion though - we call that 'other cleanup' our child cleanup.
The fact that we call fork()ed worker processes for httpd 'children' is where
my original confusion stemmed from. That's where your confusion about
Win32 'children' came from. Win32 has 'child' workers, Unix has fork()ed
workers. I don't know what we can do to eliminate that confusion in the
future for httpd/apr developers - but we aught to do something :-)
Look, some people think 0.9.2 is just the next toy for developers, but
as some RPMers have pointed out - this will be in releases of their
next builds and bundles. Since this seems to be the last major
obstacle for httpd and svn users - let's fix it, saving other less
significant nits for the next release.
httpd 2.0.45 will be released on APR's tag! I think that is a milestone
but I'd have to go back and look. I suggest that is progress for our
little effort here, and worth this extra little effort.
Bill
Re: [Patch] redux; discussion of FD_CLOEXEC and APR_INHERIT
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Mon, Mar 17, 2003 at 11:59:42PM -0600, William Rowe wrote:
...
> If Brad or Brian are available - I will need your eyes on the Unix
> patches - and I don't want to go weeks before we release 0.9.2.
> If we can address these issues on those platforms by Wed that
> would be *really* terrific!
So why not release 0.9.2 yesterday and add this in afterwards? This is a
pretty significant change in behaviour.
The unrelated logic changes apr_file_open and _file_dup make this patch
hard to review.
* for fcntl(fd, F_GETFD) you don't need the third ,0 argument
* you set FD_CLOEXEC #ifdef FD_CLOEXEC but for the #ifndef FD_CLOEXEC
case the child_cleanup is still passed as NULL.
* unix_file_cleanup does more work than just close() - it flushes and
implements DELONCLOSE. Is it really OK not to register this as a
child_cleanup if CLOEXEC is set? It certainly changes the behaviour.
* in open.c you can move the #ifdef FD_CLOEXEC can be moved after the fd
< 0 check to simplify
* what about all the new error paths you haven't decided how to handle?
Regards,
joe
Re: [Patch] redux; discussion of FD_CLOEXEC and APR_INHERIT
Posted by Brian Havard <br...@kheldar.apana.org.au>.
On Mon, 17 Mar 2003 23:59:42 -0600, William A. Rowe, Jr. wrote:
>At 11:47 PM 3/17/2003, William A. Rowe, Jr. wrote:
>>With a small typo pointed out by Bjoern, and fixing at least the
>>prototypes for the socket v.s. file implementations of inherit, here
>>is the revised patch. It is 'theoretical' - I'll vet it on OS/X in
>>the morning.
>
>YOW! Please ignore the OS2 work I started and abandoned!!!
>
>I was first attempting to fix the Unix implementation. OS2 and Netware
>will both obviously need their own pass through the file. The foo/os2/bar.c
>patches are irrelevant, and won't be committed as-is.
>
>If Brad or Brian are available - I will need your eyes on the Unix
>patches - and I don't want to go weeks before we release 0.9.2.
>If we can address these issues on those platforms by Wed that
>would be *really* terrific!
I haven't really been following this thread, can you summarize the issue?
--
______________________________________________________________________________
| Brian Havard | "He is not the messiah! |
| brianh@kheldar.apana.org.au | He's a very naughty boy!" - Life of Brian |
------------------------------------------------------------------------------
Re: [Patch] redux; discussion of FD_CLOEXEC and APR_INHERIT
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 11:47 PM 3/17/2003, William A. Rowe, Jr. wrote:
>With a small typo pointed out by Bjoern, and fixing at least the
>prototypes for the socket v.s. file implementations of inherit, here
>is the revised patch. It is 'theoretical' - I'll vet it on OS/X in
>the morning.
YOW! Please ignore the OS2 work I started and abandoned!!!
I was first attempting to fix the Unix implementation. OS2 and Netware
will both obviously need their own pass through the file. The foo/os2/bar.c
patches are irrelevant, and won't be committed as-is.
If Brad or Brian are available - I will need your eyes on the Unix
patches - and I don't want to go weeks before we release 0.9.2.
If we can address these issues on those platforms by Wed that
would be *really* terrific!
Bill
[Patch] redux; discussion of FD_CLOEXEC and APR_INHERIT
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
With a small typo pointed out by Bjoern, and fixing at least the
prototypes for the socket v.s. file implementations of inherit, here
is the revised patch. It is 'theoretical' - I'll vet it on OS/X in
the morning.
I have one question; should we also be toggling sockets as
FD_CLOEXEC? Common sense tells me, yes - we should
set that flag by default in all the assignments scattered
throughout network_io/unix/sockets.c.
Enjoy the patch... comments greatly appreciated. It's a little
to quiet on this issue... and your *win32* hacker is coding the
Unix solution. Are you scared yet :-?
Bill
A: well, at least you should be...