*) 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.



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

changes-entries/resp_passed.txt

@@ -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]

include/ap_mmn.h

@@ -717,6 +717,7 @@
  * 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" */
@@ -724,7 +725,7 @@
 #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

include/httpd.h

@@ -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;
 };
 
 /**

modules/http/http_filters.c

@@ -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);
+                if (r->final_resp_passed) {
-                ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
+                    ap_log_rerror(APLOG_MARK, APLOG_TRACE3, 0, r,
-                              "ap_http_header_filter error bucket, die with %d and error",
+                                  "ap_http_header_filter error bucket, should "
-                              status);
+                                  "die with status=%d but final response already "
-                /* This will invoke us again */
+                                  "underway", status);
-                ctx->dying = 1;
+                    ap_remove_output_filter(f);
-                ap_die(status, r);
+                    APR_BUCKET_REMOVE(e);
-                return AP_FILTER_ERROR;
+                    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;
     }

modules/http/http_request.c

@@ -84,38 +84,25 @@ static void ap_die_r(int type, request_rec *r, int recursive_error)
         return;
     }
 
+    /*
+     * 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 (!ap_is_HTTP_VALID_RESPONSE(type)) {
-        ap_filter_t *next;
+        if (type != AP_FILTER_ERROR) {
-
+            ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01579)
-        /*
+                          "Invalid response status %i", type);
-         * 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 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;
         }
         else {
-            return;
+            ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02831)
+                          "Response from AP_FILTER_ERROR");
         }
+        type = HTTP_INTERNAL_SERVER_ERROR;
     }
 
     /*

test/modules/http2/test_008_ranges.py

@@ -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}'

test/modules/http2/test_500_proxy.py

@@ -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

test/modules/http2/test_601_h2proxy_twisted.py

@@ -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']