Skip to content

Commit 094f580

Browse files
committed
mod_http2: RST_STREAM an incomplete response on the legacy c2 path
On builds without AP_HAS_RESPONSE_BUCKETS (httpd 2.4.x), the h2 worker runs the c2 connection through h2_c2_process()/c2_process() rather than the generic ap_process_connection(). c2_process() closes the output beam unconditionally when the handler returns, so a response that started but never finished (no EOS, e.g. a CGI that hits the server Timeout mid-body) leaves a closed beam that h2_beam_is_complete() reports as complete. s_c2_done() then sends no reset, and stream_data_cb()'s reset fires only on a c1 re-dispatch that is lost under load, so the stream is parked and a client without a stall timeout hangs. When c2_process() finishes with a final response started but no EOS seen (response_eos_seen, from the preceding commit), abort the output beam instead of closing it; an aborted beam returns APR_ECONNABORTED from h2_beam_receive() on the next pull, so the reset fires regardless of the wakeup. Header-only responses (204/304, and HEAD) are marked complete on this path too (h2_c2_filter.c) so they are not aborted. Adds test_h2_105_20: a CGI that goes silent past Timeout, run as concurrent streams, must be reset.
1 parent 85a7f40 commit 094f580

4 files changed

Lines changed: 86 additions & 2 deletions

File tree

modules/http2/h2_c2.c

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -824,7 +824,20 @@ static apr_status_t c2_process(h2_conn_ctx_t *conn_ctx, conn_rec *c)
824824
* request pool may have been deleted. */
825825
r = NULL;
826826
if (conn_ctx->beam_out) {
827-
h2_beam_close(conn_ctx->beam_out, c);
827+
if (conn_ctx->has_final_response && !conn_ctx->response_eos_seen) {
828+
/* A final response was started but never completed (no EOS), e.g.
829+
* the CGI/handler timed out mid-body. Abort the output beam so the
830+
* HTTP/2 stream is RST_STREAM'd. Otherwise s_c2_done() sees the closed
831+
* beam as a complete response, sends no reset, and the client is left
832+
* waiting for data/EOS that never arrive. Header-only responses
833+
* (204/304/HEAD) are marked complete earlier (see the
834+
* AP_STATUS_IS_HEADER_ONLY handling) so they are NOT aborted here;
835+
* aborting them would re-open PR 69580. */
836+
h2_beam_abort(conn_ctx->beam_out, c);
837+
}
838+
else {
839+
h2_beam_close(conn_ctx->beam_out, c);
840+
}
828841
}
829842

830843
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c,

modules/http2/h2_c2_filter.c

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -549,6 +549,12 @@ static apr_status_t pass_response(h2_conn_ctx_t *conn_ctx, ap_filter_t *f,
549549

550550
if (response->status >= HTTP_OK) {
551551
conn_ctx->has_final_response = 1;
552+
if (AP_STATUS_IS_HEADER_ONLY(response->status)) {
553+
/* A header-only response (204/304) is complete without a body
554+
* EOS; mark it complete so c2_process() does not abort/reset it as
555+
* an "incomplete" response (which would re-open PR 69580). */
556+
conn_ctx->response_eos_seen = 1;
557+
}
552558
}
553559
ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, parser->c,
554560
APLOGNO(03197) "h2_c2(%s): passed response %d",
@@ -757,6 +763,10 @@ apr_status_t h2_c2_filter_response_out(ap_filter_t *f, apr_bucket_brigade *bb)
757763
}
758764

759765
if (r->header_only || AP_STATUS_IS_HEADER_ONLY(r->status)) {
766+
/* A header-only response (HEAD, or 204/304) is complete without a body
767+
* EOS; mark it complete so c2_process() does not abort/reset it as an
768+
* "incomplete" response (which would re-open PR 69580). */
769+
conn_ctx->response_eos_seen = 1;
760770
ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, f->c,
761771
"h2_c2(%s): headers only, cleanup output brigade", conn_ctx->id);
762772
b = body_bucket? body_bucket : APR_BRIGADE_FIRST(bb);
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
#!/usr/bin/env python3
2+
# Start a response (status + headers + a little body), then go silent for
3+
# longer than the server's `Timeout`. Over HTTP/2 this makes Apache's content
4+
# filter time out reading from the CGI (AH01220): the response was started but
5+
# never completed (no EOS). A correct server RST_STREAMs the stream; a buggy
6+
# one leaves the client hanging. Used by test_105_timeout.py.
7+
import sys
8+
import time
9+
10+
sys.stdout.write("Content-Type: application/octet-stream\r\n\r\n")
11+
sys.stdout.write("X" * 16384)
12+
sys.stdout.flush()
13+
time.sleep(30)

test/modules/http2/test_105_timeout.py

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,55 @@ def test_h2_105_12(self, env):
166166
assert piper.response, f'{piper}'
167167
assert piper.response['status'] == 408, f"{piper.response}"
168168

169-
# A body-less response that is missing an EOS
169+
# A CGI that starts a response (status + headers + some body) and then goes
170+
# silent past `Timeout` must lead to the HTTP/2 stream being RST_STREAM'd,
171+
# not leave the client hanging. Regression test for the mod_http2 hang where
172+
# a c2 that finished an *incomplete* response (no EOS, e.g. CGI timed out
173+
# mid-body) was treated as complete and no reset was ever sent.
174+
def test_h2_105_20(self, env):
175+
conf = H2Conf(env)
176+
conf.add("Timeout 1")
177+
conf.add("PassEnv PATH") # let the CGI find its interpreter
178+
conf.add_vhost_cgi()
179+
conf.install()
180+
assert env.apache_restart() == 0
181+
url = env.mkurl("https", "cgi", "/h2cgi_slow.py")
182+
# Open many streams at once on ONE h2 connection. A correct server
183+
# RST_STREAMs every silent CGI, so the whole batch finishes in ~Timeout.
184+
# A buggy server fails to reset at least one of them (the missed reset is
185+
# racy per-stream, but with many streams in flight it is reliably hit),
186+
# so curl --parallel waits until its --max-time. Key on elapsed time, not
187+
# exit code: --parallel makes the exit code ambiguous, and a hang here is
188+
# a multi-second stall versus a sub-second clean teardown. --max-time
189+
# also bounds the client so a hang cannot wedge the test.
190+
#
191+
# Why 10 streams: a single silent stream hangs only racily (about 80%),
192+
# and the racy/deterministic knee is sharp. A sweep on a fast idle box
193+
# measured 80% hang at 1 to 2 streams but 100% at 3 or more (1 and 2 are
194+
# equally racy because the streams are correlated under one session
195+
# connection, so it is a threshold, not an independent-probability
196+
# curve). The knee rises on faster or less-loaded machines (the server
197+
# wins the per-stream reset race more often), so 10 keeps a wide margin
198+
# for CI hardware variance. The streams are concurrent, so the larger
199+
# count costs no wall-clock, and 10 is well under the default 100 max
200+
# concurrent streams.
201+
count = 10
202+
max_time = 10
203+
r = env.curl_raw([url] * count, options=[
204+
"--parallel", "--parallel-immediate", "--max-time", str(max_time)])
205+
assert r.duration < timedelta(seconds=max_time - 2), \
206+
f'streams hung waiting for RST_STREAM (batch took {r.duration}): {r}'
207+
# Resetting each silent CGI is logged as a CGI timeout (cgid) and the
208+
# failed body read that follows it (core); both are expected here.
209+
time.sleep(1) # let the log flush
210+
env.httpd_error_log.ignore_recent(
211+
lognos = [
212+
"AH01220", # cgid: Timeout waiting for output from CGI script
213+
"AH00574", # core: ap_content_length_filter, apr_bucket_read() failed
214+
]
215+
)
216+
217+
# Complement to test_h2_105_20: a body-less response that is missing an EOS
170218
# must NOT be reset over HTTP/2. mod_cache revalidation of a cached,
171219
# immediately-stale resource emits exactly such a 304 (EOR + Flush, no EOS).
172220
# This guards the incomplete-response reset against re-opening PR 69580:

0 commit comments

Comments
 (0)