You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Aaron Bannert <aa...@clove.org> on 2003/03/21 04:20:22 UTC
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
On Thursday, March 20, 2003, at 01:50 PM, wrowe@apache.org wrote:
> wrowe 2003/03/20 13:50:41
>
> Modified: . CHANGES
> modules/loggers mod_log_config.c
> modules/mappers mod_rewrite.c
> server log.c mpm_common.c
> server/mpm/worker pod.c
> Log:
> SECURITY: Eliminated leaks of several file descriptors to child
> processes, such as CGI scripts.
[...]
> apr_sockaddr_info_get(&(*pod)->sa,
> ap_listeners->bind_addr->hostname,
> APR_UNSPEC,
> ap_listeners->bind_addr->port, 0, p);
>
> + /* close these before exec. */
> + apr_file_unset_inherit((*pod)->pod_in);
> + apr_file_unset_inherit((*pod)->pod_out);
> +
> return APR_SUCCESS;
The PODs in the worker MPM are getting closed and the parent is then
unable to kill its children when it needs to (don't you love how
morbid that sounds?). I see one of these every second in the error log:
[Thu Mar 20 18:09:25 2003] [warn] (32)Broken pipe: write pipe_of_death
Since the unset_inherit() is being called from the open_logs hook, it's
happening in the parent process, which means that the fork for
the children is going to kill them off. We need to unset the inherit
*after* we are running in the child.
-aaron
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by "Bjoern A. Zeeb" <bz...@lists.zabbadoz.net>.
On Thu, 20 Mar 2003, Aaron Bannert wrote:
Hi,
> > Log:
> > SECURITY: Eliminated leaks of several file descriptors to child
> > processes, such as CGI scripts.
>
> [...]
>
> > apr_sockaddr_info_get(&(*pod)->sa,
> > ap_listeners->bind_addr->hostname,
> > APR_UNSPEC,
> > ap_listeners->bind_addr->port, 0, p);
> >
> > + /* close these before exec. */
> > + apr_file_unset_inherit((*pod)->pod_in);
> > + apr_file_unset_inherit((*pod)->pod_out);
> > +
> > return APR_SUCCESS;
>
> The PODs in the worker MPM are getting closed and the parent is then
> unable to kill its children when it needs to (don't you love how
> morbid that sounds?). I see one of these every second in the error log:
>
> [Thu Mar 20 18:09:25 2003] [warn] (32)Broken pipe: write pipe_of_death
>
> Since the unset_inherit() is being called from the open_logs hook, it's
> happening in the parent process, which means that the fork for
> the children is going to kill them off. We need to unset the inherit
> *after* we are running in the child.
I am not really familiar with worker but what about this (untested) ?
does
a) pod work again and
b) are the fd's still closed on exec ?
--- httpd-2.0/server/mpm/worker/pod.c.orig Fri Mar 21 08:20:07 2003
+++ httpd-2.0/server/mpm/worker/pod.c Fri Mar 21 08:20:27 2003
@@ -75,10 +75,6 @@
apr_file_pipe_timeout_set((*pod)->pod_in, 0);
*/
(*pod)->p = p;
-
- /* close these before exec. */
- apr_file_unset_inherit((*pod)->pod_in);
- apr_file_unset_inherit((*pod)->pod_out);
return APR_SUCCESS;
}
--- httpd-2.0/server/mpm/worker/worker.c.orig Fri Mar 21 08:20:31 2003
+++ httpd-2.0/server/mpm/worker/worker.c Fri Mar 21 08:21:48 2003
@@ -1387,6 +1387,10 @@
#endif
RAISE_SIGSTOP(MAKE_CHILD);
+ /* close these before exec. */
+ apr_file_unset_inherit((*pod)->pod_in);
+ apr_file_unset_inherit((*pod)->pod_out);
+
apr_signal(SIGTERM, just_die);
child_main(slot);
Else we would need s.th. in apr that only sets child_cleanup_fn and
not both I think ...
--
Bjoern A. Zeeb bzeeb at Zabbadoz dot NeT
56 69 73 69 74 http://www.zabbadoz.net/
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 10:23 AM 3/21/2003, 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.
Clarify this; it's true that 1.56 didn't register a child cleaup for the dup2()
flavor... it's also true that 1.56 and prior never *killed* an existing file
cleanup. Having (or not having) a cleanup for dup2() altogether depended
upon what was done with the target file prior to calling dup2().
A graphic might help
Old behavior (1.56 and prior):
apr_file_dup2(new, old)
\-> replace new->filedes with dup2()ed copy of old->filedes
apr_file_dup(*new, old)
\-> initialize new->filedes with dup(old->filedes)
\-> Register cleanup for (new)
New behavior (1.57):
apr_file_dup2(new, old)
\-> Kill (existing?) cleanup for (new)
\-> replace new->filedes with dup2()ed copy of old->filedes
\-> Register cleanup for (new)
apr_file_dup(*new, old)
\-> initialize new->filedes with dup(old->filedes)
\-> Register cleanup for (new)
So instead of shaking the dice of 'is there already a cleanup?', dup2()
flavor now assures we kill the old and register the new cleanup.
Another, perhaps more significant change was the change with 1.59
of both dup() and dup2()...
- (*new_file)->flags = old_file->flags & ~APR_INHERIT;
+ if ((*new_file)->filedes <= 2) {
+ (*new_file)->flags = old_file->flags | APR_INHERIT;
+ }
+ else {
+ (*new_file)->flags = old_file->flags & ~APR_INHERIT;
+ }
With this change we presume that fd 0..2 are specials that are
always inherited. I warned in the commit log;
This brings Unix into line with Win32's implementation of dup. If folks
feel we should *only* apply this code to which_dup==2 cases, that's fine
with me; although close(1); dup(n) would generally create a new fd 1 on
unix, that code isn't portable to Win32 and should be strongly discouraged.
Perhaps that dup2() discrepancy should be respected.
Finally, perhaps we revert to following the status of the dup2() flavored
cleanup. However, this is somewhat troublesome in the following case;
apr_file_close(x);
apr_file_dup2(x, y);
where x has no cleanup whatsoever, because the apr_file_close(x) has
already caused that cleanup to be killed.
The other option is to query the APR_FILE_NOCLOSE and APR_INHERIT
flags of x to determine what cleanup (if any) aught to be registered for the
dup2(x,...).
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)
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
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 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 Jeff Trawick <tr...@attglobal.net>.
Joe Orton wrote:
> On Fri, Mar 21, 2003 at 11:15:09AM -0500, Jeff Trawick wrote:
>
>>Aaron Bannert wrote:
>>
>>>The PODs in the worker MPM are getting closed and the parent is then
>>>unable to kill its children when it needs to (don't you love how
>>>morbid that sounds?). I see one of these every second in the error log:
>>>
>>>[Thu Mar 20 18:09:25 2003] [warn] (32)Broken pipe: write pipe_of_death
>>
>>lots of theories :)
>>
>>what I see with HEAD is that the pipe gets file descriptor 2 (stderr),
>>and we later make sure that the error log is file descriptor 2,
>>resulting in one side of the pipe getting closed
>
>
> Sounds like this is a different manifestation of the problem I can see.
> Does it work if you back down to r1.56 of filedup.c?
yes, backing off the patch I posted reintroduces the problem and then
reverting filedup.c back to r1.56 "fixes" the problem
>>I would guess that some other changes with closing descriptors let us
>>get fd 2 for one of the pipe handles, and that this conceivably could
>>have happened with all sorts of code changes.
>
>
> 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??)
d) we create the pod, with file descriptor 2 as one side of the pipe
e) we again set the error log to file descriptor 2, losing that side of
the pipe
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Mar 21, 2003 at 11:15:09AM -0500, Jeff Trawick wrote:
> Aaron Bannert wrote:
> >
> >The PODs in the worker MPM are getting closed and the parent is then
> >unable to kill its children when it needs to (don't you love how
> >morbid that sounds?). I see one of these every second in the error log:
> >
> >[Thu Mar 20 18:09:25 2003] [warn] (32)Broken pipe: write pipe_of_death
>
> lots of theories :)
>
> what I see with HEAD is that the pipe gets file descriptor 2 (stderr),
> and we later make sure that the error log is file descriptor 2,
> resulting in one side of the pipe getting closed
Sounds like this is a different manifestation of the problem I can see.
Does it work if you back down to r1.56 of filedup.c?
> I would guess that some other changes with closing descriptors let us
> get fd 2 for one of the pipe handles, and that this conceivably could
> have happened with all sorts of code changes.
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.
Regards,
joe
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by Jeff Trawick <tr...@attglobal.net>.
Aaron Bannert wrote:
>
> On Thursday, March 20, 2003, at 01:50 PM, wrowe@apache.org wrote:
>
>> wrowe 2003/03/20 13:50:41
>>
>> Modified: . CHANGES
>> modules/loggers mod_log_config.c
>> modules/mappers mod_rewrite.c
>> server log.c mpm_common.c
>> server/mpm/worker pod.c
>> Log:
>> SECURITY: Eliminated leaks of several file descriptors to child
>> processes, such as CGI scripts.
>
>
> [...]
>
>> apr_sockaddr_info_get(&(*pod)->sa,
>> ap_listeners->bind_addr->hostname,
>> APR_UNSPEC,
>> ap_listeners->bind_addr->port, 0, p);
>>
>> + /* close these before exec. */
>> + apr_file_unset_inherit((*pod)->pod_in);
>> + apr_file_unset_inherit((*pod)->pod_out);
>> +
>> return APR_SUCCESS;
>
>
> The PODs in the worker MPM are getting closed and the parent is then
> unable to kill its children when it needs to (don't you love how
> morbid that sounds?). I see one of these every second in the error log:
>
> [Thu Mar 20 18:09:25 2003] [warn] (32)Broken pipe: write pipe_of_death
lots of theories :)
what I see with HEAD is that the pipe gets file descriptor 2 (stderr),
and we later make sure that the error log is file descriptor 2,
resulting in one side of the pipe getting closed
I would guess that some other changes with closing descriptors let us
get fd 2 for one of the pipe handles, and that this conceivably could
have happened with all sorts of code changes.
Attached is a patch which certainly is not a fix, but which makes it
work for me by clumsily ensuring that neither pipe descriptor is
stdin/out/err; no more problems writing to the pipe at this point.
Re: cvs commit: httpd-2.0/server/mpm/worker pod.c
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Mar 20, 2003 at 07:20:22PM -0800, Aaron Bannert wrote:
> > + /* close these before exec. */
> > + apr_file_unset_inherit((*pod)->pod_in);
> > + apr_file_unset_inherit((*pod)->pod_out);
> > +
> > return APR_SUCCESS;
>
> The PODs in the worker MPM are getting closed and the parent is then
> unable to kill its children when it needs to (don't you love how
> morbid that sounds?). I see one of these every second in the error log:
>
> [Thu Mar 20 18:09:25 2003] [warn] (32)Broken pipe: write pipe_of_death
I haven't seen that, though HEAD is behaving strangely, I think it may
be some of Bill's filedup.c changes in APR. If you back down to rev
1.56 of filedup.c does this go away? I'm seeing a close(2) happening
too early with 1.57 and later, which results in this nice trick:
open("/path/to/logs/access_log", O_WRONLY|O_APPEND|O_CREAT, 0666) = 2
open("/path/to/logs/error_log", O_RDWR|O_APPEND|O_CREAT, 0666) = 4
dup2(4, 2) = 2
> Since the unset_inherit() is being called from the open_logs hook, it's
> happening in the parent process, which means that the fork for
> the children is going to kill them off. We need to unset the inherit
> *after* we are running in the child.
My discovery at the beginning of this expedition was that child_cleanups
are only run before the exec() in apr_proc_create(), so I'm not sure why
a fork() would close anything?
Regards,
joe