mod_proxy: Handle ap_proxy_buckets_lifetime_transform() errors.

* modules/proxy/mod_proxy.h,modules/proxy/proxy_util.c:
  Add ap_proxy_fill_error_brigade() to factorize proxy error handling
  on the client connection side.

* modules/proxy/mod_proxy_{http,ajp,uwsgi}.c:
  Use ap_proxy_fill_error_brigade() where needed, including when an
  empty brigade is returned on the backend side or when calling
  ap_proxy_buckets_lifetime_transform fails.



git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1893595 13f79535-47bb-0310-9956-ffa450edef68

docs/log-message-tags/next-number

@@ -1 +1 @@
-10293
+10295

include/ap_mmn.h

@@ -691,6 +691,7 @@
  *                         (this MAJOR bump). Overall there is no MAJOR bumb
  *                         for 20210506.0 + 20210924.0, MINOR bump only for
  *                         adding ap_proxy_tunnel_conn_bytes_{in,out}().
+ * 20210924.1 (2.5.1-dev)  Add ap_proxy_fill_error_brigade()
  */
 
 #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */
@@ -698,7 +699,7 @@
 #ifndef MODULE_MAGIC_NUMBER_MAJOR
 #define MODULE_MAGIC_NUMBER_MAJOR 20210924
 #endif
-#define MODULE_MAGIC_NUMBER_MINOR 0             /* 0...n */
+#define MODULE_MAGIC_NUMBER_MINOR 1             /* 0...n */
 
 /**
  * Determine if the server's current MODULE_MAGIC_NUMBER is at least a

modules/proxy/mod_proxy.h

@@ -1177,12 +1177,27 @@ PROXY_DECLARE(int) ap_proxy_connection_create_ex(const char *proxy_function,
 PROXY_DECLARE(int) ap_proxy_connection_reusable(proxy_conn_rec *conn);
 
 /**
- * Signal the upstream chain that the connection to the backend broke in the
+ * Fill a brigade that can be sent downstream to signal that the connection to
- * middle of the response. This is done by sending an error bucket with
+ * the backend broke in the middle of the response. The brigade will contain
- * status HTTP_BAD_GATEWAY and an EOS bucket up the filter chain.
+ * an error bucket with the given status and eventually an EOC bucket if asked
+ * to or heuristically if the response header was sent already.
  * @param r       current request record of client request
- * @param brigade The brigade that is sent through the output filter chain
+ * @param status  the error status
- * @deprecated To be removed in v2.6.
+ * @param bb      the brigade to fill
+ * @param eoc     0 do not add an EOC bucket, > 0 always add one, < 0 add one
+ *                only if the response header was sent already
+ */
+PROXY_DECLARE(void) ap_proxy_fill_error_brigade(request_rec *r, int status,
+                                                apr_bucket_brigade *bb,
+                                                int eoc);
+
+/**
+ * Fill a brigade that can be sent downstream to signal that the connection to
+ * the backend broke in the middle of the response. The brigade will contain
+ * an error bucket with status HTTP_BAD_GATEWAY and an EOS bucket.
+ * @param r       current request record of client request
+ * @param brigade the brigade to fill
+ * @deprecated To be removed in v2.6 (see ap_proxy_fill_error_brigade).
  */
 PROXY_DECLARE(void) ap_proxy_backend_broke(request_rec *r,
                                            apr_bucket_brigade *brigade);

modules/proxy/mod_proxy_ajp.c

@@ -670,7 +670,7 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
          * upwards in the chain.
          */
         if (data_sent) {
-            ap_proxy_backend_broke(r, output_brigade);
+            ap_proxy_fill_error_brigade(r, HTTP_BAD_GATEWAY, output_brigade, -1);
         } else if (!send_body && (is_idempotent(r) == METHOD_IDEMPOTENT)) {
             /*
              * This is only non fatal when we have not send (parts) of a possible
@@ -706,12 +706,9 @@ static int ap_proxy_ajp_request(apr_pool_t *p, request_rec *r,
 
     /*
      * Ensure that we sent an EOS bucket thru the filter chain, if we already
-     * have sent some data. Maybe ap_proxy_backend_broke was called and added
+     * have sent some data.
-     * one to the brigade already (no longer making it empty). So we should
-     * not do this in this case.
      */
-    if (data_sent && !r->eos_sent && !r->connection->aborted
+    if (data_sent && !r->eos_sent && !r->connection->aborted) {
-            && APR_BRIGADE_EMPTY(output_brigade)) {
         e = apr_bucket_eos_create(r->connection->bucket_alloc);
         APR_BRIGADE_INSERT_TAIL(output_brigade, e);
     }

modules/proxy/mod_proxy_http.c

@@ -1177,11 +1177,7 @@ int ap_proxy_http_process_response(proxy_http_req_t *req)
                               " Number of keepalives %i", backend->hostname,
                               backend->port, c->keepalives);
 
-                e = ap_bucket_error_create(HTTP_BAD_GATEWAY, NULL,
+                ap_proxy_fill_error_brigade(r, HTTP_BAD_GATEWAY, bb, 1);
-                        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);
                 /* Mark the backend connection for closing */
                 backend->close = 1;
@@ -1710,11 +1706,12 @@ int ap_proxy_http_process_response(proxy_http_req_t *req)
                     mode = APR_BLOCK_READ;
                     continue;
                 }
-                else if (rv == APR_EOF) {
+                if (rv == APR_EOF) {
                     backend->close = 1;
                     break;
                 }
-                else if (rv != APR_SUCCESS) {
+                if (rv != APR_SUCCESS || APR_BRIGADE_EMPTY(bb)) {
+                    int error_status = HTTP_BAD_GATEWAY;
                     if (rv == APR_ENOSPC) {
                         ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(02475)
                                       "Response chunk/line was too large to parse");
@@ -1723,10 +1720,15 @@ int ap_proxy_http_process_response(proxy_http_req_t *req)
                         ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(02476)
                                       "Response Transfer-Encoding was not recognised");
                     }
-                    else {
+                    else if (rv != APR_SUCCESS) {
                         ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01110)
                                       "Network error reading response");
                     }
+                    else {
+                        ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10293)
+                                      "Unexpected empty data reading response");
+                        error_status = HTTP_INTERNAL_SERVER_ERROR;
+                    }
 
                     /* In this case, we are in real trouble because
                      * our backend bailed on us. Given we're half way
@@ -1734,11 +1736,7 @@ int ap_proxy_http_process_response(proxy_http_req_t *req)
                      * disconnect the client too.
                      */
                     apr_brigade_cleanup(bb);
-                    e = ap_bucket_error_create(HTTP_BAD_GATEWAY, NULL,
+                    ap_proxy_fill_error_brigade(r, error_status, bb, 1);
-                            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;
@@ -1762,13 +1760,28 @@ int ap_proxy_http_process_response(proxy_http_req_t *req)
                               "readbytes: %#x", readbytes);
                 }
 #endif
-                /* sanity check */
-                if (APR_BRIGADE_EMPTY(bb)) {
-                    break;
-                }
 
                 /* Switch the allocator lifetime of the buckets */
-                ap_proxy_buckets_lifetime_transform(r, bb, pass_bb);
+                rv = ap_proxy_buckets_lifetime_transform(r, bb, pass_bb);
+                if (rv != APR_SUCCESS) {
+                    /* Same, half way through a response, our only option is
+                     * to notice the output filters and then disconnect the
+                     * client and backend.
+                     */
+                    if (!APR_BRIGADE_EMPTY(pass_bb)) {
+                        /* Pass what we have still */
+                        ap_pass_brigade(r->output_filters, pass_bb);
+                        apr_brigade_cleanup(pass_bb);
+                    }
+                    ap_proxy_fill_error_brigade(r, HTTP_INTERNAL_SERVER_ERROR,
+                                                pass_bb, 1);
+                    ap_pass_brigade(r->output_filters, pass_bb);
+                    apr_brigade_cleanup(pass_bb);
+
+                    backend_broke = 1;
+                    backend->close = 1;
+                    break;
+                }
 
                 /* found the last brigade? */
                 if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(pass_bb))) {

modules/proxy/mod_proxy_uwsgi.c

@@ -379,14 +379,11 @@ static int uwsgi_response(request_rec *r, proxy_conn_rec * backend,
         int status = r->status;
         r->status = HTTP_OK;
         r->status_line = NULL;
-
-        apr_brigade_cleanup(bb);
-        apr_brigade_cleanup(pass_bb);
-
         return status;
     }
 
     while (!finish) {
+        apr_brigade_cleanup(bb);
         rv = ap_get_brigade(rp->input_filters, bb,
                             AP_MODE_READBYTES, mode, conf->io_buffer_size);
         if (APR_STATUS_IS_EAGAIN(rv)
@@ -396,52 +393,60 @@ static int uwsgi_response(request_rec *r, proxy_conn_rec * backend,
             if (ap_pass_brigade(r->output_filters, bb) || c->aborted) {
                 break;
             }
-            apr_brigade_cleanup(bb);
             mode = APR_BLOCK_READ;
             continue;
         }
-        else if (rv == APR_EOF) {
+        if (rv == APR_EOF) {
             break;
         }
-        else if (rv != APR_SUCCESS) {
+        if (rv != APR_SUCCESS || APR_BRIGADE_EMPTY(bb)) {
-            ap_proxy_backend_broke(r, bb);
+            int status = HTTP_BAD_GATEWAY;
+            if (rv == APR_SUCCESS) {
+                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10294)
+                              "Unexpected empty data reading uwscgi response");
+                status = HTTP_INTERNAL_SERVER_ERROR;
+            }
+            apr_brigade_cleanup(bb);
+            ap_proxy_fill_error_brigade(r, status, bb, -1);
             ap_pass_brigade(r->output_filters, bb);
             backend_broke = 1;
             break;
         }
 
-        mode = APR_NONBLOCK_READ;
-        apr_brigade_length(bb, 0, &readbytes);
-        backend->worker->s->read += readbytes;
-
-        if (APR_BRIGADE_EMPTY(bb)) {
-            apr_brigade_cleanup(bb);
-            break;
-        }
-
-        ap_proxy_buckets_lifetime_transform(r, bb, pass_bb);
-
         /* found the last brigade? */
         if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb)))
             finish = 1;
 
-        /* do not pass chunk if it is zero_sized */
+        mode = APR_NONBLOCK_READ;
-        apr_brigade_length(pass_bb, 0, &readbytes);
+        apr_brigade_length(bb, 0, &readbytes);
+        backend->worker->s->read += readbytes;
 
-        if ((readbytes > 0
+        rv = ap_proxy_buckets_lifetime_transform(r, bb, pass_bb);
-             && ap_pass_brigade(r->output_filters, pass_bb) != APR_SUCCESS)
+        if (rv != APR_SUCCESS) {
+            /* Half way through a response, our only option is
+             * to notice the output filters and then disconnect the
+             * client and backend.
+             */
+            if (!APR_BRIGADE_EMPTY(pass_bb)) {
+                /* Pass what we have still */
+                ap_pass_brigade(r->output_filters, pass_bb);
+                apr_brigade_cleanup(pass_bb);
+            }
+            ap_proxy_fill_error_brigade(r, HTTP_INTERNAL_SERVER_ERROR,
+                                        pass_bb, -1);
+            ap_pass_brigade(r->output_filters, pass_bb);
+            apr_brigade_cleanup(pass_bb);
+
+            backend_broke = 1;
+            break;
+        }
+        if (ap_pass_brigade(r->output_filters, pass_bb) != APR_SUCCESS
             || c->aborted) {
             finish = 1;
         }
-
-        apr_brigade_cleanup(bb);
         apr_brigade_cleanup(pass_bb);
     }
 
-    e = apr_bucket_eos_create(c->bucket_alloc);
-    APR_BRIGADE_INSERT_TAIL(bb, e);
-    ap_pass_brigade(r->output_filters, bb);
-
     apr_brigade_cleanup(bb);
 
     if (c->aborted || backend_broke) {

modules/proxy/proxy_util.c

@@ -3542,11 +3542,54 @@ PROXY_DECLARE(int) ap_proxy_should_override(proxy_dir_conf *conf, int code)
                                  code);
 }
 
+PROXY_DECLARE(void) ap_proxy_fill_error_brigade(request_rec *r, int status,
+                                                apr_bucket_brigade *bb,
+                                                int eoc)
+{
+    apr_bucket *e, *eos;
+    conn_rec *c = r->connection;
+
+    /*
+     * Add an error and (eventually) EOC buckets to signal the http filter
+     * that it should get out of our way, BUT ensure that they are 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.
+     */
+
+    eos = APR_BRIGADE_LAST(bb);
+    while (eos != APR_BRIGADE_SENTINEL(bb) && !APR_BUCKET_IS_EOS(eos)) {
+        eos = APR_BUCKET_PREV(eos);
+    }
+
+    e = ap_bucket_error_create(status, NULL, c->pool, c->bucket_alloc);
+    if (eos == APR_BRIGADE_SENTINEL(bb)) {
+        APR_BRIGADE_INSERT_TAIL(bb, e);
+        eos = APR_BRIGADE_SENTINEL(bb);
+    }
+    else {
+        APR_BUCKET_INSERT_BEFORE(eos, e);
+    }
+
+    /* If asked to (eoc > 0) or if heuristically (eoc < 0) the header was
+     * sent already we need to terminate the connection.
+     */
+    if (eoc > 0 || (eoc < 0 && r->sent_bodyct)) {
+        e = ap_bucket_eoc_create(c->bucket_alloc);
+        if (eos == APR_BRIGADE_SENTINEL(bb)) {
+            APR_BRIGADE_INSERT_TAIL(bb, e);
+        }
+        else {
+            APR_BUCKET_INSERT_BEFORE(eos, e);
+        }
+    }
+}
+
 /* deprecated - to be removed in v2.6 */
 PROXY_DECLARE(void) ap_proxy_backend_broke(request_rec *r,
                                            apr_bucket_brigade *brigade)
 {
-    apr_bucket *e;
     conn_rec *c = r->connection;
 
     r->no_cache = 1;
@@ -3556,11 +3599,9 @@ PROXY_DECLARE(void) ap_proxy_backend_broke(request_rec *r,
      */
     if (r->main)
         r->main->no_cache = 1;
-    e = ap_bucket_error_create(HTTP_BAD_GATEWAY, NULL, c->pool,
+
-                               c->bucket_alloc);
+    APR_BRIGADE_INSERT_TAIL(brigade, apr_bucket_eos_create(c->bucket_alloc));
-    APR_BRIGADE_INSERT_TAIL(brigade, e);
+    ap_proxy_fill_error_brigade(r, HTTP_BAD_GATEWAY, brigade, 0);
-    e = apr_bucket_eos_create(c->bucket_alloc);
-    APR_BRIGADE_INSERT_TAIL(brigade, e);
 }
 
 /*