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/21 18:37:48 UTC
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
At 10:48 AM 3/21/2003, Jeff Trawick wrote:
>Joe Orton wrote:
>>
>>In the r1.57 filedup.c change, a cleanup is registered for the "new"
>>fd coming out of apr_file_dup2(), which wasn't happening previously.
>>I'm guessing that this cleanup is closing fd 2 when pconf is cleared, or
>>something like that.
>
>backtracking uses of fd 2 up through this time is somewhat funny
>
>it looks like
>
>a) we set the error log to file descriptor 2
>
>b) we close file descriptor 2 and set it to /dev/null via the freopen() in apr_proc_detach
>
>c) we close file descriptor 2 (because of cleanup registered around step a??)
Bang. We are dup2()ing that fd, and that fd didn't have a cleanup
(in fact it was opened APR_FILE_NOCLOSE).
Attached is a patch that uses the logic
apr_file_dup() always registers a cleanup, inherited for 0..2,
otherwise not inherited by default.
apr_file_dup2() registers the same cleanup the original open()
or the toggled apr_file_inherit_[un]set had indicated. We still
don't trust the original cleanup, but register the 'proper' one
based on the desired behavior of that target apr_file_t.
Give it a whirl and I will commit if you folks concur.
Bill
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 02:36 PM 3/21/2003, Jeff Trawick wrote:
>>>Why isn't the problem this:
>>>
>>>Apache opened stderr from the wrong pool, or failed to open it again when the original pool for stderr was cleaned up; in other words, it was cleaned up because of a pool lifetime problem in the application, not because 0-2 inherently need to be handled in a different special way
>>
>>Because we agreed that the API would support APR_FILE_NOCLOSE and
>>that is how apr_file_stderr_open() works.
>
>you mean APR_FILE_NOCLEANUP?
right...
>I'll buy that, except that I can't see the connection between apr_file_open_stderr() and APR_FILE_NOCLEANUP
simple;
>APR_DECLARE(apr_status_t) apr_file_open_stderr(apr_file_t **thefile,
> apr_pool_t *pool)
>{
> int fd = STDERR_FILENO;
>
> return apr_os_file_put(thefile, &fd, 0, pool);
>}
The bottom line? apr_os_file_put has always stuffed an fd into a handle,
and registered no cleanup. Our change to dup2() began registering
a cleanup - which is the side effect you were watching this morning.
So it's impossible to toggle the APR_INHERIT bit (which flips between
a child cleanup of apr_pool_cleanup_null and apr_unix_file_cleanup.)
If you look at the patch to apr_os_file_put file_io/unix/open.c rev 1.111
you will see that we now reflect in the ->flags that no cleanup has
been registered (APR_FILE_NOCLEANUP).
Now I foresaw the apr_os_file_put side of the coin, but we need to start
respecting those two bits in apr_file_dup2. We still can't be sure that
the old cleanup is valid (did the user call apr_file_close() before dup2?
if so the cleanup is now broke), but we sure can pre-kill the old cleanup
and then register the appropriate cleanup based on the ->flags value.
Please let me know if the patch picks back up the correct behavior.
Bill
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 02:36 PM 3/21/2003, Jeff Trawick wrote:
>>>Why isn't the problem this:
>>>
>>>Apache opened stderr from the wrong pool, or failed to open it again when the original pool for stderr was cleaned up; in other words, it was cleaned up because of a pool lifetime problem in the application, not because 0-2 inherently need to be handled in a different special way
>>
>>Because we agreed that the API would support APR_FILE_NOCLOSE and
>>that is how apr_file_stderr_open() works.
>
>you mean APR_FILE_NOCLEANUP?
right...
>I'll buy that, except that I can't see the connection between apr_file_open_stderr() and APR_FILE_NOCLEANUP
simple;
>APR_DECLARE(apr_status_t) apr_file_open_stderr(apr_file_t **thefile,
> apr_pool_t *pool)
>{
> int fd = STDERR_FILENO;
>
> return apr_os_file_put(thefile, &fd, 0, pool);
>}
The bottom line? apr_os_file_put has always stuffed an fd into a handle,
and registered no cleanup. Our change to dup2() began registering
a cleanup - which is the side effect you were watching this morning.
So it's impossible to toggle the APR_INHERIT bit (which flips between
a child cleanup of apr_pool_cleanup_null and apr_unix_file_cleanup.)
If you look at the patch to apr_os_file_put file_io/unix/open.c rev 1.111
you will see that we now reflect in the ->flags that no cleanup has
been registered (APR_FILE_NOCLEANUP).
Now I foresaw the apr_os_file_put side of the coin, but we need to start
respecting those two bits in apr_file_dup2. We still can't be sure that
the old cleanup is valid (did the user call apr_file_close() before dup2?
if so the cleanup is now broke), but we sure can pre-kill the old cleanup
and then register the appropriate cleanup based on the ->flags value.
Please let me know if the patch picks back up the correct behavior.
Bill
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by Jeff Trawick <tr...@attglobal.net>.
William A. Rowe, Jr. wrote:
> At 01:09 PM 3/21/2003, Jeff Trawick wrote:
>
>
>>I'm a bit nervous about the existing/new special handling for descriptors 0-2.
>
>
> Are you looking at my original or new patch?
either
> I won't disagree, if you want apr_file_dup() to always return an uninherited
> handle, that isn't a problem. Just look in the which_dup == 1 logic of my
> patch and change that block to always untoggle APR_INHERIT (and we
> should also untoggle APR_FILE_NOCLOSE for apr_file_dup() results,
> now that I think about it.)
>
>
>>Why isn't the problem this:
>>
>>Apache opened stderr from the wrong pool, or failed to open it again when the original pool for stderr was cleaned up; in other words, it was cleaned up because of a pool lifetime problem in the application, not because 0-2 inherently need to be handled in a different special way
>
>
> Because we agreed that the API would support APR_FILE_NOCLOSE and
> that is how apr_file_stderr_open() works.
you mean APR_FILE_NOCLEANUP? I'll buy that, except that I can't see the
connection between apr_file_open_stderr() and APR_FILE_NOCLEANUP
APR_DECLARE(apr_status_t) apr_file_open_stderr(apr_file_t **thefile,
apr_pool_t *pool)
{
int fd = STDERR_FILENO;
return apr_os_file_put(thefile, &fd, 0, pool);
}
>>(and maybe in addition: Apache should be able to tell apr_proc_detach() to leave stderr alone if it wants fd 2 to be unchanged across the detach)
>
>
> Hmmm. Interesting point, but I think that if we just respect the values
> for APR_FILE_NOCLOSE and APR_INHERIT - as my most recent patch
> suggests, that this problem should simply go away.
I suspect that it will "simply go away" in that it will "not be
noticably broken" since we will get fd 2 pointing to the error log again
instead of /dev/null before too many opportunities for writing to stderr
go by.
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by Jeff Trawick <tr...@attglobal.net>.
William A. Rowe, Jr. wrote:
> At 01:09 PM 3/21/2003, Jeff Trawick wrote:
>
>
>>I'm a bit nervous about the existing/new special handling for descriptors 0-2.
>
>
> Are you looking at my original or new patch?
either
> I won't disagree, if you want apr_file_dup() to always return an uninherited
> handle, that isn't a problem. Just look in the which_dup == 1 logic of my
> patch and change that block to always untoggle APR_INHERIT (and we
> should also untoggle APR_FILE_NOCLOSE for apr_file_dup() results,
> now that I think about it.)
>
>
>>Why isn't the problem this:
>>
>>Apache opened stderr from the wrong pool, or failed to open it again when the original pool for stderr was cleaned up; in other words, it was cleaned up because of a pool lifetime problem in the application, not because 0-2 inherently need to be handled in a different special way
>
>
> Because we agreed that the API would support APR_FILE_NOCLOSE and
> that is how apr_file_stderr_open() works.
you mean APR_FILE_NOCLEANUP? I'll buy that, except that I can't see the
connection between apr_file_open_stderr() and APR_FILE_NOCLEANUP
APR_DECLARE(apr_status_t) apr_file_open_stderr(apr_file_t **thefile,
apr_pool_t *pool)
{
int fd = STDERR_FILENO;
return apr_os_file_put(thefile, &fd, 0, pool);
}
>>(and maybe in addition: Apache should be able to tell apr_proc_detach() to leave stderr alone if it wants fd 2 to be unchanged across the detach)
>
>
> Hmmm. Interesting point, but I think that if we just respect the values
> for APR_FILE_NOCLOSE and APR_INHERIT - as my most recent patch
> suggests, that this problem should simply go away.
I suspect that it will "simply go away" in that it will "not be
noticably broken" since we will get fd 2 pointing to the error log again
instead of /dev/null before too many opportunities for writing to stderr
go by.
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 01:09 PM 3/21/2003, Jeff Trawick wrote:
>I'm a bit nervous about the existing/new special handling for descriptors 0-2.
Are you looking at my original or new patch?
I won't disagree, if you want apr_file_dup() to always return an uninherited
handle, that isn't a problem. Just look in the which_dup == 1 logic of my
patch and change that block to always untoggle APR_INHERIT (and we
should also untoggle APR_FILE_NOCLOSE for apr_file_dup() results,
now that I think about it.)
>Why isn't the problem this:
>
>Apache opened stderr from the wrong pool, or failed to open it again when the original pool for stderr was cleaned up; in other words, it was cleaned up because of a pool lifetime problem in the application, not because 0-2 inherently need to be handled in a different special way
Because we agreed that the API would support APR_FILE_NOCLOSE and
that is how apr_file_stderr_open() works.
>(and maybe in addition: Apache should be able to tell apr_proc_detach() to leave stderr alone if it wants fd 2 to be unchanged across the detach)
Hmmm. Interesting point, but I think that if we just respect the values
for APR_FILE_NOCLOSE and APR_INHERIT - as my most recent patch
suggests, that this problem should simply go away.
Bill
Resolved: [Patches] options for filedup.
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 04:49 PM 3/21/2003, William A. Rowe, Jr. wrote:
>apr_dupfix.patch keeps the cleanup_kill -> dup2 -> register_cleanup
>logic that protects us from dup2'ing into an apr_file_close()ed fd.
>
>apr_duprevert.patch keeps the reorganization of the code, but drops
>the cleanup_kill and register_cleanup in the dup2 case, in favor of
>trusting the current cleanups (and that they correpond to ->flags.)
I commited the second apr_duprevert flavor that reflects Joe's concern
with too many recent changes. This restores the old behavior of _dup2().
>In *BOTH* patches we revert apr_file_dup() assumption that fd 0..2 are
>inherited, to reflect Jeff's concern with that change.
This patch was applied separately, as I agree with Jeff and expressed
the concern in my original commit message.
I've checked out on OS/X... others are encourgaged to cvs up their
apr trees and see if the oddities introduced Wed night are sufficiently
resolved. I have gone ahead and tagged the final tag and will roll in
the morning if Sander or Aaron don't beat me to it. (I need to do
some magic to get a good build schema with all the bits that the
release.sh script expects.)
Sorry for the temporary breakage, and my enthusiasm for addressing
these real sorts of concerns with only Joe, and later Jeff also willing
to participate in an effort in a usable and yet somewhat more secure
release.
Bill
Resolved: [Patches] options for filedup.
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 04:49 PM 3/21/2003, William A. Rowe, Jr. wrote:
>apr_dupfix.patch keeps the cleanup_kill -> dup2 -> register_cleanup
>logic that protects us from dup2'ing into an apr_file_close()ed fd.
>
>apr_duprevert.patch keeps the reorganization of the code, but drops
>the cleanup_kill and register_cleanup in the dup2 case, in favor of
>trusting the current cleanups (and that they correpond to ->flags.)
I commited the second apr_duprevert flavor that reflects Joe's concern
with too many recent changes. This restores the old behavior of _dup2().
>In *BOTH* patches we revert apr_file_dup() assumption that fd 0..2 are
>inherited, to reflect Jeff's concern with that change.
This patch was applied separately, as I agree with Jeff and expressed
the concern in my original commit message.
I've checked out on OS/X... others are encourgaged to cvs up their
apr trees and see if the oddities introduced Wed night are sufficiently
resolved. I have gone ahead and tagged the final tag and will roll in
the morning if Sander or Aaron don't beat me to it. (I need to do
some magic to get a good build schema with all the bits that the
release.sh script expects.)
Sorry for the temporary breakage, and my enthusiasm for addressing
these real sorts of concerns with only Joe, and later Jeff also willing
to participate in an effort in a usable and yet somewhat more secure
release.
Bill
[Patches] two options for filedup.
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Here are two patches that address the current bug, for comparison.
apr_dupfix.patch keeps the cleanup_kill -> dup2 -> register_cleanup
logic that protects us from dup2'ing into an apr_file_close()ed fd.
apr_duprevert.patch keeps the reorganization of the code, but drops
the cleanup_kill and register_cleanup in the dup2 case, in favor of
trusting the current cleanups (and that they correpond to ->flags.)
I honestly like the second patch better, only if we agree to not support
apr_file_dup2()ing into any apr_file_t that's already been apr_file_close()d.
Supporting that behavior will be a PITA on Win32.
In *BOTH* patches we revert apr_file_dup() assumption that fd 0..2 are
inherited, to reflect Jeff's concern with that change.
Please express a preference and I'll commit the favored patch and
test on OS/X prior to T&R of 0.9.2.
Bill
[Patches] two options for filedup.
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Here are two patches that address the current bug, for comparison.
apr_dupfix.patch keeps the cleanup_kill -> dup2 -> register_cleanup
logic that protects us from dup2'ing into an apr_file_close()ed fd.
apr_duprevert.patch keeps the reorganization of the code, but drops
the cleanup_kill and register_cleanup in the dup2 case, in favor of
trusting the current cleanups (and that they correpond to ->flags.)
I honestly like the second patch better, only if we agree to not support
apr_file_dup2()ing into any apr_file_t that's already been apr_file_close()d.
Supporting that behavior will be a PITA on Win32.
In *BOTH* patches we revert apr_file_dup() assumption that fd 0..2 are
inherited, to reflect Jeff's concern with that change.
Please express a preference and I'll commit the favored patch and
test on OS/X prior to T&R of 0.9.2.
Bill
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Fri, Mar 21, 2003 at 01:56:52PM -0600, William Rowe wrote:
> At 01:09 PM 3/21/2003, Jeff Trawick wrote:
>
> >I'm a bit nervous about the existing/new special handling for descriptors 0-2.
>
> Ok, here is a patch that uses the target file's inherit and noclose
> flag bits for apr_file_dup2(), and leaves apr_file_dup() results as
> never inherited and always registering a cleanup.
>
> Would this patch satisfy your concerns? Does this patch resolve
> the new misbehavior?
This patch does fix the problem I was seeing. To play the conservative
again, why not just back down to the r1.56 filedup.c for the new APR
0.9.2 release (and presumably hence the next httpd 2.0 release), and
stop trying to slip in these behaviour changes at the last minute
without testing them first?
Regards,
joe
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by Joe Orton <jo...@manyfish.co.uk>.
On Fri, Mar 21, 2003 at 01:56:52PM -0600, William Rowe wrote:
> At 01:09 PM 3/21/2003, Jeff Trawick wrote:
>
> >I'm a bit nervous about the existing/new special handling for descriptors 0-2.
>
> Ok, here is a patch that uses the target file's inherit and noclose
> flag bits for apr_file_dup2(), and leaves apr_file_dup() results as
> never inherited and always registering a cleanup.
>
> Would this patch satisfy your concerns? Does this patch resolve
> the new misbehavior?
This patch does fix the problem I was seeing. To play the conservative
again, why not just back down to the r1.56 filedup.c for the new APR
0.9.2 release (and presumably hence the next httpd 2.0 release), and
stop trying to slip in these behaviour changes at the last minute
without testing them first?
Regards,
joe
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 01:09 PM 3/21/2003, Jeff Trawick wrote:
>I'm a bit nervous about the existing/new special handling for descriptors 0-2.
Ok, here is a patch that uses the target file's inherit and noclose
flag bits for apr_file_dup2(), and leaves apr_file_dup() results as
never inherited and always registering a cleanup.
Would this patch satisfy your concerns? Does this patch resolve
the new misbehavior?
Bill
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 01:09 PM 3/21/2003, Jeff Trawick wrote:
>I'm a bit nervous about the existing/new special handling for descriptors 0-2.
Are you looking at my original or new patch?
I won't disagree, if you want apr_file_dup() to always return an uninherited
handle, that isn't a problem. Just look in the which_dup == 1 logic of my
patch and change that block to always untoggle APR_INHERIT (and we
should also untoggle APR_FILE_NOCLOSE for apr_file_dup() results,
now that I think about it.)
>Why isn't the problem this:
>
>Apache opened stderr from the wrong pool, or failed to open it again when the original pool for stderr was cleaned up; in other words, it was cleaned up because of a pool lifetime problem in the application, not because 0-2 inherently need to be handled in a different special way
Because we agreed that the API would support APR_FILE_NOCLOSE and
that is how apr_file_stderr_open() works.
>(and maybe in addition: Apache should be able to tell apr_proc_detach() to leave stderr alone if it wants fd 2 to be unchanged across the detach)
Hmmm. Interesting point, but I think that if we just respect the values
for APR_FILE_NOCLOSE and APR_INHERIT - as my most recent patch
suggests, that this problem should simply go away.
Bill
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 01:09 PM 3/21/2003, Jeff Trawick wrote:
>I'm a bit nervous about the existing/new special handling for descriptors 0-2.
Ok, here is a patch that uses the target file's inherit and noclose
flag bits for apr_file_dup2(), and leaves apr_file_dup() results as
never inherited and always registering a cleanup.
Would this patch satisfy your concerns? Does this patch resolve
the new misbehavior?
Bill
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by Jeff Trawick <tr...@attglobal.net>.
William A. Rowe, Jr. wrote:
> At 10:48 AM 3/21/2003, Jeff Trawick wrote:
>
>>backtracking uses of fd 2 up through this time is somewhat funny
>>
>>it looks like
>>
>>a) we set the error log to file descriptor 2
>>
>>b) we close file descriptor 2 and set it to /dev/null via the freopen() in apr_proc_detach
>>
>>c) we close file descriptor 2 (because of cleanup registered around step a??)
>
>
> Bang. We are dup2()ing that fd, and that fd didn't have a cleanup
> (in fact it was opened APR_FILE_NOCLOSE).
>
> Attached is a patch that uses the logic
>
> apr_file_dup() always registers a cleanup, inherited for 0..2,
> otherwise not inherited by default.
>
> apr_file_dup2() registers the same cleanup the original open()
> or the toggled apr_file_inherit_[un]set had indicated. We still
> don't trust the original cleanup, but register the 'proper' one
> based on the desired behavior of that target apr_file_t.
I'm a bit nervous about the existing/new special handling for
descriptors 0-2.
Why isn't the problem this:
Apache opened stderr from the wrong pool, or failed to open it again
when the original pool for stderr was cleaned up; in other words, it was
cleaned up because of a pool lifetime problem in the application, not
because 0-2 inherently need to be handled in a different special way
(and maybe in addition: Apache should be able to tell apr_proc_detach()
to leave stderr alone if it wants fd 2 to be unchanged across the detach)
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by Jeff Trawick <tr...@attglobal.net>.
William A. Rowe, Jr. wrote:
> At 10:48 AM 3/21/2003, Jeff Trawick wrote:
>
>>backtracking uses of fd 2 up through this time is somewhat funny
>>
>>it looks like
>>
>>a) we set the error log to file descriptor 2
>>
>>b) we close file descriptor 2 and set it to /dev/null via the freopen() in apr_proc_detach
>>
>>c) we close file descriptor 2 (because of cleanup registered around step a??)
>
>
> Bang. We are dup2()ing that fd, and that fd didn't have a cleanup
> (in fact it was opened APR_FILE_NOCLOSE).
>
> Attached is a patch that uses the logic
>
> apr_file_dup() always registers a cleanup, inherited for 0..2,
> otherwise not inherited by default.
>
> apr_file_dup2() registers the same cleanup the original open()
> or the toggled apr_file_inherit_[un]set had indicated. We still
> don't trust the original cleanup, but register the 'proper' one
> based on the desired behavior of that target apr_file_t.
I'm a bit nervous about the existing/new special handling for
descriptors 0-2.
Why isn't the problem this:
Apache opened stderr from the wrong pool, or failed to open it again
when the original pool for stderr was cleaned up; in other words, it was
cleaned up because of a pool lifetime problem in the application, not
because 0-2 inherently need to be handled in a different special way
(and maybe in addition: Apache should be able to tell apr_proc_detach()
to leave stderr alone if it wants fd 2 to be unchanged across the detach)