From: Yann Ylavic Date: Fri, 6 Feb 2015 16:54:16 +0000 (+0000) Subject: mod_proxy(es): Avoid error response/document handling by the core if some X-Git-Tag: 2.5.0-alpha~3471 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b26841b8dc2fbd08a4345aaa9789e4655bcc9a7a;p=apache mod_proxy(es): Avoid error response/document handling by the core if some input filter already did it while reading client's payload. When an input filter returns AP_FILTER_ERROR, it has already called ap_die() or at least already responded to the client. Here we don't want to lose AP_FILTER_ERROR when returning from proxy handlers, so we use ap_map_http_request_error() to forward any AP_FILTER_ERROR to ap_die() which knows whether a response needs to be completed or not. Before this commit, returning an HTTP error code in this case caused a double response to be generated. Depends on r1657881 to preserve r->status (for logging) when nothing is to be done by ap_die() when handling AP_FILTER_ERROR. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1657897 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index a9b3723bb9..93383bdd3c 100644 --- a/CHANGES +++ b/CHANGES @@ -6,6 +6,9 @@ Changes with Apache 2.5.0 calls r:wsupgrade() can cause a child process crash. [Edward Lu ] + *) mod_proxy(es): Avoid error response/document handling by the core if some + input filter already did it while reading client's payload. [Yann Ylavic] + *) http: Make ap_die() robust against any HTTP error code and not modify response status (finally logged) when nothing is to be done. [Yann Ylavic] diff --git a/modules/proxy/mod_proxy.c b/modules/proxy/mod_proxy.c index 06ec750387..a1403e1e80 100644 --- a/modules/proxy/mod_proxy.c +++ b/modules/proxy/mod_proxy.c @@ -970,19 +970,15 @@ static int proxy_handler(request_rec *r) case M_TRACE: { int access_status; r->proxyreq = PROXYREQ_NONE; - if ((access_status = ap_send_http_trace(r))) - ap_die(access_status, r); - else - ap_finalize_request_protocol(r); + access_status = ap_send_http_trace(r); + ap_die(access_status, r); return OK; } case M_OPTIONS: { int access_status; r->proxyreq = PROXYREQ_NONE; - if ((access_status = ap_send_http_options(r))) - ap_die(access_status, r); - else - ap_finalize_request_protocol(r); + access_status = ap_send_http_options(r); + ap_die(access_status, r); return OK; } default: { diff --git a/modules/proxy/mod_proxy_ajp.c b/modules/proxy/mod_proxy_ajp.c index bdd0031379..a0f3253c0e 100644 --- a/modules/proxy/mod_proxy_ajp.c +++ b/modules/proxy/mod_proxy_ajp.c @@ -256,10 +256,10 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r, if (status != APR_SUCCESS) { /* We had a failure: Close connection to backend */ conn->close = 1; - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00871) + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, status, r, APLOGNO(00871) "ap_get_brigade failed"); apr_brigade_destroy(input_brigade); - return HTTP_BAD_REQUEST; + return ap_map_http_request_error(status, HTTP_BAD_REQUEST); } /* have something */ @@ -394,6 +394,9 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r, if (APR_STATUS_IS_TIMEUP(status)) { rv = HTTP_REQUEST_TIME_OUT; } + else if (status == AP_FILTER_ERROR) { + data_sent = -1; + } output_failed = 1; break; } @@ -608,8 +611,13 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r, "output: %i", backend_failed, output_failed); /* We had a failure: Close connection to backend */ conn->close = 1; - /* Return DONE to avoid error messages being added to the stream */ - if (data_sent) { + if (data_sent < 0) { + /* Return AP_FILTER_ERROR to let ap_die() handle the error */ + rv = AP_FILTER_ERROR; + data_sent = 0; + } + else if (data_sent) { + /* Return DONE to avoid error messages being added to the stream */ rv = DONE; } } @@ -705,8 +713,10 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r, } /* If we have added something to the brigade above, send it */ - if (!APR_BRIGADE_EMPTY(output_brigade)) - ap_pass_brigade(r->output_filters, output_brigade); + if (!APR_BRIGADE_EMPTY(output_brigade) + && ap_pass_brigade(r->output_filters, output_brigade) != APR_SUCCESS) { + rv = AP_FILTER_ERROR; + } apr_brigade_destroy(output_brigade); diff --git a/modules/proxy/mod_proxy_balancer.c b/modules/proxy/mod_proxy_balancer.c index d2f9f5b5ac..54acef3de1 100644 --- a/modules/proxy/mod_proxy_balancer.c +++ b/modules/proxy/mod_proxy_balancer.c @@ -1039,7 +1039,7 @@ static int balancer_handler(request_rec *r) rv = ap_get_brigade(r->input_filters, ib, AP_MODE_READBYTES, APR_BLOCK_READ, len); if (rv != APR_SUCCESS) { - return HTTP_INTERNAL_SERVER_ERROR; + return ap_map_http_request_error(rv, HTTP_BAD_REQUEST); } apr_brigade_flatten(ib, buf, &len); buf[len] = '\0'; diff --git a/modules/proxy/mod_proxy_fcgi.c b/modules/proxy/mod_proxy_fcgi.c index 0e33545d78..ba813ed500 100644 --- a/modules/proxy/mod_proxy_fcgi.c +++ b/modules/proxy/mod_proxy_fcgi.c @@ -427,8 +427,8 @@ static int handle_headers(request_rec *r, int *state, static apr_status_t dispatch(proxy_conn_rec *conn, proxy_dir_conf *conf, request_rec *r, apr_pool_t *setaside_pool, - apr_uint16_t request_id, - const char **err) + apr_uint16_t request_id, const char **err, + int *bad_request, int *has_responded) { apr_bucket_brigade *ib, *ob; int seen_end_of_headers = 0, done = 0, ignore_body = 0; @@ -486,6 +486,7 @@ static apr_status_t dispatch(proxy_conn_rec *conn, proxy_dir_conf *conf, iobuf_size); if (rv != APR_SUCCESS) { *err = "reading input brigade"; + *bad_request = 1; break; } @@ -637,9 +638,14 @@ recv_again: apr_brigade_cleanup(ob); tmp_b = apr_bucket_eos_create(c->bucket_alloc); APR_BRIGADE_INSERT_TAIL(ob, tmp_b); + + *has_responded = 1; r->status = status; - ap_pass_brigade(r->output_filters, ob); - if (status == HTTP_NOT_MODIFIED) { + rv = ap_pass_brigade(r->output_filters, ob); + if (rv != APR_SUCCESS) { + *err = "passing headers brigade to output filters"; + } + else if (status == HTTP_NOT_MODIFIED) { /* The 304 response MUST NOT contain * a message-body, ignore it. */ ignore_body = 1; @@ -671,6 +677,7 @@ recv_again: /* Send the part of the body that we read while * reading the headers. */ + *has_responded = 1; rv = ap_pass_brigade(r->output_filters, ob); if (rv != APR_SUCCESS) { *err = "passing brigade to output filters"; @@ -696,6 +703,7 @@ recv_again: * along smaller chunks */ if (script_error_status == HTTP_OK && !ignore_body) { + *has_responded = 1; rv = ap_pass_brigade(r->output_filters, ob); if (rv != APR_SUCCESS) { *err = "passing brigade to output filters"; @@ -717,6 +725,8 @@ recv_again: if (script_error_status == HTTP_OK) { b = apr_bucket_eos_create(c->bucket_alloc); APR_BRIGADE_INSERT_TAIL(ob, b); + + *has_responded = 1; rv = ap_pass_brigade(r->output_filters, ob); if (rv != APR_SUCCESS) { *err = "passing brigade to output filters"; @@ -771,6 +781,7 @@ recv_again: if (script_error_status != HTTP_OK) { ap_die(script_error_status, r); /* send ErrorDocument */ + *has_responded = 1; } return rv; @@ -795,6 +806,8 @@ static int fcgi_do_request(apr_pool_t *p, request_rec *r, apr_status_t rv; apr_pool_t *temp_pool; const char *err; + int bad_request = 0, + has_responded = 0; /* Step 1: Send AP_FCGI_BEGIN_REQUEST */ rv = send_begin_request(conn, request_id); @@ -817,7 +830,8 @@ static int fcgi_do_request(apr_pool_t *p, request_rec *r, } /* Step 3: Read records from the back end server and handle them. */ - rv = dispatch(conn, conf, r, temp_pool, request_id, &err); + rv = dispatch(conn, conf, r, temp_pool, request_id, + &err, &bad_request, &has_responded); if (rv != APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01075) "Error dispatching request to %s: %s%s%s", @@ -826,6 +840,12 @@ static int fcgi_do_request(apr_pool_t *p, request_rec *r, err ? err : "", err ? ")" : ""); conn->close = 1; + if (has_responded) { + return AP_FILTER_ERROR; + } + if (bad_request) { + return ap_map_http_request_error(rv, HTTP_BAD_REQUEST); + } return HTTP_SERVICE_UNAVAILABLE; } diff --git a/modules/proxy/mod_proxy_http.c b/modules/proxy/mod_proxy_http.c index ae9c479047..ddab4a2f02 100644 --- a/modules/proxy/mod_proxy_http.c +++ b/modules/proxy/mod_proxy_http.c @@ -337,7 +337,7 @@ static int stream_reqbody_chunked(apr_pool_t *p, " from %s (%s)", p_conn->addr, p_conn->hostname ? p_conn->hostname: "", c->client_ip, c->remote_host ? c->remote_host: ""); - return HTTP_BAD_REQUEST; + return ap_map_http_request_error(status, HTTP_BAD_REQUEST); } } @@ -502,7 +502,7 @@ static int stream_reqbody_cl(apr_pool_t *p, " from %s (%s)", p_conn->addr, p_conn->hostname ? p_conn->hostname: "", c->client_ip, c->remote_host ? c->remote_host: ""); - return HTTP_BAD_REQUEST; + return ap_map_http_request_error(status, HTTP_BAD_REQUEST); } } @@ -653,7 +653,7 @@ static int spool_reqbody_cl(apr_pool_t *p, ap_log_rerror(APLOG_MARK, APLOG_ERR, status, r, APLOGNO(02610) "read request body failed from %s (%s)", c->client_ip, c->remote_host ? c->remote_host: ""); - return HTTP_BAD_REQUEST; + return ap_map_http_request_error(status, HTTP_BAD_REQUEST); } if (bytes_spooled || force_cl) { @@ -837,7 +837,7 @@ static int ap_proxy_http_prefetch(apr_pool_t *p, request_rec *r, " from %s (%s)", p_conn->addr, p_conn->hostname ? p_conn->hostname: "", c->client_ip, c->remote_host ? c->remote_host: ""); - return HTTP_BAD_REQUEST; + return ap_map_http_request_error(status, HTTP_BAD_REQUEST); } apr_brigade_length(temp_brigade, 1, &bytes); diff --git a/modules/proxy/mod_proxy_scgi.c b/modules/proxy/mod_proxy_scgi.c index 7fb2b873a9..464a3bc355 100644 --- a/modules/proxy/mod_proxy_scgi.c +++ b/modules/proxy/mod_proxy_scgi.c @@ -431,8 +431,9 @@ static int pass_response(request_rec *r, proxy_conn_rec *conn) } } - /* XXX: What could we do with that return code? */ - (void)ap_pass_brigade(r->output_filters, bb); + if (ap_pass_brigade(r->output_filters, bb)) { + return AP_FILTER_ERROR; + } return OK; }