]> git.ipfire.org Git - thirdparty/apache/httpd.git/commitdiff
*) core: add `final_resp_passed` flag to request_rec to allow
authorStefan Eissing <icing@apache.org>
Thu, 1 Jun 2023 12:21:03 +0000 (12:21 +0000)
committerStefan Eissing <icing@apache.org>
Thu, 1 Jun 2023 12:21:03 +0000 (12:21 +0000)
     ap_die() to judge if it can send out a response. Bump mmn.
     Enable test cases that check errors during response body to
     appear as error on client side.

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

changes-entries/resp_passed.txt [new file with mode: 0644]
include/ap_mmn.h
include/httpd.h
modules/http/http_filters.c
modules/http/http_request.c
test/modules/http2/test_008_ranges.py
test/modules/http2/test_500_proxy.py
test/modules/http2/test_601_h2proxy_twisted.py

diff --git a/changes-entries/resp_passed.txt b/changes-entries/resp_passed.txt
new file mode 100644 (file)
index 0000000..901b595
--- /dev/null
@@ -0,0 +1,5 @@
+  *) core: add `final_resp_passed` flag to request_rec to allow
+     ap_die() to judge if it can send out a response. Bump mmn.
+     Enable test cases that check errors during response body to
+     appear as error on client side.
+     [Stefan Eissing]
\ No newline at end of file
index 7b6e524aaeb50453cf2afd4d27a4b4afbe673748..b7acf6e4bfeb0c8704a8afc0ff006c3cb02ffbac 100644 (file)
  * 20211221.12 (2.5.1-dev) Add cmd_parms->regex
  * 20211221.13 (2.5.1-dev) Add hook token_checker to check for authorization other
  *                         than username / password. Add autht_provider structure.
+ * 20211221.14 (2.5.1-dev) Add request_rec->final_resp_passed bit
  */
 
 #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */
 #ifndef MODULE_MAGIC_NUMBER_MAJOR
 #define MODULE_MAGIC_NUMBER_MAJOR 20211221
 #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
index 1ef00212f4e7c8d3f91c2ef7b25389c955cbc411..0affc58edb2e3aba5288bc8e98f674b59784ab13 100644 (file)
@@ -1152,6 +1152,11 @@ struct request_rec {
      * to conclude that no body is there.
      */
     int body_indeterminate;
+    /** Whether a final (status >= 200) RESPONSE BUCKET has been passed down
+     * the output filters already. Relevant for ap_die().
+     *  TODO: compact elsewhere
+     */
+    unsigned int final_resp_passed:1;
 };
 
 /**
index 06ab85da4fd6c34576ed1384578c42190f77e3f1..6e3bd57ac57eade006b7b2c62095d1512ea13607 100644 (file)
@@ -1265,7 +1265,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
 
     if (ctx->final_status && ctx->final_header_only) {
         /* The final RESPONSE has already been sent or is in front of `bcontent`
-         * in the brigade. For a header_only respsone, remove all content buckets
+         * in the brigade. For a header_only respone, remove all content buckets
          * up to the first EOS. On seeing EOS, we remove ourself and are done.
          * NOTE that `header_only` responses never generate trailes.
          */
@@ -1287,6 +1287,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
         if (!APR_BRIGADE_EMPTY(b)) {
             rv = ap_pass_brigade(f->next, b);
         }
+        r->final_resp_passed = 1;
         return rv;
     }
 
@@ -1310,14 +1311,32 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
                 int status;
                 eb = e->data;
                 status = eb->status;
-                apr_brigade_cleanup(b);
-                ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
-                              "ap_http_header_filter error bucket, die with %d and error",
-                              status);
-                /* This will invoke us again */
-                ctx->dying = 1;
-                ap_die(status, r);
-                return AP_FILTER_ERROR;
+                if (r->final_resp_passed) {
+                    ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
+                                  "ap_http_header_filter error bucket, should "
+                                  "die with status=%d but final response already "
+                                  "underway", status);
+                    ap_remove_output_filter(f);
+                    APR_BUCKET_REMOVE(e);
+                    apr_brigade_cleanup(b);
+                    APR_BRIGADE_INSERT_TAIL(b, e);
+                    e = ap_bucket_eoc_create(c->bucket_alloc);
+                    APR_BRIGADE_INSERT_TAIL(b, e);
+                    e = apr_bucket_eos_create(c->bucket_alloc);
+                    APR_BRIGADE_INSERT_TAIL(b, e);
+                    c->aborted = 1;
+                    return ap_pass_brigade(f->next, b);
+                }
+                else {
+                    ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
+                                  "ap_http_header_filter error bucket, die with %d and error",
+                                  status);
+                    apr_brigade_cleanup(b);
+                    /* This will invoke us again */
+                    ctx->dying = 1;
+                    ap_die(status, r);
+                    return AP_FILTER_ERROR;
+                }
             }
         }
     }
@@ -1343,6 +1362,8 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_http_header_filter(ap_filter_t *f,
 
     rv = ap_pass_brigade(f->next, b);
 out:
+    if (ctx->final_status)
+        r->final_resp_passed = 1;
     if (recursive_error) {
         return AP_FILTER_ERROR;
     }
index 5327dd0e04e35f5f19addbc0b14c6f3636e54b7f..cb7af9cafb1b8ac5e69520b1711425d127a2b227 100644 (file)
@@ -84,38 +84,25 @@ static void ap_die_r(int type, request_rec *r, int recursive_error)
         return;
     }
 
-    if (!ap_is_HTTP_VALID_RESPONSE(type)) {
-        ap_filter_t *next;
-
-        /*
-         * Check if we still have the ap_http_header_filter in place. If
-         * this is the case we should not ignore the error here because
-         * it means that we have not sent any response at all and never
-         * will. This is bad. Sent an internal server error instead.
-         */
-        next = r->output_filters;
-        while (next && (next->frec != ap_http_header_filter_handle)) {
-               next = next->next;
-        }
+    /*
+     * if we have already passed the final response down the
+     * output filter chain, we cannot generate a second final
+     * response here.
+     */
+    if (r->final_resp_passed) {
+        return;
+    }
 
-        /*
-         * If next != NULL then we left the while above because of
-         * next->frec == ap_http_header_filter
-         */
-        if (next) {
-            if (type != AP_FILTER_ERROR) {
-                ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01579)
-                              "Invalid response status %i", type);
-            }
-            else {
-                ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02831)
-                              "Response from AP_FILTER_ERROR");
-            }
-            type = HTTP_INTERNAL_SERVER_ERROR;
+    if (!ap_is_HTTP_VALID_RESPONSE(type)) {
+        if (type != AP_FILTER_ERROR) {
+            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01579)
+                          "Invalid response status %i", type);
         }
         else {
-            return;
+            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02831)
+                          "Response from AP_FILTER_ERROR");
         }
+        type = HTTP_INTERNAL_SERVER_ERROR;
     }
 
     /*
index 5e2a061777b9d4faff0b4921d5cbc0b5bc706ef0..8bf8a3f161be17d2cd0b4283c48df678c233cd77 100644 (file)
@@ -122,7 +122,7 @@ class TestRanges:
                 assert e['bytes_rx_I'] > 0
                 assert e['bytes_resp_B'] == 100*1024*1024
                 assert e['bytes_tx_O'] > 1024
-                assert e['bytes_tx_O'] < 5*1024*1024  # curl buffers, but not that much
+                assert e['bytes_tx_O'] < 10*1024*1024  # curl buffers, but not that much
                 found = True
                 break
         assert found, f'request not found in {self.LOGFILE}'
index 306568e2d5a5c3ea1c1b4a6812c21ca9eaf1010f..88a8ece3f6e98667b91a3da1ef95f080db6cf1bf 100644 (file)
@@ -146,16 +146,12 @@ class TestProxy:
 
     # produce an error during response body
     def test_h2_500_31(self, env, repeat):
-        if env.httpd_is_at_least("2.5.0"):
-            pytest.skip("needs fix in core protocol handling")
         url = env.mkurl("https", "cgi", "/proxy/h2test/error?body_error=timeout")
         r = env.curl_get(url)
         assert r.exit_code != 0, r
 
     # produce an error, fail to generate an error bucket
     def test_h2_500_32(self, env, repeat):
-        if env.httpd_is_at_least("2.5.0"):
-            pytest.skip("needs fix in core protocol handling")
         url = env.mkurl("https", "cgi", "/proxy/h2test/error?body_error=timeout&error_bucket=0")
         r = env.curl_get(url)
         assert r.exit_code != 0, r
index 748a4940860b8a804488ae80c38b76c49552ee4f..224726eca04e740693e775931ea2f2c39b81c3d0 100644 (file)
@@ -45,8 +45,6 @@ class TestH2ProxyTwisted:
         "data-1k", "data-10k", "data-100k", "data-1m",
     ])
     def test_h2_601_03_echo_fail_early(self, env, name):
-        if env.httpd_is_at_least("2.5.0"):
-            pytest.skip("needs mod_proxy_http2 fix")
         fpath = os.path.join(env.gen_dir, name)
         url = env.mkurl("https", "cgi", "/h2proxy/h2test/echo?fail_after=512")
         r = env.curl_upload(url, fpath, options=[])
@@ -57,8 +55,6 @@ class TestH2ProxyTwisted:
         "data-1k", "data-10k", "data-100k", "data-1m",
     ])
     def test_h2_601_04_echo_fail_late(self, env, name):
-        if env.httpd_is_at_least("2.5.0"):
-            pytest.skip("needs mod_proxy_http2 fix")
         fpath = os.path.join(env.gen_dir, name)
         url = env.mkurl("https", "cgi", f"/h2proxy/h2test/echo?fail_after={os.path.getsize(fpath)}")
         r = env.curl_upload(url, fpath, options=[])
@@ -66,8 +62,6 @@ class TestH2ProxyTwisted:
         assert r.exit_code == 92 or r.response["status"] == 502
 
     def test_h2_601_05_echo_fail_many(self, env):
-        if env.httpd_is_at_least("2.5.0"):
-            pytest.skip("needs mod_proxy_http2 fix")
         count = 200
         fpath = os.path.join(env.gen_dir, "data-100k")
         args = [env.curl, '--parallel', '--parallel-max', '20']