From 19aa2d83b379719420f3a178413325156d7a62f3 Mon Sep 17 00:00:00 2001
From: Yann Ylavic <ylavic@apache.org>
Date: Mon, 7 Mar 2022 14:46:08 +0000
Subject: [PATCH] core: Simpler connection close logic if discarding the
 request body fails.

If ap_discard_request_body() sets AP_CONN_CLOSE by itself it simplifies and
allows to consolidate end_output_stream() and error_output_stream().


Merge r1898683 from trunk.
Submitted by: ylavic, rpluem
Reviewed by: ylavic, rpluem, covener


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1898692 13f79535-47bb-0310-9956-ffa450edef68
---
 changes-entries/discard_body.diff |  2 +
 modules/http/http_filters.c       | 69 ++++++++++++++++---------------
 server/protocol.c                 | 14 +++++--
 3 files changed, 48 insertions(+), 37 deletions(-)
 create mode 100644 changes-entries/discard_body.diff

diff --git a/changes-entries/discard_body.diff b/changes-entries/discard_body.diff
new file mode 100644
index 00000000000..6b467ac5ee3
--- /dev/null
+++ b/changes-entries/discard_body.diff
@@ -0,0 +1,2 @@
+  *) core: Simpler connection close logic if discarding the request body fails.
+     [Yann Ylavic, Ruediger Pluem]
\ No newline at end of file
diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c
index d9b36212155..43e8c6dd5d5 100644
--- a/modules/http/http_filters.c
+++ b/modules/http/http_filters.c
@@ -1598,9 +1598,9 @@ AP_DECLARE(int) ap_map_http_request_error(apr_status_t rv, int status)
  */
 AP_DECLARE(int) ap_discard_request_body(request_rec *r)
 {
+    int rc = OK;
+    conn_rec *c = r->connection;
     apr_bucket_brigade *bb;
-    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
@@ -1609,54 +1609,57 @@ AP_DECLARE(int) ap_discard_request_body(request_rec *r)
      *
      * This function is also a no-op on a subrequest.
      */
-    if (r->main || r->connection->keepalive == AP_CONN_CLOSE ||
-        ap_status_drops_connection(r->status)) {
+    if (r->main || c->keepalive == AP_CONN_CLOSE) {
+        return OK;
+    }
+    if (ap_status_drops_connection(r->status)) {
+        c->keepalive = AP_CONN_CLOSE;
         return OK;
     }
 
     bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
-    seen_eos = 0;
-    do {
-        apr_bucket *bucket;
+    for (;;) {
+        apr_status_t rv;
 
         rv = ap_get_brigade(r->input_filters, bb, AP_MODE_READBYTES,
                             APR_BLOCK_READ, HUGE_STRING_LEN);
-
         if (rv != APR_SUCCESS) {
-            apr_brigade_destroy(bb);
-            return ap_map_http_request_error(rv, HTTP_BAD_REQUEST);
+            rc = ap_map_http_request_error(rv, HTTP_BAD_REQUEST);
+            goto cleanup;
         }
 
-        for (bucket = APR_BRIGADE_FIRST(bb);
-             bucket != APR_BRIGADE_SENTINEL(bb);
-             bucket = APR_BUCKET_NEXT(bucket))
-        {
-            const char *data;
-            apr_size_t len;
+        while (!APR_BRIGADE_EMPTY(bb)) {
+            apr_bucket *b = APR_BRIGADE_FIRST(bb);
 
-            if (APR_BUCKET_IS_EOS(bucket)) {
-                seen_eos = 1;
-                break;
-            }
-
-            /* These are metadata buckets. */
-            if (bucket->length == 0) {
-                continue;
+            if (APR_BUCKET_IS_EOS(b)) {
+                goto cleanup;
             }
 
-            /* We MUST read because in case we have an unknown-length
-             * bucket or one that morphs, we want to exhaust it.
+            /* There is no need to read empty or metadata buckets or
+             * buckets of known length, but we MUST read buckets of
+             * unknown length in order to exhaust them.
              */
-            rv = apr_bucket_read(bucket, &data, &len, APR_BLOCK_READ);
-            if (rv != APR_SUCCESS) {
-                apr_brigade_destroy(bb);
-                return HTTP_BAD_REQUEST;
+            if (b->length == (apr_size_t)-1) {
+                apr_size_t len;
+                const char *data;
+
+                rv = apr_bucket_read(b, &data, &len, APR_BLOCK_READ);
+                if (rv != APR_SUCCESS) {
+                    rc = HTTP_BAD_REQUEST;
+                    goto cleanup;
+                }
             }
+
+            apr_bucket_delete(b);
         }
-        apr_brigade_cleanup(bb);
-    } while (!seen_eos);
+    }
 
-    return OK;
+cleanup:
+    apr_brigade_cleanup(bb);
+    if (rc != OK) {
+        c->keepalive = AP_CONN_CLOSE;
+    }
+    return rc;
 }
 
 /* Here we deal with getting the request message body from the client.
diff --git a/server/protocol.c b/server/protocol.c
index 2214f72b5a4..298f61e1fb8 100644
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -1687,23 +1687,29 @@ AP_DECLARE(void) ap_set_sub_req_protocol(request_rec *rnew,
     rnew->main = (request_rec *) r;
 }
 
-static void end_output_stream(request_rec *r)
+static void end_output_stream(request_rec *r, int status)
 {
     conn_rec *c = r->connection;
     apr_bucket_brigade *bb;
     apr_bucket *b;
 
     bb = apr_brigade_create(r->pool, c->bucket_alloc);
+    if (status != OK) {
+        b = ap_bucket_error_create(status, NULL, r->pool, c->bucket_alloc);
+        APR_BRIGADE_INSERT_TAIL(bb, b);
+    }
     b = apr_bucket_eos_create(c->bucket_alloc);
     APR_BRIGADE_INSERT_TAIL(bb, b);
+
     ap_pass_brigade(r->output_filters, bb);
+    apr_brigade_cleanup(bb);
 }
 
 AP_DECLARE(void) ap_finalize_sub_req_protocol(request_rec *sub)
 {
     /* tell the filter chain there is no more content coming */
     if (!sub->eos_sent) {
-        end_output_stream(sub);
+        end_output_stream(sub, OK);
     }
 }
 
@@ -1714,11 +1720,11 @@ AP_DECLARE(void) ap_finalize_sub_req_protocol(request_rec *sub)
  */
 AP_DECLARE(void) ap_finalize_request_protocol(request_rec *r)
 {
-    (void) ap_discard_request_body(r);
+    int status = ap_discard_request_body(r);
 
     /* tell the filter chain there is no more content coming */
     if (!r->eos_sent) {
-        end_output_stream(r);
+        end_output_stream(r, status);
     }
 }