You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2003/03/13 11:46:57 UTC
[PATCH] resend: fix fd leaks
[Resend. There are currently two outstanding fixes for public security
issues in the 2.0 stable branch: this and escaping of untrusted request
data in mod_log_config which Andre forward-ported from 1.3]
Hi, here is a version of the patch in #17206 which removes the current
the fd leaks. Most of these were introduced in this commit
http://marc.theaimsgroup.com/?l=apache-cvs&m=99531770520998&w=2
though the pod leak has been around longer. I haven't checked whether
the mod_file_cache change in that commit should be reverted as well. The
patch is against 2.0 HEAD.
Submitted by: Christian Kratzer, Bjoern A. Zeeb
Index: modules/loggers/mod_log_config.c
===================================================================
RCS file: /store/cvs/root/httpd-2.0/modules/loggers/mod_log_config.c,v
retrieving revision 1.99
diff -u -r1.99 mod_log_config.c
--- modules/loggers/mod_log_config.c 14 Feb 2003 04:17:34 -0000 1.99
+++ modules/loggers/mod_log_config.c 6 Mar 2003 15:38:07 -0000
@@ -1291,7 +1291,6 @@
"could not open transfer log file %s.", fname);
return NULL;
}
- apr_file_inherit_set(fd);
return fd;
}
}
Index: modules/mappers/mod_rewrite.c
===================================================================
RCS file: /store/cvs/root/httpd-2.0/modules/mappers/mod_rewrite.c,v
retrieving revision 1.148
diff -u -r1.148 mod_rewrite.c
--- modules/mappers/mod_rewrite.c 1 Mar 2003 18:35:50 -0000 1.148
+++ modules/mappers/mod_rewrite.c 6 Mar 2003 15:38:08 -0000
@@ -3429,7 +3429,6 @@
"file %s", fname);
exit(1);
}
- apr_file_inherit_set(conf->rewritelogfp);
}
return;
}
Index: server/log.c
===================================================================
RCS file: /store/cvs/root/httpd-2.0/server/log.c,v
retrieving revision 1.130
diff -u -r1.130 log.c
--- server/log.c 10 Feb 2003 16:27:28 -0000 1.130
+++ server/log.c 6 Mar 2003 15:38:08 -0000
@@ -320,8 +320,6 @@
ap_server_argv0, fname);
return DONE;
}
-
- apr_file_inherit_set(s->error_log);
}
return OK;
Index: server/mpm_common.c
===================================================================
RCS file: /store/cvs/root/httpd-2.0/server/mpm_common.c,v
retrieving revision 1.103
diff -u -r1.103 mpm_common.c
--- server/mpm_common.c 3 Feb 2003 17:53:19 -0000 1.103
+++ server/mpm_common.c 6 Mar 2003 15:38:08 -0000
@@ -410,6 +410,10 @@
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;
}
Index: server/mpm/worker/pod.c
===================================================================
RCS file: /store/cvs/root/httpd-2.0/server/mpm/worker/pod.c,v
retrieving revision 1.7
diff -u -r1.7 pod.c
--- server/mpm/worker/pod.c 3 Feb 2003 17:53:26 -0000 1.7
+++ server/mpm/worker/pod.c 6 Mar 2003 15:38:08 -0000
@@ -76,6 +76,10 @@
*/
(*pod)->p = p;
+ /* close these before exec. */
+ apr_file_unset_inherit((*pod)->pod_in);
+ apr_file_unset_inherit((*pod)->pod_out);
+
return APR_SUCCESS;
}
Re: [PATCH] resend: fix fd leaks
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 09:03 AM 3/20/2003, Joe Orton wrote:
>> As I mentioned before, I'm +1 on this patch. I'll put back the appropriate
>> comments in httpd-2.1-dev since this will be nearly impossible to fix
>> for Win32 without breaking many things that we wouldn't consider good
>> for the health of 2.0-releases.
>
>Thanks, I still don't really grasp the Win32 side of things, so I'd
>rather that you added the appropriate comments.
>
>Let me know if there's anything more I can do.
Feel free to commit the patch to 2.0.45-dev and 2.1.0-dev, and I'll commit
those comments to 2.1.0-dev a little later.
Bill
Re: [PATCH] resend: fix fd leaks
Posted by "Bjoern A. Zeeb" <bz...@lists.zabbadoz.net>.
On Thu, 20 Mar 2003, Joe Orton wrote:
Hi,
> > >> >Submitted by: Christian Kratzer, Bjoern A. Zeeb
> > >>
> > >> +1 here. I have one comment; please *don't* simply delete those lines
> > >> from server/log.c, modules/mappers/mod_rewrite.c and, of course,
> > >> modules/loggers/mod_log_config.c. Please comment them out with
> > >> /* XXX: this would be required in the Win32 parent */
I had just tested wrowes latest apr commits with httpd-2.0 cvs co and
Joe's latest patch and the open fds are gone and apache seems to work
fine.
I think we should get this in now with the requested comments.
--
Bjoern A. Zeeb bzeeb at Zabbadoz dot NeT
56 69 73 69 74 http://www.zabbadoz.net/
Re: [PATCH] resend: fix fd leaks
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Mar 19, 2003 at 01:15:21PM -0600, William Rowe wrote:
> At 03:25 AM 3/18/2003, Joe Orton wrote:
> >> >Hi, here is a version of the patch in #17206 which removes the current
> >> >the fd leaks. Most of these were introduced in this commit
> >> >
> >> >http://marc.theaimsgroup.com/?l=apache-cvs&m=99531770520998&w=2
> >> >
> >> >though the pod leak has been around longer. I haven't checked whether
> >> >the mod_file_cache change in that commit should be reverted as well. The
> >> >patch is against 2.0 HEAD.
> >> >
> >> >Submitted by: Christian Kratzer, Bjoern A. Zeeb
> >>
> >> +1 here. I have one comment; please *don't* simply delete those lines
> >> from server/log.c, modules/mappers/mod_rewrite.c and, of course,
> >> modules/loggers/mod_log_config.c. Please comment them out with
> >> /* XXX: this would be required in the Win32 parent */
> >
> >I don't really understand this... the file_inherit interface concerns
> >whether files are inherited by exec()d processes, not any "child
> >processes", on Unix. Is there some mismatch in the Win32
> >implementation? Why should httpd need this APR call specifically for
> >Win32?
>
> As I mentioned before, I'm +1 on this patch. I'll put back the appropriate
> comments in httpd-2.1-dev since this will be nearly impossible to fix
> for Win32 without breaking many things that we wouldn't consider good
> for the health of 2.0-releases.
Thanks, I still don't really grasp the Win32 side of things, so I'd
rather that you added the appropriate comments.
Let me know if there's anything more I can do.
joe
Re: [PATCH] resend: fix fd leaks
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 03:25 AM 3/18/2003, Joe Orton wrote:
>> >Hi, here is a version of the patch in #17206 which removes the current
>> >the fd leaks. Most of these were introduced in this commit
>> >
>> >http://marc.theaimsgroup.com/?l=apache-cvs&m=99531770520998&w=2
>> >
>> >though the pod leak has been around longer. I haven't checked whether
>> >the mod_file_cache change in that commit should be reverted as well. The
>> >patch is against 2.0 HEAD.
>> >
>> >Submitted by: Christian Kratzer, Bjoern A. Zeeb
>>
>> +1 here. I have one comment; please *don't* simply delete those lines
>> from server/log.c, modules/mappers/mod_rewrite.c and, of course,
>> modules/loggers/mod_log_config.c. Please comment them out with
>> /* XXX: this would be required in the Win32 parent */
>
>I don't really understand this... the file_inherit interface concerns
>whether files are inherited by exec()d processes, not any "child
>processes", on Unix. Is there some mismatch in the Win32
>implementation? Why should httpd need this APR call specifically for
>Win32?
As I mentioned before, I'm +1 on this patch. I'll put back the appropriate
comments in httpd-2.1-dev since this will be nearly impossible to fix
for Win32 without breaking many things that we wouldn't consider good
for the health of 2.0-releases.
Our mutual confusion (which I explained over on apr) is that httpd fork()s
it's worker processes. In that sense, they are only sort-of child processes.
They aren't subject to FD_CLOEXEC and don't respect the "child" pool
cleanups.
On Win32, we exec() (actually, we CreateProcess()) our worker processes.
That means our workers are truly "child" processes.
So to do things 'right', we need to inherit common handles from the parent
process, tell the child worker process about them, and the child worker
process must then make them uninheritable once again on Windows.
We need something slick and abstract enough for Unix and Win32 so that
the right inheritence is used by platform, the handles survive from the parent
to the worker, and httpd mpms take care of all the dirty details.
I'm working on designs now and should have something to propose to 2.1-dev
over the next month. It won't be proposed for back porting to 2.0, but it should
make it into 2.1-dev quickly enough to give 2.2 module maintainers that are
affected enough time to react to the API, provide feedback and fix their modules.
Bill
Re: [PATCH] resend: fix fd leaks
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 03:25 AM 3/18/2003, Joe Orton wrote:
>On Mon, Mar 17, 2003 at 11:56:11PM -0600, William Rowe wrote:
>> We don't have the mechanics in place so right now this is a beneficial
>> noop on Win32. However, we should be passing those handles on to
>> all child processes. That won't happen today, but will occur in the very
>> near future. The XXX comments will remind me what I need to wiggle
>> to make this work.
>
>I don't really understand this... the file_inherit interface concerns
>whether files are inherited by exec()d processes, not any "child
>processes", on Unix. Is there some mismatch in the Win32
>implementation? Why should httpd need this APR call specifically for
>Win32?
Unix fork()s children. Win32 must exec() (CreateProcess) them, and
we need a mechanism for storing those "must be shared" objects between
the parent and worker processes. We need a relatively trivial way to
distribute those fds/handles from parent to child on !APR_HAVE_FORK
platforms. I don't have the "one right answer" to this, but it must be
trivial for module authors to use.
One thought is a shared userdata table of apr_file_t's that can be stashed
in apr_process_t. If that table has entries - it could become the MPM's
responsibility to propogate that table from parent to child process. Or the
mechanism could 'act' more like Unix domain sockets, with an accessor
that moves these handles from parent to child.
Anyway, 10% of the effort will be on the module author when they want to
share items like log file handles, etc, 10% by existing APR code, and
the 80% of the heavy lifting will be done by the MPMs of those processes
that need it.
Bill
Re: [PATCH] resend: fix fd leaks
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Mar 17, 2003 at 11:56:11PM -0600, William Rowe wrote:
> At 04:46 AM 3/13/2003, Joe Orton wrote:
> >[Resend. There are currently two outstanding fixes for public security
> >issues in the 2.0 stable branch: this and escaping of untrusted request
> >data in mod_log_config which Andre forward-ported from 1.3]
> >
> >Hi, here is a version of the patch in #17206 which removes the current
> >the fd leaks. Most of these were introduced in this commit
> >
> >http://marc.theaimsgroup.com/?l=apache-cvs&m=99531770520998&w=2
> >
> >though the pod leak has been around longer. I haven't checked whether
> >the mod_file_cache change in that commit should be reverted as well. The
> >patch is against 2.0 HEAD.
> >
> >Submitted by: Christian Kratzer, Bjoern A. Zeeb
>
> +1 here. I have one comment; please *don't* simply delete those lines
> from server/log.c, modules/mappers/mod_rewrite.c and, of course,
> modules/loggers/mod_log_config.c. Please comment them out with
> /* XXX: this would be required in the Win32 parent */
>
> We don't have the mechanics in place so right now this is a beneficial
> noop on Win32. However, we should be passing those handles on to
> all child processes. That won't happen today, but will occur in the very
> near future. The XXX comments will remind me what I need to wiggle
> to make this work.
I don't really understand this... the file_inherit interface concerns
whether files are inherited by exec()d processes, not any "child
processes", on Unix. Is there some mismatch in the Win32
implementation? Why should httpd need this APR call specifically for
Win32?
Regards,
joe
Re: [PATCH] resend: fix fd leaks
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 04:46 AM 3/13/2003, Joe Orton wrote:
>[Resend. There are currently two outstanding fixes for public security
>issues in the 2.0 stable branch: this and escaping of untrusted request
>data in mod_log_config which Andre forward-ported from 1.3]
>
>Hi, here is a version of the patch in #17206 which removes the current
>the fd leaks. Most of these were introduced in this commit
>
>http://marc.theaimsgroup.com/?l=apache-cvs&m=99531770520998&w=2
>
>though the pod leak has been around longer. I haven't checked whether
>the mod_file_cache change in that commit should be reverted as well. The
>patch is against 2.0 HEAD.
>
>Submitted by: Christian Kratzer, Bjoern A. Zeeb
+1 here. I have one comment; please *don't* simply delete those lines
from server/log.c, modules/mappers/mod_rewrite.c and, of course,
modules/loggers/mod_log_config.c. Please comment them out with
/* XXX: this would be required in the Win32 parent */
We don't have the mechanics in place so right now this is a beneficial
noop on Win32. However, we should be passing those handles on to
all child processes. That won't happen today, but will occur in the very
near future. The XXX comments will remind me what I need to wiggle
to make this work.
Of course it won't be reenabled across the board. Folks have asked
for a very long time "how do I tell this is a win32 parent process v.s.
a child process?" That's a legit question for the MPM - and one
I intend to hack up once the crunch of work is off of me.
Oh - the answer on Unix is 'this is the parent and child' - on win32 it's
either one or the other, or we are using -D ONE_PROCESS. But we
can debate those mechanics later...
Bill