On Wed, Dec 14, 2016 at 9:45 AM, Plüm, Rüdiger, Vodafone Group
<[email protected]> wrote:
>
>> -----Ursprüngliche Nachricht-----
>> Von: Yann Ylavic [mailto:[email protected]]
>> Gesendet: Mittwoch, 14. Dezember 2016 01:42
>>
>> 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:
Actually I played with your proposal and it's indeed cleaner.
I also revised my position about whether we should or not close some
origin/backend connection early when turning the response into 500,
and think now we should make it know about the "issue" anyway (by
closing prematurely).
Hence the attached patch, no content slurping, use of AP_FILTER_ERROR
and EOC semantics to respectively warn the caller and cleanly
terminate the connection afterwards.
>>
>> 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.
Same issue w/o slurping the content if the response is made of
multiple brigades (i.e. EOS not in first one), because the http_header
filter is then removed and http_outerror would receive any following
METADATA directly (after the EOC+EOS we add ourself).
So I had still to handle this in the proposed patch (see
filter_error=1 handling), such that we always return AP_FILTER_ERROR
once the 500 is sent out (I could also have added the EOS after the
EOC only if it was already in the original brigade, but I think it's
better to stop the caller [ap_die() here] from sending more
[meta-]data, the connection is over anyway).
Overall I think it's still a simplification, more semantically correct too...
>>
>> For the EOC case, how about:
>
> Looks good apart that I wouldn't do the break in the EOS case to catch EOC
> buckets after the EOS.
Applied in r1774286 (and proposed for backport, 2.4.24?).
No hurry about the new (attached) patch however, only an improvement IMHO.
Regards,
Yann.
Index: modules/http/http_filters.c
===================================================================
--- modules/http/http_filters.c (revision 1774286)
+++ modules/http/http_filters.c (working copy)
@@ -1188,7 +1188,6 @@ AP_DECLARE_NONSTD(int) ap_send_http_trace(request_
typedef struct header_filter_ctx {
int headers_sent;
- int headers_error;
} header_filter_ctx;
AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
@@ -1203,7 +1202,8 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
header_filter_ctx *ctx = f->ctx;
const char *ctype;
ap_bucket_error *eb = NULL;
- apr_bucket *eos = NULL;
+ apr_status_t rv = APR_SUCCESS;
+ int filter_error = 0;
AP_DEBUG_ASSERT(!r->main);
@@ -1210,7 +1210,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
if (!ctx) {
ctx = f->ctx = apr_pcalloc(r->pool, sizeof(header_filter_ctx));
}
- if (ctx->headers_sent) {
+ else if (ctx->headers_sent) {
/* Eat body if response must not have one. */
if (r->header_only || r->status == HTTP_NO_CONTENT) {
apr_brigade_cleanup(b);
@@ -1217,9 +1217,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
return APR_SUCCESS;
}
}
- else if (!ctx->headers_error && !check_headers(r)) {
- ctx->headers_error = 1;
- }
+
for (e = APR_BRIGADE_FIRST(b);
e != APR_BRIGADE_SENTINEL(b);
e = APR_BUCKET_NEXT(e))
@@ -1236,23 +1234,15 @@ 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 = e;
- }
}
- if (ctx->headers_error) {
- if (!eos) {
- /* Eat body until EOS */
- apr_brigade_cleanup(b);
- return APR_SUCCESS;
- }
+ if (!ctx->headers_sent && !check_headers(r)) {
/* We may come back here from ap_die() below,
* so clear anything from this response.
*/
- ctx->headers_error = 0;
apr_table_clear(r->headers_out);
apr_table_clear(r->err_headers_out);
+ apr_brigade_cleanup(b);
/* Don't recall ap_die() if we come back here (from its own internal
* redirect or error response), otherwise we can end up in infinite
@@ -1260,17 +1250,18 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
* empty body (EOS only).
*/
if (!check_headers_recursion(r)) {
- apr_brigade_cleanup(b);
ap_die(HTTP_INTERNAL_SERVER_ERROR, r);
return AP_FILTER_ERROR;
}
- APR_BUCKET_REMOVE(eos);
- apr_brigade_cleanup(b);
- APR_BRIGADE_INSERT_TAIL(b, eos);
r->status = HTTP_INTERNAL_SERVER_ERROR;
+ e = ap_bucket_eoc_create(c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+ e = apr_bucket_eos_create(c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(b, e);
r->content_type = r->content_encoding = NULL;
r->content_languages = NULL;
ap_set_content_length(r, 0);
+ filter_error = 1;
}
else if (eb) {
int status;
@@ -1283,7 +1274,8 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
if (r->assbackwards) {
r->sent_bodyct = 1;
ap_remove_output_filter(f);
- return ap_pass_brigade(f->next, b);
+ rv = ap_pass_brigade(f->next, b);
+ goto out;
}
/*
@@ -1403,12 +1395,15 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
terminate_header(b2);
- ap_pass_brigade(f->next, b2);
+ rv = ap_pass_brigade(f->next, b2);
+ if (rv != APR_SUCCESS) {
+ goto out;
+ }
ctx->headers_sent = 1;
if (r->header_only || r->status == HTTP_NO_CONTENT) {
apr_brigade_cleanup(b);
- return APR_SUCCESS;
+ goto out;
}
r->sent_bodyct = 1; /* Whatever follows is real body stuff... */
@@ -1426,7 +1421,12 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_heade
* brigade won't be chunked properly.
*/
ap_remove_output_filter(f);
- return ap_pass_brigade(f->next, b);
+ rv = ap_pass_brigade(f->next, b);
+out:
+ if (filter_error) {
+ return AP_FILTER_ERROR;
+ }
+ return rv;
}
/*
