diff options
author | Eric Covener <covener@apache.org> | 2016-12-22 14:54:33 +0000 |
---|---|---|
committer | Eric Covener <covener@apache.org> | 2016-12-22 14:54:33 +0000 |
commit | f8b61c1df288f660a63c4a4db0ede65624afec16 (patch) | |
tree | 2d13708b84979b5cceeab7d3e971cafdeadbf5e2 | |
parent | 20a29242f9204c16887db64e324615a52803f10b (diff) | |
download | httpd-f8b61c1df288f660a63c4a4db0ede65624afec16.tar.gz |
Merge r1773293, r1773761, r1773779, r1773812, r1773861, r1773862, r1773865 from trunk:
change error handling for bad resp headers
- avoid looping between ap_die and the http filter
- remove the header that failed the check
- keep calling apr_table_do until our fn stops matching
This is still not great. We get the original body, a 500 status
code and status line.
(r1773285 + fix for first return from check_headers)
Follow up to r1773293.
When check_headers() fails, clear anything (headers and body) from original/errorneous
response before returning 500.
Follow up to r1773761: don't check_headers() more than once.
Follow up to r1773761: don't recurse on internal redirects.
Follow up to r1773761: don't recurse on ap_send_error_response() either.
Follow up to r1773761: we need to check both ap_send_error_response() and internal redirect recursions.
Follow up to r1773761: improved recursion detection.
git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x-merge-http-strict@1775669 13f79535-47bb-0310-9956-ffa450edef68
-rw-r--r-- | modules/http/http_filters.c | 89 |
1 files changed, 70 insertions, 19 deletions
diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c index 027382b91c..05c9ba4ddc 100644 --- a/modules/http/http_filters.c +++ b/modules/http/http_filters.c @@ -738,10 +738,22 @@ static APR_INLINE int check_headers(request_rec *r) ctx.r = r; ctx.strict = (conf->http_conformance != AP_HTTP_CONFORMANCE_UNSAFE); - if (!apr_table_do(check_header, &ctx, r->headers_out, NULL)) - return 0; /* problem has been logged by check_header() */ + return apr_table_do(check_header, &ctx, r->headers_out, NULL) && + apr_table_do(check_header, &ctx, r->err_headers_out, NULL); +} - return 1; +static int check_headers_recursion(request_rec *r) +{ + request_rec *rr; + for (rr = r; rr; rr = rr->prev) { + void *dying = NULL; + apr_pool_userdata_get(&dying, "check_headers_recursion", rr->pool); + if (dying) { + return 1; + } + } + apr_pool_userdata_setn("true", "check_headers_recursion", NULL, r->pool); + return 0; } typedef struct header_struct { @@ -1235,6 +1247,7 @@ AP_DECLARE_NONSTD(int) ap_send_http_trace(request_rec *r) 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, @@ -1250,23 +1263,34 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f, header_filter_ctx *ctx = f->ctx; const char *ctype; ap_bucket_error *eb = NULL; + int eos = 0; AP_DEBUG_ASSERT(!r->main); - if (r->header_only || r->status == HTTP_NO_CONTENT) { - if (!ctx) { - ctx = f->ctx = apr_pcalloc(r->pool, sizeof(header_filter_ctx)); - } - else if (ctx->headers_sent) { + if (!ctx) { + ctx = f->ctx = apr_pcalloc(r->pool, sizeof(header_filter_ctx)); + } + 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); - return OK; + 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)) { + 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; @@ -1280,9 +1304,41 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f, return ap_pass_brigade(f->next, b); } } - if (eb) { - int status; + 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. + */ + ctx->headers_error = 0; + apr_table_clear(r->headers_out); + apr_table_clear(r->err_headers_out); + + /* 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 + * recursion; better fall through with 500, minimal headers and an + * empty body (EOS only). + */ + if (!check_headers_recursion(r)) { + apr_brigade_cleanup(b); + 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); + 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; status = eb->status; apr_brigade_cleanup(b); ap_die(status, r); @@ -1305,11 +1361,6 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f, r->headers_out); } - if (!check_headers(r)) { - ap_die(HTTP_INTERNAL_SERVER_ERROR, r); - return AP_FILTER_ERROR; - } - /* * Remove the 'Vary' header field if the client can't handle it. * Since this will have nasty effects on HTTP/1.1 caches, force @@ -1436,11 +1487,11 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f, terminate_header(b2); ap_pass_brigade(f->next, b2); + ctx->headers_sent = 1; if (r->header_only || r->status == HTTP_NO_CONTENT) { apr_brigade_cleanup(b); - ctx->headers_sent = 1; - return OK; + return APR_SUCCESS; } r->sent_bodyct = 1; /* Whatever follows is real body stuff... */ |