diff --git a/changes-entries/h3_connect_crash.txt b/changes-entries/h3_connect_crash.txt new file mode 100644 index 00000000000..e92df68df0d --- /dev/null +++ b/changes-entries/h3_connect_crash.txt @@ -0,0 +1,2 @@ + *) mod_http3: A CONNECT request with no :path no longer crashes the child + process. [Alexander Gerasimov ] diff --git a/changes-entries/h3_lifetime_fixes.txt b/changes-entries/h3_lifetime_fixes.txt new file mode 100644 index 00000000000..c5c8dc8f46f --- /dev/null +++ b/changes-entries/h3_lifetime_fixes.txt @@ -0,0 +1,4 @@ + *) mod_http3: Fix a write to a freed session when a request completes on + a closed connection, lost entries in the QUIC accept queues, and new + connections that stay allocated during a graceful stop. + [Alexander Gerasimov ] diff --git a/modules/http3/h3_request.c b/modules/http3/h3_request.c index d96bbbf8484..c88932b0432 100644 --- a/modules/http3/h3_request.c +++ b/modules/http3/h3_request.c @@ -290,11 +290,9 @@ static void* APR_THREAD_FUNC stream_worker(apr_thread_t* thd, void* data) r->header_only = (r->method_number == M_GET && r->method[0] == 'H'); h3s->r = r; - if (h3s->path) - { - ap_parse_uri(r, h3s->path); - r->the_request = apr_pstrcat(r->pool, r->method, " ", r->unparsed_uri, " ", r->protocol, NULL); - } + /* CONNECT has no :path; its target is the authority, as in HTTP/1.1. */ + ap_parse_uri(r, h3s->path ? h3s->path : h3s->authority ? h3s->authority : ""); + r->the_request = apr_pstrcat(r->pool, r->method, " ", r->unparsed_uri, " ", r->protocol, NULL); if (h3s->authority) { apr_table_setn(r->headers_in, "Host", apr_pstrdup(r->pool, h3s->authority)); @@ -421,8 +419,8 @@ static void* APR_THREAD_FUNC stream_worker(apr_thread_t* thd, void* data) apr_thread_mutex_lock(session->lock); h3s->worker_done = 1; apr_thread_mutex_unlock(session->lock); - apr_atomic_dec32(&session->active_tasks); h3_session_wake(session); /* the event thread reclaims the stream */ + apr_atomic_dec32(&session->active_tasks); /* last: at zero the event thread can free the session */ return NULL; } diff --git a/modules/http3/h3_threads.c b/modules/http3/h3_threads.c index 488ec9d6f5c..a9e80837e17 100644 --- a/modules/http3/h3_threads.c +++ b/modules/http3/h3_threads.c @@ -103,7 +103,7 @@ void* APR_THREAD_FUNC h3_event_thread(apr_thread_t* thread, void* data) } while (pumped > 0 && ++pump_rounds < H3_PUMP_BUDGET); } - if (io->thread_running && !io->draining) + if (io->thread_running) { for (;;) { @@ -112,6 +112,11 @@ void* APR_THREAD_FUNC h3_event_thread(apr_thread_t* thread, void* data) { break; } + if (io->draining) + { + h3q_conn_free(conn); /* no new connections during a graceful stop */ + continue; + } ap_log_error(APLOG_MARK, APLOG_INFO, 0, io->server, "accepted new QUIC connection"); if (h3_io_at_connection_limit(io)) { diff --git a/modules/http3/quic/h3q_conn.c b/modules/http3/quic/h3q_conn.c index b801e2e7bc8..6d95db74e1f 100644 --- a/modules/http3/quic/h3q_conn.c +++ b/modules/http3/quic/h3q_conn.c @@ -590,14 +590,14 @@ void h3q_conn_free(h3q_conn* conn) break; } } - for (h3q_conn** slot = &engine->accept_head; *slot; slot = &(*slot)->next_accept) + for (h3q_conn *prev = NULL, **slot = &engine->accept_head; *slot; prev = *slot, slot = &(*slot)->next_accept) { if (*slot == conn) { *slot = conn->next_accept; - if (!*slot) + if (engine->accept_tail == conn) { - engine->accept_tail = NULL; + engine->accept_tail = prev; } break; } diff --git a/modules/http3/quic/h3q_stream.c b/modules/http3/quic/h3q_stream.c index 2a374eda3c4..71e952cf2c7 100644 --- a/modules/http3/quic/h3q_stream.c +++ b/modules/http3/quic/h3q_stream.c @@ -241,14 +241,14 @@ void h3q_stream_free(h3q_stream* st) break; } } - for (h3q_stream** slot = &conn->accept_head; *slot; slot = &(*slot)->next_accept) + for (h3q_stream *prev = NULL, **slot = &conn->accept_head; *slot; prev = *slot, slot = &(*slot)->next_accept) { if (*slot == st) { *slot = st->next_accept; - if (!*slot) + if (conn->accept_tail == st) { - conn->accept_tail = NULL; + conn->accept_tail = prev; } break; } diff --git a/test/modules/http3/test_108_misc.py b/test/modules/http3/test_108_misc.py index 1c200ad25dc..843816c41fd 100644 --- a/test/modules/http3/test_108_misc.py +++ b/test/modules/http3/test_108_misc.py @@ -24,6 +24,8 @@ def _class_scope(cls, env): " SetOutputFilter INCLUDES", "", f"ProxyPass /proxy/ http://127.0.0.1:{env.http_port}/", + "", + "", ]).install() assert env.apache_restart() == 0 @@ -59,3 +61,10 @@ def test_h3_108_06_proxy_post(self, env): body=body) assert r.status == 200, f"{r}" assert r.body == body + + def test_h3_108_07_connect_no_path(self, env): + # A CONNECT has no :path. The location walk must not get a NULL URI. + r1, r2 = env.h3_client().requests([dict(method="CONNECT"), dict(path="/")], + sequential=True) + assert 400 <= r1.status < 500, f"{r1}" + assert r2.status == 200, f"{r2}" diff --git a/test/pyhttpd/h3client.py b/test/pyhttpd/h3client.py index 49bfaa48147..42560ddf0b2 100644 --- a/test/pyhttpd/h3client.py +++ b/test/pyhttpd/h3client.py @@ -118,6 +118,9 @@ def send_request(self, method, path, authority, headers, body, self.http.head_streams.add(sid) hdrs = [(b":method", method.encode()), (b":scheme", b"https"), (b":authority", authority.encode()), (b":path", path.encode())] + if method == "CONNECT": + # RFC 9114 4.4: no :scheme and no :path + hdrs = [hdrs[0], hdrs[2]] hdrs.extend((k.lower().encode(), v.encode()) for k, v in headers) self.http.send_headers(sid, hdrs, end_stream=body is None) if body is not None: