From: Stefan Eissing Date: Thu, 1 Jun 2023 12:21:03 +0000 (+0000) Subject: *) core: add `final_resp_passed` flag to request_rec to allow X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ed69ae3384bd80c0c80f7a421662331c29c2e72c;p=thirdparty%2Fapache%2Fhttpd.git *) 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 --- diff --git a/changes-entries/resp_passed.txt b/changes-entries/resp_passed.txt new file mode 100644 index 00000000000..901b59566ba --- /dev/null +++ b/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] \ No newline at end of file diff --git a/include/ap_mmn.h b/include/ap_mmn.h index 7b6e524aaeb..b7acf6e4bfe 100644 --- a/include/ap_mmn.h +++ b/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 diff --git a/include/httpd.h b/include/httpd.h index 1ef00212f4e..0affc58edb2 100644 --- a/include/httpd.h +++ b/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; }; /** diff --git a/modules/http/http_filters.c b/modules/http/http_filters.c index 06ab85da4fd..6e3bd57ac57 100644 --- a/modules/http/http_filters.c +++ b/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); - 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; } diff --git a/modules/http/http_request.c b/modules/http/http_request.c index 5327dd0e04e..cb7af9cafb1 100644 --- a/modules/http/http_request.c +++ b/modules/http/http_request.c @@ -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; } /* diff --git a/test/modules/http2/test_008_ranges.py b/test/modules/http2/test_008_ranges.py index 5e2a061777b..8bf8a3f161b 100644 --- a/test/modules/http2/test_008_ranges.py +++ b/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}' diff --git a/test/modules/http2/test_500_proxy.py b/test/modules/http2/test_500_proxy.py index 306568e2d5a..88a8ece3f6e 100644 --- a/test/modules/http2/test_500_proxy.py +++ b/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 diff --git a/test/modules/http2/test_601_h2proxy_twisted.py b/test/modules/http2/test_601_h2proxy_twisted.py index 748a4940860..224726eca04 100644 --- a/test/modules/http2/test_601_h2proxy_twisted.py +++ b/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']