From: Graham Leggett <minfrin@apache.org>
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 <coad measurement-factory.com>]
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);