> -----Ursprüngliche Nachricht-----
> Von: Yann Ylavic [mailto:[email protected]]
> Gesendet: Mittwoch, 14. Dezember 2016 01:42
> An: httpd-dev <[email protected]>
> Betreff: Re: svn commit: r1773865 -
> /httpd/httpd/trunk/modules/http/http_filters.c
>
> On Tue, Dec 13, 2016 at 8:37 PM, Ruediger Pluem <[email protected]>
> wrote:
> >
> > On 12/13/2016 02:49 PM, Yann Ylavic wrote:
> >>
> >> The pros with this is indeed the reduced complexity (though the loop
> >> to walk the brigade already existed), the cons are that we rely on
> the
> >> caller/handler to not ignore ap_pass_brigade()'s return value and
> >
> > IMHO this would violate a principal and easy to follow design pattern.
> > I don't think that we can and want to protect module developers from
> > all errors they could make, especially if they are easy to spot
> > and understand. And if why catch this here?
>
> I agree that defensive programming in not good (though the whole
> check_headers() patchset is nothing else after all...).
>
> >
> > If someone in the the filter chain continues sending content even
> > after passing things down the chain caused an error weird things
> > can happen before us or if the source is after us after us.
>
> Agreed, still.
>
> >
> > This change is also unrelated to the bad header issue and I think
> > if there is interest to address this it should be done in a separate
> > patch set.
>
> It is actually, we could eventually avoid reading/consuming the body
> of the original response (closing prematuraly any backend
> connection...), but in any case we need to eat the ap_die()'s one if
> we encounter a bad header on its generated response.
> That's because we want to generate a minimal 500 response in this
> case, with no body (even the ap_die(500)'s one not be related,
> ErrorDocument can be any redirection, including proxying).
We could do that easier in this case:
Index: http_filters.c
===================================================================
--- http_filters.c (revision 1774129)
+++ http_filters.c (working copy)
@@ -1248,11 +1248,6 @@
}
}
if (ctx->headers_error) {
- if (!eos) {
- /* Eat body until EOS */
- apr_brigade_cleanup(b);
- return APR_SUCCESS;
- }
/* We may come back here from ap_die() below,
* so clear anything from this response.
@@ -1271,14 +1266,16 @@
ap_die(HTTP_INTERNAL_SERVER_ERROR, r);
return AP_FILTER_ERROR;
}
- AP_DEBUG_ASSERT(APR_BUCKET_IS_EOS(e));
- APR_BUCKET_REMOVE(e);
apr_brigade_cleanup(b);
+ e = apr_bucket_eoc_create(f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(b, e);
+ e = apr_bucket_eos_create(f->c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(b, e);
r->status = HTTP_INTERNAL_SERVER_ERROR;
r->content_type = r->content_encoding = NULL;
r->content_languages = NULL;
ap_set_content_length(r, 0);
+
}
else if (eb) {
int status;
The ap_http_outerror_filter will then kill any further possible data
that gets down the chain.
>
> Thus the eat-body code is needed anyway, let's also use it for the
> original response then.
>
> >
> > Furthermore it causes a handler to continue generating content
> > in a properly resource intensive way that we drop directly. And the
> > handler has no chance to know this because we return APR_SUCCESS.
>
> Hmm, we return AP_FILTER_ERROR right?
But only after we slurped all content. We return APR_SUCESS until we see EOS.
For the reasons above this is bad.
>
> >>
> >> I'm even inclined to do the below changes, so that we are really safe
> >> (i.e. ignore any data) after EOS...
> >
> > After this patch we do the wrong thing with an EOC bucket.
> > The current contract is that an EOC bucket *anywhere* in the brigade
> causes the
> > header filter to go out of the way. After the patch this is broken if
> > a bad header is present. But as EOC tells us to get out of the way and
> do nothing,
> > we don't need to take care about bad headers. This breaks edge case
> with mod_proxy_http
>
> OK, since we don't send the headers in the EOC case, I agree we should
> bypass check_headers(). But we may not be aware of an EOC unless it
> is part of the first brigade sent down the chain, and in this case
> check_headers() could trigger (that's fine I think, for late EOC
> cases, i.e. error while forwarding bodies, we need to send or block
> something already).
>
> >
> > Albeit this would be fixable I see more negative points then positive
> points here
> > and I am -0.5 on that content slurping.
>
> You mean, the one already backported to 2.4.x or the additional hunks
> I proposed above?
The code already in 2.4.x
> I think we should fix the EOC case in 2.4.x, but still eat ignored
> bodies in ap_http_header_filter().
>
> For the EOC case, how about:
>
> @@ -1227,13 +1224,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t)
> ap_http_heade
> e != APR_BRIGADE_SENTINEL(b);
> e = APR_BUCKET_NEXT(e))
> {
> - if (ctx->headers_error) {
> - if (APR_BUCKET_IS_EOS(e)) {
> - eos = 1;
> - break;
> - }
> - continue;
> - }
> if (AP_BUCKET_IS_ERROR(e) && !eb) {
> eb = e->data;
> continue;
> @@ -1246,6 +1236,11 @@ AP_CORE_DECLARE_NONSTD(apr_status_t)
> ap_http_heade
> ap_remove_output_filter(f);
> return ap_pass_brigade(f->next, b);
> }
> +
> + if (ctx->headers_error && APR_BUCKET_IS_EOS(e)) {
> + eos = 1;
> + break;
> + }
> }
> if (ctx->headers_error) {
> if (!eos) {
> ?
>
Looks good apart that I wouldn't do the break in the EOS case to catch EOC
buckets after the EOS.
I guess in practice that will not add much or additional rounds in the loop at
all, but it
Keeps the current expectation that EOC can be anywhere.
Regards
Rüdiger
