You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by wr...@apache.org on 2002/01/08 04:45:09 UTC
cvs commit: apr/file_io/unix filedup.c
wrowe 02/01/07 19:45:09
Modified: file_io/unix filedup.c
Log:
We cannot close-on-fork any fd 0 through 2 (stdin, stdout, stderr)!!!
This patch is possibly still borked, we probably should remove any
existing cleanup registered again (*new_file) if it was given.
This api really is dirty, should really have an apr_file_dup2() with
different conventions (passing apr_file_t* for both left and right args.)
I can see users 'forgetting' to null the target apr_file_t** destination.
Revision Changes Path
1.37 +13 -4 apr/file_io/unix/filedup.c
Index: filedup.c
===================================================================
RCS file: /home/cvs/apr/file_io/unix/filedup.c,v
retrieving revision 1.36
retrieving revision 1.37
diff -u -r1.36 -r1.37
--- filedup.c 6 Dec 2001 13:43:45 -0000 1.36
+++ filedup.c 8 Jan 2002 03:45:09 -0000 1.37
@@ -92,11 +92,20 @@
/* make sure unget behavior is consistent */
(*new_file)->ungetchar = old_file->ungetchar;
/* apr_file_dup() clears the inherit attribute, user must call
- * apr_file_set_inherit() again on the dupped handle, as necessary.
+ * apr_file_set_inherit() again on the dupped handle, as necessary,
+ * unless you have dup2'ed fd 0-2 (stdin, stdout or stderr) which
+ * should never, never, never close on fork()
*/
- (*new_file)->flags = old_file->flags & ~APR_INHERIT;
- apr_pool_cleanup_register((*new_file)->cntxt, (void *)(*new_file),
- apr_unix_file_cleanup, apr_unix_file_cleanup);
+ if (have_file && old_file->filedes >= 0 and old_file->filedes <= 2) {
+ (*new_file)->flags = old_file->flags | APR_INHERIT;
+ apr_pool_cleanup_register((*new_file)->cntxt, (void *)(*in),
+ apr_unix_file_cleanup, apr_pool_cleanup_null);
+ }
+ else {
+ (*new_file)->flags = old_file->flags & ~APR_INHERIT;
+ apr_pool_cleanup_register((*new_file)->cntxt, (void *)(*new_file),
+ apr_unix_file_cleanup, apr_unix_file_cleanup);
+ }
return APR_SUCCESS;
}
Re: cvs commit: apr/file_io/unix filedup.c
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "David Reid" <dr...@jetnet.co.uk>
Sent: Tuesday, January 08, 2002 11:51 AM
> But the patch didn't actually do anything to prevent the problem we saw as
> it was in NO way related to a fork(). The issue is that if we call a
> pool_clear we'll run the cleanup's anyway, as we should.
That's right. This is why my last patch dropped ALL cleanups of the fd,
for fd 0, 1, and 2. I agree my first patch was of dubious value :)
Bill
Re: cvs commit: apr/file_io/unix filedup.c
Posted by David Reid <dr...@jetnet.co.uk>.
But the patch didn't actually do anything to prevent the problem we saw as
it was in NO way related to a fork(). The issue is that if we call a
pool_clear we'll run the cleanup's anyway, as we should.
I think I'd be happier just letting the user decide and not getting all
motherly and trying to keep stdin/out/err open at all costs. It could be
that the user wants them closed! It's perfectly OK to try and close stderr,
open a pipe and get stderr as one end, if that what you *want* to happen.
(can't think why, but it should be possible).
Httpd was a simple case, but once again the correct solution was concerned
with pool lifetimes, not the opening/closing of fd's. In fact my tree
hasn't got your commit in it, and it runs like a dream once we use plog :)
We've said it time and time again, we're only a library and if people want
to use us to do stupid stuff, that's not our concern.
I have a patch that adds dup2, just need some test's for it.
david
> > Sorry, but I really don't like this patch.
> >
> > The problem that we had wouldn't have been addressed by this patch
anyways,
> > and we are a library - so why are we trying to tell users what they can
and
> > can't do?
> >
> > I'd like to revert this patch.
> >
> > I think we should also have a _dup2 version of this function, but I
think we
> > shoudl basically do just what dup/dup2 do and not try to get overly
fancy.
> > We're a library, not a babysitter!
>
> I agree with dup2. It's easy for the user to forget to null out a **
target.
>
> But as far as this patch is concerned [the final version, where we never
close
> fd's 0, 1, or 2 on cleanup], I disagree. It's too trivial for a race
condition
> to intervene, between the user clearing a pool, and then dup'ing the
replacement
> handle, in which time another thread could call fork.
>
> Httpd is too simple an example of how bad this could be. Debugging such
> a problem is a real PITA (as you discovered :) We should never expose
fork to
> this risk again, IMHO.
>
> Bill
Re: cvs commit: apr/file_io/unix filedup.c
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
----- Original Message -----
From: "David Reid" <dr...@jetnet.co.uk>
To: <de...@apr.apache.org>
Sent: Tuesday, January 08, 2002 10:01 AM
Subject: Re: cvs commit: apr/file_io/unix filedup.c
> Sorry, but I really don't like this patch.
>
> The problem that we had wouldn't have been addressed by this patch anyways,
> and we are a library - so why are we trying to tell users what they can and
> can't do?
>
> I'd like to revert this patch.
>
> I think we should also have a _dup2 version of this function, but I think we
> shoudl basically do just what dup/dup2 do and not try to get overly fancy.
> We're a library, not a babysitter!
I agree with dup2. It's easy for the user to forget to null out a ** target.
But as far as this patch is concerned [the final version, where we never close
fd's 0, 1, or 2 on cleanup], I disagree. It's too trivial for a race condition
to intervene, between the user clearing a pool, and then dup'ing the replacement
handle, in which time another thread could call fork.
Httpd is too simple an example of how bad this could be. Debugging such
a problem is a real PITA (as you discovered :) We should never expose fork to
this risk again, IMHO.
Bill
> > wrowe 02/01/07 19:45:09
> >
> > Modified: file_io/unix filedup.c
> > Log:
> > We cannot close-on-fork any fd 0 through 2 (stdin, stdout, stderr)!!!
> > This patch is possibly still borked, we probably should remove any
> > existing cleanup registered again (*new_file) if it was given.
> >
> > This api really is dirty, should really have an apr_file_dup2() with
> > different conventions (passing apr_file_t* for both left and right
> args.)
> > I can see users 'forgetting' to null the target apr_file_t**
> destination.
> >
>
>
>
Re: cvs commit: apr/file_io/unix filedup.c
Posted by David Reid <dr...@jetnet.co.uk>.
Sorry, but I really don't like this patch.
The problem that we had wouldn't have been addressed by this patch anyways,
and we are a library - so why are we trying to tell users what they can and
can't do?
I'd like to revert this patch.
I think we should also have a _dup2 version of this function, but I think we
shoudl basically do just what dup/dup2 do and not try to get overly fancy.
We're a library, not a babysitter!
david
> wrowe 02/01/07 19:45:09
>
> Modified: file_io/unix filedup.c
> Log:
> We cannot close-on-fork any fd 0 through 2 (stdin, stdout, stderr)!!!
> This patch is possibly still borked, we probably should remove any
> existing cleanup registered again (*new_file) if it was given.
>
> This api really is dirty, should really have an apr_file_dup2() with
> different conventions (passing apr_file_t* for both left and right
args.)
> I can see users 'forgetting' to null the target apr_file_t**
destination.
>