You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ryan Bloom <rb...@covalent.net> on 2002/03/06 19:35:07 UTC
RE: filtering problems & fix
> A simple SSI file with two #include file directives
> will coredump due the fact that we are copying the
> filter chain *pointers* from the main request to
> the subrequest in make_sub_request. When we add
> the subreq_core_filter this corrupts the main
> filter chain.
There should be no reason to copy the entire chain to the sub-request.
In fact, if we do have to do so, then there is a bigger problem. I
think the actual problem is that we no longer really need the previous
pointer that I added. That pointer is causing more problems that it
solves, and we have to walk the filter chain anyway in
add_any_filter_handle.
The other thing is that we actually want to use the exact same filters.
That is one of the goals of subrequests, so I believe that this is the
wrong solution. I will remove the previous filter ASAP and see if it
still works.
> We need to copy the filter chain, not just the
> pointers. Attached is a patch to do this. I will
> commit if there are no objections.
>
> There is also at least one remaining bug in
> add_any_filter handle due to the fact that
> r->output_filters and/or r->proto_output_filters
> are not getting updated when filters are added.
> I'm still looking into that one if no-one beats
> me to it.
Can you provide a config that causes this bug?
Ryan
RE: filtering problems & fix
Posted by Ryan Bloom <rb...@covalent.net>.
> > The other thing is that we actually want to use the exact same
filters.
>
> My patch replaces the ap_filter_t structures from the original
> chain with exact copies in the subreq chain so 1) we have the
> exact same filter chain and 2) and adds/removes will only affect
> the subrequest chain.
I don't want the adds/removes to only affect the sub-request, unless it
is an add/remove of a resource filter. Any add/remove of a connection
or protocol filter in a sub-request is a BIG problem. Those filters
should be setup based on the headers from the client, not based on which
resource we are returning.
> > That is one of the goals of subrequests, so I believe that this is
the
> > wrong solution. I will remove the previous filter ASAP and see if
it
> > still works.
>
> I guess I don't see how this fixes the problem that
> the subrequest can be adding/subtracting filters
> and if we are just copying the pointers from the
> main request we had better be sure that before we
> return from the subrequest all adds/subtracts are
> undone, and I don't see how we can guarantee that.
It is guaranteed, because the sub-request shouldn't be adding any
protocol filters. If it does, then there is a bigger more important
bug. IF the sub-request is only adding resource filters, then they are
on a separate filter chain. The problem with doing the copy, is that if
the sub-request is adding a protocol filter, then you are hiding that
problem instead of pointing it out in big red seg fault letters.
> > Can you provide a config that causes this bug?
>
> A simple SSI file with two #include file directives
Okay, I thought that was just causing the first bug, not both. I'll
look into it ASAP.
Ryan
RE: filtering problems & fix
Posted by Allan Edwards <ak...@meepzor.com>.
> > My only other observation is that any filters added in the subreq
> > must only be added to the top of the rnew->output_filters
> > chain or must be cleanly removed before the subreq returns
> > otherwise the main chain will get corrupted. I haven't seen
> > this happen so far, so maybe this is just a hypothetical
> > concern.
>
> It isn't hypothetical, but it also isn't a bug we want to protect
> against. The thing is that the filters added in the subrequest by
> definition must be resource filters, which means that they must be added
> above protocol and connection filters. If you are adding either a
> protocol or connection filter in a sub request, then you are doing
> something incredibly wrong, and we want the server to fail.
Absolutely. The case I was actually thinking about but didn't clearly
explain was something like say mod_deflate, which if it were
"AP_FTYPE_CONTENT_SET" would be above the proto chain passed by
next_filter. Even so I don't know of any subreq filter that would
get placed after this.
Allan
RE: filtering problems & fix
Posted by Ryan Bloom <rb...@covalent.net>.
> > Did that patch fix the bug for everybody? If so, I want to commit
it.
> > I have a three hour meeting now, so I'm not going to have time to
> > though. Can somebody else commit it this afternoon if it works?
>
> Thanks Ryan, with your patch we longer trap in the SSI testcase but
> I have a question. In make_sub_request where the filter pointers
> are copied, why this:
>
> rnew->proto_output_filters = r->connection->output_filters;
That would be a bug, and a pretty big one at that. How in the world did
that slip through all this time. :-(
> rnew->output_filters is pointing to the remainder of the
> main chain (next_filter) and rnew->proto_output_filters is
> not pointing to the proto part of it. Is this just a
> placeholder in case some subrequest code tries to add
> a proto filter?
Ah, this is how it slipped through. Because r->output_filters is setup
correctly, and r->proto_output_filters should never be modified by a
sub-request, this will work. Of course, it is incorrect, and should be
fixed. I'll fix it along with the other patch. Thanks for catching
this.
> My only other observation is that any filters added in the subreq
> must only be added to the top of the rnew->output_filters
> chain or must be cleanly removed before the subreq returns
> otherwise the main chain will get corrupted. I haven't seen
> this happen so far, so maybe this is just a hypothetical
> concern.
It isn't hypothetical, but it also isn't a bug we want to protect
against. The thing is that the filters added in the subrequest by
definition must be resource filters, which means that they must be added
above protocol and connection filters. If you are adding either a
protocol or connection filter in a sub request, then you are doing
something incredibly wrong, and we want the server to fail.
Ryan
RE: filtering problems & fix
Posted by Allan Edwards <ak...@meepzor.com>.
> Did that patch fix the bug for everybody? If so, I want to commit it.
> I have a three hour meeting now, so I'm not going to have time to
> though. Can somebody else commit it this afternoon if it works?
Thanks Ryan, with your patch we longer trap in the SSI testcase but
I have a question. In make_sub_request where the filter pointers
are copied, why this:
rnew->proto_output_filters = r->connection->output_filters;
rnew->output_filters is pointing to the remainder of the
main chain (next_filter) and rnew->proto_output_filters is
not pointing to the proto part of it. Is this just a
placeholder in case some subrequest code tries to add
a proto filter?
My only other observation is that any filters added in the subreq
must only be added to the top of the rnew->output_filters
chain or must be cleanly removed before the subreq returns
otherwise the main chain will get corrupted. I haven't seen
this happen so far, so maybe this is just a hypothetical
concern.
Allan
RE: filtering problems & fix
Posted by Ryan Bloom <rb...@covalent.net>.
Did that patch fix the bug for everybody? If so, I want to commit it.
I have a three hour meeting now, so I'm not going to have time to
though. Can somebody else commit it this afternoon if it works?
Ryan
----------------------------------------------
Ryan Bloom rbb@covalent.net
645 Howard St. rbb@apache.org
San Francisco, CA
> -----Original Message-----
> From: Justin Erenkrantz [mailto:jerenkrantz@ebuilt.com]
> Sent: Wednesday, March 06, 2002 12:00 PM
> To: dev@httpd.apache.org; rbb@covalent.net
> Subject: Re: filtering problems & fix
>
> On Wed, Mar 06, 2002 at 11:30:49AM -0800, Ryan Bloom wrote:
> > Index: server/core.c
> > ===================================================================
> > RCS file: /home/cvs/httpd-2.0/server/core.c,v
> > retrieving revision 1.159
> > diff -u -r1.159 core.c
> > --- server/core.c 6 Mar 2002 18:03:19 -0000 1.159
> > +++ server/core.c 6 Mar 2002 19:52:52 -0000
> > @@ -4040,7 +4040,7 @@
> > AP_FTYPE_NETWORK);
> > ap_subreq_core_filter_handle =
> > ap_register_output_filter("SUBREQ_CORE",
> > ap_sub_req_output_filter,
> > - AP_FTYPE_HTTP_HEADER);
> > + AP_FTYPE_CONTENT + 5);
> > ap_old_write_func = ap_register_output_filter("OLD_WRITE",
> > ap_old_write_filter,
> > AP_FTYPE_CONTENT - 10);
>
> Should we add a new filter type (say AP_FTYPE_CONTENT_SET) where
> SUBREQ_CORE, mod_deflate, mod_headers, etc should go? It would
> be in between AP_FTYPE_CONTENT and AP_FTYPE_HTTP_HEADER (which
> should be renamed to AP_FTYPE_PROTOCOL). The idea is that the
> content-type should be finalized at this filter level. The name
> sucks though. -- justin
Re: filtering problems & fix
Posted by Ian Holsman <ia...@apache.org>.
Ryan Bloom wrote:
>>On Wed, Mar 06, 2002 at 11:30:49AM -0800, Ryan Bloom wrote:
>>
>>>Index: server/core.c
>>>===================================================================
>>>RCS file: /home/cvs/httpd-2.0/server/core.c,v
>>>retrieving revision 1.159
>>>diff -u -r1.159 core.c
>>>--- server/core.c 6 Mar 2002 18:03:19 -0000 1.159
>>>+++ server/core.c 6 Mar 2002 19:52:52 -0000
>>>@@ -4040,7 +4040,7 @@
>>> AP_FTYPE_NETWORK);
>>> ap_subreq_core_filter_handle =
>>> ap_register_output_filter("SUBREQ_CORE",
>>>ap_sub_req_output_filter,
>>>- AP_FTYPE_HTTP_HEADER);
>>>+ AP_FTYPE_CONTENT + 5);
thats cool.. as long as it's not FTYPE_CONTENT were
sweet.. (I changed it to header to stop mod_cache breaking)
>>> ap_old_write_func = ap_register_output_filter("OLD_WRITE",
>>> ap_old_write_filter,
>>>AP_FTYPE_CONTENT - 10);
>>>
>>Should we add a new filter type (say AP_FTYPE_CONTENT_SET) where
>>SUBREQ_CORE, mod_deflate, mod_headers, etc should go? It would
>>be in between AP_FTYPE_CONTENT and AP_FTYPE_HTTP_HEADER (which
>>should be renamed to AP_FTYPE_PROTOCOL). The idea is that the
>>content-type should be finalized at this filter level. The name
>>sucks though. -- justin
>>
>
> As I have said on IRC, and in my long message this morning, yes there
> should be. I dislike the name that you have chosen (just as you do),
> but I don't have any better ideas either. :-(
>
> Ryan
>
>
>
RE: filtering problems & fix
Posted by Ryan Bloom <rb...@covalent.net>.
> On Wed, Mar 06, 2002 at 11:30:49AM -0800, Ryan Bloom wrote:
> > Index: server/core.c
> > ===================================================================
> > RCS file: /home/cvs/httpd-2.0/server/core.c,v
> > retrieving revision 1.159
> > diff -u -r1.159 core.c
> > --- server/core.c 6 Mar 2002 18:03:19 -0000 1.159
> > +++ server/core.c 6 Mar 2002 19:52:52 -0000
> > @@ -4040,7 +4040,7 @@
> > AP_FTYPE_NETWORK);
> > ap_subreq_core_filter_handle =
> > ap_register_output_filter("SUBREQ_CORE",
> > ap_sub_req_output_filter,
> > - AP_FTYPE_HTTP_HEADER);
> > + AP_FTYPE_CONTENT + 5);
> > ap_old_write_func = ap_register_output_filter("OLD_WRITE",
> > ap_old_write_filter,
> > AP_FTYPE_CONTENT - 10);
>
> Should we add a new filter type (say AP_FTYPE_CONTENT_SET) where
> SUBREQ_CORE, mod_deflate, mod_headers, etc should go? It would
> be in between AP_FTYPE_CONTENT and AP_FTYPE_HTTP_HEADER (which
> should be renamed to AP_FTYPE_PROTOCOL). The idea is that the
> content-type should be finalized at this filter level. The name
> sucks though. -- justin
As I have said on IRC, and in my long message this morning, yes there
should be. I dislike the name that you have chosen (just as you do),
but I don't have any better ideas either. :-(
Ryan
Re: filtering problems & fix
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Wed, Mar 06, 2002 at 11:30:49AM -0800, Ryan Bloom wrote:
> Index: server/core.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/server/core.c,v
> retrieving revision 1.159
> diff -u -r1.159 core.c
> --- server/core.c 6 Mar 2002 18:03:19 -0000 1.159
> +++ server/core.c 6 Mar 2002 19:52:52 -0000
> @@ -4040,7 +4040,7 @@
> AP_FTYPE_NETWORK);
> ap_subreq_core_filter_handle =
> ap_register_output_filter("SUBREQ_CORE",
> ap_sub_req_output_filter,
> - AP_FTYPE_HTTP_HEADER);
> + AP_FTYPE_CONTENT + 5);
> ap_old_write_func = ap_register_output_filter("OLD_WRITE",
> ap_old_write_filter,
> AP_FTYPE_CONTENT - 10);
Should we add a new filter type (say AP_FTYPE_CONTENT_SET) where
SUBREQ_CORE, mod_deflate, mod_headers, etc should go? It would
be in between AP_FTYPE_CONTENT and AP_FTYPE_HTTP_HEADER (which
should be renamed to AP_FTYPE_PROTOCOL). The idea is that the
content-type should be finalized at this filter level. The name
sucks though. -- justin
RE: filtering problems & fix
Posted by Ryan Bloom <rb...@covalent.net>.
I don't know why CVS was being a pain, but here is the patch finally.
:-)
Index: include/util_filter.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/util_filter.h,v
retrieving revision 1.67
diff -u -r1.67 util_filter.h
--- include/util_filter.h 3 Mar 2002 06:04:08 -0000 1.67
+++ include/util_filter.h 6 Mar 2002 19:52:52 -0000
@@ -263,9 +263,6 @@
/** The next filter in the chain */
ap_filter_t *next;
- /** The previous filter in the chain */
- ap_filter_t *prev;
-
/** The request_rec associated with the current filter. If a
sub-request
* adds filters, then the sub-request is the request associated
with the
* filter.
Index: server/core.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/core.c,v
retrieving revision 1.159
diff -u -r1.159 core.c
--- server/core.c 6 Mar 2002 18:03:19 -0000 1.159
+++ server/core.c 6 Mar 2002 19:52:52 -0000
@@ -4040,7 +4040,7 @@
AP_FTYPE_NETWORK);
ap_subreq_core_filter_handle =
ap_register_output_filter("SUBREQ_CORE",
ap_sub_req_output_filter,
- AP_FTYPE_HTTP_HEADER);
+ AP_FTYPE_CONTENT + 5);
ap_old_write_func = ap_register_output_filter("OLD_WRITE",
ap_old_write_filter,
AP_FTYPE_CONTENT - 10);
}
Index: server/protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/protocol.c,v
retrieving revision 1.87
diff -u -r1.87 protocol.c
--- server/protocol.c 3 Mar 2002 22:04:03 -0000 1.87
+++ server/protocol.c 6 Mar 2002 19:52:52 -0000
@@ -777,9 +777,6 @@
r->request_config = ap_create_request_config(r->pool);
/* Must be set before we run create request hook */
- conn->output_filters->prev = NULL;
- conn->input_filters->prev = NULL;
-
r->proto_output_filters = conn->output_filters;
r->output_filters = r->proto_output_filters;
r->proto_input_filters = conn->input_filters;
Index: server/util_filter.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/util_filter.c,v
retrieving revision 1.85
diff -u -r1.85 util_filter.c
--- server/util_filter.c 6 Mar 2002 17:29:39 -0000 1.85
+++ server/util_filter.c 6 Mar 2002 19:52:52 -0000
@@ -329,7 +329,7 @@
f->ctx = ctx;
f->r = r;
f->c = c;
- f->next = f->prev = NULL;
+ f->next = NULL;
if (INSERT_BEFORE(f, *outf)) {
f->next = *outf;
@@ -351,10 +351,6 @@
}
if (first && first != (*outf)) {
first->next = f;
- f->prev = first;
- }
- if (*outf && ((*outf)->prev == first)) {
- (*outf)->prev = f;
}
}
*outf = f;
@@ -365,11 +361,7 @@
fscan = fscan->next;
f->next = fscan->next;
- if (fscan->next->prev == fscan) {
- f->prev = fscan;
- fscan->next->prev = f;
- fscan->next = f;
- }
+ fscan->next = f;
}
if (frec->ftype < AP_FTYPE_CONNECTION && (*r_filters ==
*c_filters)) {
@@ -472,7 +464,6 @@
if (*curr == f) {
*curr = (*curr)->next;
- (*curr)->prev = NULL;
return;
}
@@ -483,7 +474,6 @@
}
fscan->next = f->next;
- f->next->prev = fscan;
}
AP_DECLARE(void) ap_remove_input_filter(ap_filter_t *f)
RE: filtering problems & fix
Posted by Allan Edwards <ak...@meepzor.com>.
> I found the source of the bug. The SUB_REQ filter is an HTTP_HEADER
> filter, but it shouldn't be. That is a resource filter, because it is
> only added when the resource changes.
Yes, that's what I just saw and is the source of
at least one of the bugs I've been hitting. I
eagerly await your patch!
Allan
RE: filtering problems & fix
Posted by Ryan Bloom <rb...@covalent.net>.
I found the source of the bug. The SUB_REQ filter is an HTTP_HEADER
filter, but it shouldn't be. That is a resource filter, because it is
only added when the resource changes.
I have a patch that will fix the multiple includes case, but cvs is
giving me a hard time. I'll post it as soon as I can generate the patch
file.
Ryan
RE: filtering problems & fix
Posted by Allan Edwards <ak...@meepzor.com>.
> The other thing is that we actually want to use the exact same filters.
My patch replaces the ap_filter_t structures from the original
chain with exact copies in the subreq chain so 1) we have the
exact same filter chain and 2) and adds/removes will only affect
the subrequest chain.
> That is one of the goals of subrequests, so I believe that this is the
> wrong solution. I will remove the previous filter ASAP and see if it
> still works.
I guess I don't see how this fixes the problem that
the subrequest can be adding/subtracting filters
and if we are just copying the pointers from the
main request we had better be sure that before we
return from the subrequest all adds/subtracts are
undone, and I don't see how we can guarantee that.
> Can you provide a config that causes this bug?
A simple SSI file with two #include file directives
Cheers, Allan