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

CHANGES

@@ -6,6 +6,9 @@ Changes with Apache 2.5.0
      calls r:wsupgrade() can cause a child process crash. 
      [Edward Lu <Chaosed0 gmail.com>]
 
+  *) 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]
 

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)))
+                access_status = ap_send_http_trace(r);
-                    ap_die(access_status, r);
+                ap_die(access_status, r);
-                else
-                    ap_finalize_request_protocol(r);
                 return OK;
             }
             case M_OPTIONS: {
                 int access_status;
                 r->proxyreq = PROXYREQ_NONE;
-                if ((access_status = ap_send_http_options(r)))
+                access_status = ap_send_http_options(r);
-                    ap_die(access_status, r);
+                ap_die(access_status, r);
-                else
-                    ap_finalize_request_protocol(r);
                 return OK;
             }
             default: {

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 < 0) {
-        if (data_sent) {
+            /* 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))
+    if (!APR_BRIGADE_EMPTY(output_brigade)
-        ap_pass_brigade(r->output_filters, output_brigade);
+        && ap_pass_brigade(r->output_filters, output_brigade) != APR_SUCCESS) {
+        rv = AP_FILTER_ERROR;
+    }
 
     apr_brigade_destroy(output_brigade);
 

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';

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,
+                             apr_uint16_t request_id, const char **err,
-                             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);
+                                rv = ap_pass_brigade(r->output_filters, ob);
-                                if (status == HTTP_NOT_MODIFIED) {
+                                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;
     }
 

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);

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? */
+    if (ap_pass_brigade(r->output_filters, bb)) {
-    (void)ap_pass_brigade(r->output_filters, bb);
+        return AP_FILTER_ERROR;
+    }
 
     return OK;
 }