From: Graham Leggett Date: Tue, 14 May 2013 18:58:06 +0000 (+0000) Subject: core: Stop the HTTP_IN filter from attempting to write error buckets X-Git-Tag: 2.5.0-alpha~5456 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=9bc9d79079bbeeb65f46c59ab6aa40588cbde9ff;p=apache core: Stop the HTTP_IN filter from attempting to write error buckets to the output filters, which is bogus in the proxy case. Create a clean mapping from APR codes to HTTP status codes, and use it where needed. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1482522 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index c664391e19..1bedba70ad 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,11 @@ -*- coding: utf-8 -*- Changes with Apache 2.5.0 + *) core: Stop the HTTP_IN filter from attempting to write error buckets + to the output filters, which is bogus in the proxy case. Create a + clean mapping from APR codes to HTTP status codes, and use it where + needed. [Graham Leggett] + *) mod_proxy: Ensure we don't attempt to amend a table we are iterating through, ensuring that all headers listed by Connection are removed. [Graham Leggett, Co-Advisor ] diff --git a/docs/log-message-tags/next-number b/docs/log-message-tags/next-number index 70081150e1..721cbb13b4 100644 --- a/docs/log-message-tags/next-number +++ b/docs/log-message-tags/next-number @@ -1 +1 @@ -2475 +2478 diff --git a/include/ap_mmn.h b/include/ap_mmn.h index 425f3033da..f411492eb6 100644 --- a/include/ap_mmn.h +++ b/include/ap_mmn.h @@ -429,6 +429,7 @@ * ap_condition_if_modified_since(), * ap_condition_if_range() * 20121222.13 (2.5.0-dev) Add ap_proxy_clear_connection() + * 20121222.14 (2.5.0-dev) Add ap_map_http_request_error() */ #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */ @@ -436,7 +437,7 @@ #ifndef MODULE_MAGIC_NUMBER_MAJOR #define MODULE_MAGIC_NUMBER_MAJOR 20121222 #endif -#define MODULE_MAGIC_NUMBER_MINOR 13 /* 0...n */ +#define MODULE_MAGIC_NUMBER_MINOR 14 /* 0...n */ /** * Determine if the server's current MODULE_MAGIC_NUMBER is at least a diff --git a/include/http_protocol.h b/include/http_protocol.h index 415270b2ad..57fc9c0f66 100644 --- a/include/http_protocol.h +++ b/include/http_protocol.h @@ -502,6 +502,23 @@ AP_DECLARE(int) ap_should_client_block(request_rec *r); */ AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer, apr_size_t bufsiz); +/* + * Map specific APR codes returned by the filter stack to HTTP error + * codes, or the default status code provided. Use it as follows: + * + * return ap_map_http_response(rv, HTTP_BAD_REQUEST); + * + * If the filter has already handled the error, AP_FILTER_ERROR will + * be returned, which is cleanly passed through. + * + * These mappings imply that the filter stack is reading from the + * downstream client, the proxy will map these codes differently. + * @param rv APR status code + * @param status Default HTTP code should the APR code not be recognised + * @return Mapped HTTP status code + */ +AP_DECLARE(int) ap_map_http_request_error(apr_status_t rv, int status); + /** * In HTTP/1.1, any method can have a body. However, most GET handlers * wouldn't know what to do with a request body if they received one. diff --git a/modules/dav/main/mod_dav.c b/modules/dav/main/mod_dav.c index 6094049858..1d204e048f 100644 --- a/modules/dav/main/mod_dav.c +++ b/modules/dav/main/mod_dav.c @@ -1001,9 +1001,11 @@ static int dav_method_put(request_rec *r) } else { /* XXX: should this actually be HTTP_BAD_REQUEST? */ - http_err = HTTP_INTERNAL_SERVER_ERROR; - msg = apr_psprintf(r->pool, "An error occurred while reading" - " the request body (URI: %s)", msg); + http_err = ap_map_http_request_error(rc, + HTTP_INTERNAL_SERVER_ERROR); + msg = apr_psprintf(r->pool, + "An error occurred while reading" + " the request body (URI: %s)", msg); } err = dav_new_error(r->pool, http_err, 0, rc, msg); break; diff --git a/modules/generators/mod_cgi.c b/modules/generators/mod_cgi.c index 7808262f0f..57eebb1f42 100644 --- a/modules/generators/mod_cgi.c +++ b/modules/generators/mod_cgi.c @@ -857,7 +857,7 @@ static int cgi_handler(request_rec *r) } ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01225) "Error reading request entity data"); - return HTTP_INTERNAL_SERVER_ERROR; + return ap_map_http_request_error(rv, HTTP_INTERNAL_SERVER_ERROR); } for (bucket = APR_BRIGADE_FIRST(bb); diff --git a/modules/generators/mod_cgid.c b/modules/generators/mod_cgid.c index 56d3524330..07c31930e5 100644 --- a/modules/generators/mod_cgid.c +++ b/modules/generators/mod_cgid.c @@ -1473,7 +1473,7 @@ static int cgid_handler(request_rec *r) } ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01270) "Error reading request entity data"); - return HTTP_INTERNAL_SERVER_ERROR; + return ap_map_http_request_error(rv, HTTP_INTERNAL_SERVER_ERROR); } for (bucket = APR_BRIGADE_FIRST(bb); diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c index ff068bba82..9ce3752617 100644 --- a/modules/http/http_filters.c +++ b/modules/http/http_filters.c @@ -78,30 +78,6 @@ typedef struct http_filter_ctx { apr_bucket_brigade *bb; } http_ctx_t; -/* bail out if some error in the HTTP input filter happens */ -static apr_status_t bail_out_on_error(http_ctx_t *ctx, - ap_filter_t *f, - int http_error) -{ - apr_bucket *e; - apr_bucket_brigade *bb = ctx->bb; - - apr_brigade_cleanup(bb); - e = ap_bucket_error_create(http_error, - NULL, f->r->pool, - f->c->bucket_alloc); - APR_BRIGADE_INSERT_TAIL(bb, e); - e = apr_bucket_eos_create(f->c->bucket_alloc); - APR_BRIGADE_INSERT_TAIL(bb, e); - ctx->eos_sent = 1; - /* If chunked encoding / content-length are corrupt, we may treat parts - * of this request's body as the next one's headers. - * To be safe, disable keep-alive. - */ - f->r->connection->keepalive = AP_CONN_CLOSE; - return ap_pass_brigade(f->r->output_filters, bb); -} - static apr_status_t get_remaining_chunk_line(http_ctx_t *ctx, apr_bucket_brigade *b, int linelimit) @@ -269,7 +245,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, */ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01585) "Unknown Transfer-Encoding: %s", tenc); - return bail_out_on_error(ctx, f, HTTP_NOT_IMPLEMENTED); + return APR_ENOTIMPL; } else { ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, f->r, APLOGNO(01586) @@ -292,7 +268,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r, APLOGNO(01587) "Invalid Content-Length"); - return bail_out_on_error(ctx, f, HTTP_REQUEST_ENTITY_TOO_LARGE); + return APR_ENOSPC; } /* If we have a limit in effect and we know the C-L ahead of @@ -303,7 +279,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, "Requested content-length of %" APR_OFF_T_FMT " is larger than the configured limit" " of %" APR_OFF_T_FMT, ctx->remaining, ctx->limit); - return bail_out_on_error(ctx, f, HTTP_REQUEST_ENTITY_TOO_LARGE); + return APR_ENOSPC; } } @@ -396,10 +372,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, (ctx->remaining < 0) ? "(overflow)" : ""); ctx->remaining = 0; /* Reset it in case we have to * come back here later */ - if (APR_STATUS_IS_TIMEUP(rv)) { - http_error = HTTP_REQUEST_TIME_OUT; - } - return bail_out_on_error(ctx, f, http_error); + return rv; } if (!ctx->remaining) { @@ -502,10 +475,7 @@ apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, (ctx->remaining < 0) ? "(overflow)" : ""); ctx->remaining = 0; /* Reset it in case we have to * come back here later */ - if (APR_STATUS_IS_TIMEUP(rv)) { - http_error = HTTP_REQUEST_TIME_OUT; - } - return bail_out_on_error(ctx, f, http_error); + return rv; } if (!ctx->remaining) { @@ -1422,6 +1392,39 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f, return ap_pass_brigade(f->next, b); } +/* + * Map specific APR codes returned by the filter stack to HTTP error + * codes, or the default status code provided. Use it as follows: + * + * return ap_map_http_response(rv, HTTP_BAD_REQUEST); + * + * If the filter has already handled the error, AP_FILTER_ERROR will + * be returned, which is cleanly passed through. + * + * These mappings imply that the filter stack is reading from the + * downstream client, the proxy will map these codes differently. + */ +AP_DECLARE(int) ap_map_http_request_error(apr_status_t rv, int status) +{ + switch (rv) { + case AP_FILTER_ERROR: { + return AP_FILTER_ERROR; + } + case APR_ENOSPC: { + return HTTP_REQUEST_ENTITY_TOO_LARGE; + } + case APR_ENOTIMPL: { + return HTTP_NOT_IMPLEMENTED; + } + case APR_ETIMEDOUT: { + return HTTP_REQUEST_TIME_OUT; + } + default: { + return status; + } + } +} + /* In HTTP/1.1, any method can have a body. However, most GET handlers * wouldn't know what to do with a request body if they received one. * This helper routine tests for and reads any message body in the request, @@ -1439,7 +1442,8 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f, AP_DECLARE(int) ap_discard_request_body(request_rec *r) { apr_bucket_brigade *bb; - int rv, seen_eos; + int seen_eos; + apr_status_t rv; /* Sometimes we'll get in a state where the input handling has * detected an error where we want to drop the connection, so if @@ -1462,21 +1466,8 @@ AP_DECLARE(int) ap_discard_request_body(request_rec *r) APR_BLOCK_READ, HUGE_STRING_LEN); if (rv != APR_SUCCESS) { - /* FIXME: If we ever have a mapping from filters (apr_status_t) - * to HTTP error codes, this would be a good place for them. - * - * If we received the special case AP_FILTER_ERROR, it means - * that the filters have already handled this error. - * Otherwise, we should assume we have a bad request. - */ - if (rv == AP_FILTER_ERROR) { - apr_brigade_destroy(bb); - return rv; - } - else { - apr_brigade_destroy(bb); - return HTTP_BAD_REQUEST; - } + apr_brigade_destroy(bb); + return ap_map_http_request_error(rv, HTTP_BAD_REQUEST); } for (bucket = APR_BRIGADE_FIRST(bb); diff --git a/modules/proxy/mod_proxy.h b/modules/proxy/mod_proxy.h index f23aa65067..0089d5360b 100644 --- a/modules/proxy/mod_proxy.h +++ b/modules/proxy/mod_proxy.h @@ -863,6 +863,7 @@ PROXY_DECLARE(int) ap_proxy_connection_create(const char *proxy_function, * status HTTP_BAD_GATEWAY and an EOS bucket up the filter chain. * @param r current request record of client request * @param brigade The brigade that is sent through the output filter chain + * @deprecated To be removed in v2.6. */ PROXY_DECLARE(void) ap_proxy_backend_broke(request_rec *r, apr_bucket_brigade *brigade); diff --git a/modules/proxy/mod_proxy_http.c b/modules/proxy/mod_proxy_http.c index 622f132c15..b55b8cf4b0 100644 --- a/modules/proxy/mod_proxy_http.c +++ b/modules/proxy/mod_proxy_http.c @@ -1207,11 +1207,10 @@ apr_status_t ap_proxygetline(apr_bucket_brigade *bb, char *s, int n, request_rec #endif static -apr_status_t ap_proxy_http_process_response(apr_pool_t * p, request_rec *r, - proxy_conn_rec **backend_ptr, - proxy_worker *worker, - proxy_server_conf *conf, - char *server_portstr) { +int ap_proxy_http_process_response(apr_pool_t * p, request_rec *r, + proxy_conn_rec **backend_ptr, proxy_worker *worker, + proxy_server_conf *conf, char *server_portstr) +{ conn_rec *c = r->connection; char buffer[HUGE_STRING_LEN]; const char *buf; @@ -1308,36 +1307,18 @@ apr_status_t ap_proxy_http_process_response(apr_pool_t * p, request_rec *r, */ if (r->proxyreq == PROXYREQ_REVERSE && c->keepalives && !APR_STATUS_IS_TIMEUP(rc)) { - apr_bucket *eos; ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01104) "Closing connection to client because" " reading from backend server %s:%d failed." " Number of keepalives %i", backend->hostname, backend->port, c->keepalives); - ap_proxy_backend_broke(r, bb); - /* - * Add an EOC bucket to signal the ap_http_header_filter - * that it should get out of our way, BUT ensure that the - * EOC bucket is inserted BEFORE an EOS bucket in bb as - * some resource filters like mod_deflate pass everything - * up to the EOS down the chain immediately and sent the - * remainder of the brigade later (or even never). But in - * this case the ap_http_header_filter does not get out of - * our way soon enough. - */ + + e = ap_bucket_error_create(HTTP_GATEWAY_TIME_OUT, NULL, + r->pool, c->bucket_alloc); + APR_BRIGADE_INSERT_TAIL(bb, e); e = ap_bucket_eoc_create(c->bucket_alloc); - eos = APR_BRIGADE_LAST(bb); - while ((APR_BRIGADE_SENTINEL(bb) != eos) - && !APR_BUCKET_IS_EOS(eos)) { - eos = APR_BUCKET_PREV(eos); - } - if (eos == APR_BRIGADE_SENTINEL(bb)) { - APR_BRIGADE_INSERT_TAIL(bb, e); - } - else { - APR_BUCKET_INSERT_BEFORE(eos, e); - } + APR_BRIGADE_INSERT_TAIL(bb, e); ap_pass_brigade(r->output_filters, bb); /* Mark the backend connection for closing */ backend->close = 1; @@ -1698,14 +1679,31 @@ apr_status_t ap_proxy_http_process_response(apr_pool_t * p, request_rec *r, break; } else if (rv != APR_SUCCESS) { + if (rv == APR_ENOSPC) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(02475) + "Response chunk/line was too large to parse"); + } + else if (rv == APR_ENOTIMPL) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(02476) + "Response Transfer-Encoding was not recognised"); + } + else { + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01110) + "Network error reading response"); + } + /* In this case, we are in real trouble because - * our backend bailed on us. Pass along a 504 error - * error bucket + * our backend bailed on us. Given we're half way + * through a response, our only option is to + * disconnect the client too. */ - ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01110) - "error reading response"); - ap_proxy_backend_broke(r, bb); + e = ap_bucket_error_create(HTTP_GATEWAY_TIME_OUT, NULL, + r->pool, c->bucket_alloc); + APR_BRIGADE_INSERT_TAIL(bb, e); + e = ap_bucket_eoc_create(c->bucket_alloc); + APR_BRIGADE_INSERT_TAIL(bb, e); ap_pass_brigade(r->output_filters, bb); + backend_broke = 1; backend->close = 1; break; @@ -2024,7 +2022,7 @@ static int proxy_http_post_config(apr_pool_t *pconf, apr_pool_t *plog, ap_proxy_clear_connection_fn = APR_RETRIEVE_OPTIONAL_FN(ap_proxy_clear_connection); if (!ap_proxy_clear_connection_fn) { - ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO() + ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO(02477) "mod_proxy must be loaded for mod_proxy_http"); return !OK; } diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c index 0ffdbdcb1b..2b2eb8296e 100644 --- a/modules/proxy/proxy_util.c +++ b/modules/proxy/proxy_util.c @@ -2754,6 +2754,7 @@ int ap_proxy_lb_workers(void) return lb_workers_limit; } +/* deprecated - to be removed in v2.6 */ PROXY_DECLARE(void) ap_proxy_backend_broke(request_rec *r, apr_bucket_brigade *brigade) { diff --git a/server/util.c b/server/util.c index 6be14736b7..9a795eac37 100644 --- a/server/util.c +++ b/server/util.c @@ -2529,7 +2529,7 @@ AP_DECLARE(int) ap_parse_form_data(request_rec *r, ap_filter_t *f, APR_BLOCK_READ, HUGE_STRING_LEN); if (rv != APR_SUCCESS) { apr_brigade_destroy(bb); - return (rv == AP_FILTER_ERROR) ? rv : HTTP_BAD_REQUEST; + return ap_map_http_request_error(rv, HTTP_BAD_REQUEST); } for (bucket = APR_BRIGADE_FIRST(bb);