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 2010/06/08 14:37:37 UTC
[PATCH] PR 17629 and all that
https://issues.apache.org/bugzilla/show_bug.cgi?id=17629
Here's an attempt at fixing the dreaded PR 17629. This is a bug in the
handling of the output filter chain at the point where an internal
redirect is applied to a subrequest. Complications:
a) a subrequest's filter chain may start at an arbitrary point in
r->main's filter chain
-- for an SSI include, the output from the include must continue from
the next filter along from the INCLUDES filter
b) that point where the filter chains join cannot be lost by the
subrequest doing an internal redirect
-- obvious breakage as per the PR is losing the DEFLATE filter for
the output of the redirect from the subreq
c) the subrequest's filter chain cannot be preserved as-is, since it may
contain filters which should not be applied to the destination URI
The approach I've used is to distinguish inherited filters from
subreq-specific filters by seeing what f->r points to, and nuking all
but the inherited filters. Here's the patch - test case in the test
suite:
Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 952555)
+++ modules/http/http_request.c (working copy)
@@ -460,17 +460,47 @@
new->proto_output_filters = r->proto_output_filters;
new->proto_input_filters = r->proto_input_filters;
- new->output_filters = new->proto_output_filters;
new->input_filters = new->proto_input_filters;
if (new->main) {
- /* Add back the subrequest filter, which we lost when
- * we set output_filters to include only the protocol
- * output filters from the original request.
- */
- ap_add_output_filter_handle(ap_subreq_core_filter_handle,
- NULL, new, new->connection);
+ ap_filter_t *f, *nextf;
+
+ /* If this is a subrequest, the filter chain may contain a
+ * mixture of filters specific to the old request (r), and
+ * some inherited from r->main. Here, inherit that filter
+ * chain, and remove all those which are specific to the old
+ * request; ensuring the subreq filter is left in place. */
+ new->output_filters = r->output_filters;
+
+ f = new->output_filters;
+ do {
+ nextf = f->next;
+
+ if (f->r == r && f->frec != ap_subreq_core_filter_handle) {
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "dropping filter '%s' in internal redirect from %s to %s",
+ f->frec->name, r->unparsed_uri, new_uri);
+
+ /* To remove the filter, first set f->r to the *new*
+ * request_rec, so that ->output_filters on 'new' is
+ * changed (if necessary) when removing the filter. */
+ f->r = new;
+ ap_remove_output_filter(f);
+ }
+
+ f = nextf;
+
+ /* Stop at the protocol filters. If a protocol filter has
+ * been newly installed for this resource, better leave it
+ * in place, though it's probably a misconfiguration or
+ * filter bug to get into this state. */
+ } while (f && f != new->proto_output_filters);
}
+ else {
+ /* If this is not a subrequest, clear out all
+ * resource-specific filters. */
+ new->output_filters = new->proto_output_filters;
+ }
update_r_in_filters(new->input_filters, r, new);
update_r_in_filters(new->output_filters, r, new);
Re: [PATCH] PR 17629 and all that
Posted by "Dennis J." <de...@conversis.de>.
On 06/08/2010 02:37 PM, Joe Orton wrote:
> https://issues.apache.org/bugzilla/show_bug.cgi?id=17629
>
> Here's an attempt at fixing the dreaded PR 17629. This is a bug in the
> handling of the output filter chain at the point where an internal
> redirect is applied to a subrequest. Complications:
>
> a) a subrequest's filter chain may start at an arbitrary point in
> r->main's filter chain
>
> -- for an SSI include, the output from the include must continue from
> the next filter along from the INCLUDES filter
>
Could this in some way be related to the bug I filed a while ago where the
SSI output filter mangles the QUERY_STRING variable by replacing it with
the query string used in the last include directive?
https://issues.apache.org/bugzilla/show_bug.cgi?id=49043
This behavior changed between 1.3 and 2.x and my uninformed guess is that
this is probably a scoping issue that happened when SSI was turned into a
filter (i.e. before the QUERY_STRING was only modified for each subrequest
but in a filter context the changed QUERY_STRING string no basically is
used for "all following data").
Regards,
Dennis
Re: [PATCH] PR 17629 and all that
Posted by Joe Orton <jo...@redhat.com>.
So since nobody has told me I'm an idiot yet, I've committed this to the
trunk for wider testing:
http://svn.apache.org/viewvc?view=revision&revision=952828
Regards, Joe
Re: [PATCH] PR 17629 and all that
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jun 08, 2010 at 06:17:29PM +0200, "Plüm, Rüdiger, VF-Group" wrote:
> > I'd spent some time working on exactly that approach, trying to
> > answer that question. But surprisingly the answer is "yes" - the
> > subreq filter has ftype=AP_FTYPE_CONTENT_SET, so any filter
> > registered with an ftype greater than that can be inserted and will
> > work (so long as it doesn't depend on seeing EOS).
>
> Ah. So deleting everything between r->output_filters and ap_subreq_core_filter
> will not delete too much (they all belong to the subrequest), but we may miss
> subrequest specific filters between ap_subreq_core_filter and the protocol
> output filters if not following your patch, correct?
Ah, I didn't mean to imply that sorry. The subreq-specific (to be
removed) filters and inherited (to be kept) filters can both appear
either before or after the subreq filter in the subreq's filter chain.
So yes, we do need to be sure to cull correctly after the subreq filter,
but culling everything *before* it may in fact delete too much.
I don't have a test case added with an inherited filter before the
subreq_core but I have been manually testing this one.
Thanks a lot for looking at this!
Regards, Joe
RE: [PATCH] PR 17629 and all that
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Joe Orton
> Sent: Dienstag, 8. Juni 2010 17:36
> To: dev@httpd.apache.org
> Subject: Re: [PATCH] PR 17629 and all that
>
> On Tue, Jun 08, 2010 at 05:20:16PM +0200, "Plüm, Rüdiger,
> VF-Group" wrote:
> > Is it possible to have any non subrequest specific filters
> below the ap_subreq_core_filter_handle?
> > It is no that the current patch does not handle this but
> wouldn't it be possible
> > to just throw away any filter between
> >
> > r->output_filters and ap_subreq_core_filter_handle without
> any further test?
>
> I'd spent some time working on exactly that approach, trying
> to answer
> that question. But surprisingly the answer is "yes" - the
> subreq filter
> has ftype=AP_FTYPE_CONTENT_SET, so any filter registered with
> an ftype
> greater than that can be inserted and will work (so long as
> it doesn't
> depend on seeing EOS).
Ah. So deleting everything between r->output_filters and ap_subreq_core_filter
will not delete too much (they all belong to the subrequest), but we may miss
subrequest specific filters between ap_subreq_core_filter and the protocol
output filters if not following your patch, correct?
Regards
Rüdiger
Re: [PATCH] PR 17629 and all that
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jun 08, 2010 at 05:20:16PM +0200, "Plüm, Rüdiger, VF-Group" wrote:
> Is it possible to have any non subrequest specific filters below the ap_subreq_core_filter_handle?
> It is no that the current patch does not handle this but wouldn't it be possible
> to just throw away any filter between
>
> r->output_filters and ap_subreq_core_filter_handle without any further test?
I'd spent some time working on exactly that approach, trying to answer
that question. But surprisingly the answer is "yes" - the subreq filter
has ftype=AP_FTYPE_CONTENT_SET, so any filter registered with an ftype
greater than that can be inserted and will work (so long as it doesn't
depend on seeing EOS).
So I couldn't convince myself it was correct to leave such filters in
place, though the situation is certainly odd.
A solution to this problem which André talked about in the list archive
was to turn the subreq core filter into a "chaining" filter, perhaps at
PROTOCOL level within the subreq, to ensure it is "really last" in the
chain. This seems cleaner than the current mess, but I couldn't get it
to work without regressions (particularly in fast internal redirect
handling).
Regards, Joe
RE: [PATCH] PR 17629 and all that
Posted by "Plüm, Rüdiger, VF-Group" <ru...@vodafone.com>.
> -----Original Message-----
> From: Joe Orton
> Sent: Dienstag, 8. Juni 2010 14:38
> To: dev@httpd.apache.org
> Subject: [PATCH] PR 17629 and all that
>
> https://issues.apache.org/bugzilla/show_bug.cgi?id=17629
>
> Here's an attempt at fixing the dreaded PR 17629. This is a
> bug in the
> handling of the output filter chain at the point where an internal
> redirect is applied to a subrequest. Complications:
>
> a) a subrequest's filter chain may start at an arbitrary point in
> r->main's filter chain
>
> -- for an SSI include, the output from the include must
> continue from
> the next filter along from the INCLUDES filter
>
> b) that point where the filter chains join cannot be lost by the
> subrequest doing an internal redirect
>
> -- obvious breakage as per the PR is losing the DEFLATE filter for
> the output of the redirect from the subreq
>
> c) the subrequest's filter chain cannot be preserved as-is,
> since it may
> contain filters which should not be applied to the destination URI
>
> The approach I've used is to distinguish inherited filters from
> subreq-specific filters by seeing what f->r points to, and nuking all
> but the inherited filters. Here's the patch - test case in the test
> suite:
>
> Index: modules/http/http_request.c
> ===================================================================
> --- modules/http/http_request.c (revision 952555)
> +++ modules/http/http_request.c (working copy)
> @@ -460,17 +460,47 @@
> new->proto_output_filters = r->proto_output_filters;
> new->proto_input_filters = r->proto_input_filters;
>
> - new->output_filters = new->proto_output_filters;
> new->input_filters = new->proto_input_filters;
>
> if (new->main) {
> - /* Add back the subrequest filter, which we lost when
> - * we set output_filters to include only the protocol
> - * output filters from the original request.
> - */
> - ap_add_output_filter_handle(ap_subreq_core_filter_handle,
> - NULL, new, new->connection);
> + ap_filter_t *f, *nextf;
> +
> + /* If this is a subrequest, the filter chain may contain a
> + * mixture of filters specific to the old request (r), and
> + * some inherited from r->main. Here, inherit that filter
> + * chain, and remove all those which are specific to the old
> + * request; ensuring the subreq filter is left in place. */
> + new->output_filters = r->output_filters;
> +
> + f = new->output_filters;
> + do {
> + nextf = f->next;
> +
> + if (f->r == r && f->frec !=
> ap_subreq_core_filter_handle) {
Is it possible to have any non subrequest specific filters below the ap_subreq_core_filter_handle?
It is no that the current patch does not handle this but wouldn't it be possible
to just throw away any filter between
r->output_filters and ap_subreq_core_filter_handle without any further test?
Otherwise patch looks good.
Regards
Rüdiger