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).
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?
>>
>> 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?
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) {
?
Regards,
Yann.
