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.
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.
Allan
Index: include/util_filter.h
===
RCS file: /home/cvs/httpd-2.0/include/util_filter.h,v
retrieving revision 1.67
diff -u -d -b -r1.67 util_filter.h
--- include/util_filter.h 3 Mar 2002 06:04:08 - 1.67
+++ include/util_filter.h 6 Mar 2002 18:24:23 -
-515,6 +515,18
...)
__attribute__((format(printf,3,4)));
+/**
+ * Copy the in/out filter chains from one request to another
+ * param from The request to copy the chains from
+ * param to The request to copy the chains to
+ * param start_filter The filter from which point the output chain copy starts
+ * return void
+ * deffunc void ap_copy_filter_chains(const request_rec *from, request_rec *to,
+const ap_filter_t *start_filter)
+ */
+void ap_copy_filter_chains(const request_rec *from,
+ request_rec *to,
+ const ap_filter_t *start_filter);
+
#ifdef __cplusplus
}
#endif
Index: server/request.c
===
RCS file: /home/cvs/httpd-2.0/server/request.c,v
retrieving revision 1.102
diff -u -d -b -r1.102 request.c
--- server/request.c5 Mar 2002 05:24:21 - 1.102
+++ server/request.c6 Mar 2002 18:24:25 -
-1492,14 +1492,7
/* start with the same set of output filters */
if (next_filter) {
-/* while there are no input filters for a subrequest, we will
- * try to insert some, so if we don't have valid data, the code
- * will seg fault.
- */
-rnew-input_filters = r-input_filters;
-rnew-proto_input_filters = r-proto_input_filters;
-rnew-output_filters = next_filter;
- rnew-proto_output_filters = r-connection-output_filters;
+ap_copy_filter_chains(r, rnew, next_filter);
ap_add_output_filter_handle(ap_subreq_core_filter_handle,
NULL, rnew, rnew-connection);
}
Index: server/util_filter.c
===
RCS file: /home/cvs/httpd-2.0/server/util_filter.c,v
retrieving revision 1.85
diff -u -d -b -r1.85 util_filter.c
--- server/util_filter.c6 Mar 2002 17:29:39 - 1.85
+++ server/util_filter.c6 Mar 2002 18:24:25 -
-615,3 +615,96
va_end(args);
return rv;
}
+
+void ap_copy_filter_chains(const request_rec *from,
+ request_rec *to,
+ const ap_filter_t *start_filter)
+{
+int connection_done = 0, proto_done = 0;
+const ap_filter_t *from_filter;
+ap_filter_t *to_filter, *curr_filter;
+
+from_filter = from-connection-output_filters;
+while (from_filter-next != NULL)
+from_filter = from_filter-next;
+
+curr_filter = NULL;
+while(1) {
+
+to_filter = apr_palloc(to-pool, sizeof(ap_filter_t));
+memcpy(to_filter, from_filter, sizeof(ap_filter_t));
+to_filter-next = curr_filter;
+
+if (NULL != curr_filter)
+ curr_filter-prev = to_filter;
+
+curr_filter = to_filter;
+
+if (!connection_done) {
+if (from_filter == from-connection-output_filters) {
+to-connection-output_filters = curr_filter;
+connection_done = 1;
+}
+}
+else if (!proto_done) {
+if (from_filter == from-proto_output_filters) {
+to-proto_output_filters = curr_filter;
+proto_done = 1;
+}
+}
+
+if (from_filter == start_filter)
+break;
+
+from_filter = from_filter-prev;
+if (NULL == from_filter)
+break;
+}
+
+if (!proto_done)
+to-proto_output_filters = curr_filter;
+if (NULL == to-output_filters)
+to-output_filters = to-proto_output_filters;
+
+from_filter = from-connection-input_filters;
+while (from_filter-next != NULL)
+from_filter = from_filter-next;
+
+connection_done = proto_done = 0;
+curr_filter = NULL;
+
+while(1) {
+
+to_filter = apr_palloc(to-pool, sizeof(ap_filter_t));
+memcpy(to_filter, from_filter, sizeof(ap_filter_t));
+
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
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
RE: filtering problems fix
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
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
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
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 - 1.159 +++ server/core.c 6 Mar 2002 19:52:52 - -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
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 - 1.159 +++ server/core.c 6 Mar 2002 19:52:52 - -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
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.c6 Mar 2002 18:03:19 - 1.159 +++ server/core.c6 Mar 2002 19:52:52 - -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
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 [EMAIL PROTECTED] 645 Howard St. [EMAIL PROTECTED] San Francisco, CA -Original Message- From: Justin Erenkrantz [mailto:[EMAIL PROTECTED]] Sent: Wednesday, March 06, 2002 12:00 PM To: [EMAIL PROTECTED]; [EMAIL PROTECTED] 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 - 1.159 +++ server/core.c 6 Mar 2002 19:52:52 - @@ -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
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
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
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
