From ccf4365117ef1df09334d02a3ee9e3bb329b8360 Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Tue, 13 Jun 2023 14:36:43 +0000 Subject: *) mod_http2: fixed a bug that could lead to a crash in main connection output handling. This occured only when the last request on a HTTP/2 connection had been processed and the session decided to shut down. This could lead to an attempt to send a final GOAWAY while the previous write was still in progress. See PR 66646. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1910386 13f79535-47bb-0310-9956-ffa450edef68 --- modules/http2/h2_c1_io.c | 23 ++++++++++++++---- modules/http2/h2_c1_io.h | 3 ++- modules/http2/h2_session.c | 4 ---- modules/http2/h2_stream.c | 60 +++++++++++++++++++++++++++++++++++++++++++++- modules/http2/h2_stream.h | 15 ++++++++++++ modules/http2/h2_version.h | 4 ++-- 6 files changed, 97 insertions(+), 12 deletions(-) (limited to 'modules/http2') diff --git a/modules/http2/h2_c1_io.c b/modules/http2/h2_c1_io.c index ade8836635..c43fa1b801 100644 --- a/modules/http2/h2_c1_io.c +++ b/modules/http2/h2_c1_io.c @@ -260,9 +260,22 @@ static apr_status_t read_to_scratch(h2_c1_io *io, apr_bucket *b) static apr_status_t pass_output(h2_c1_io *io, int flush) { conn_rec *c = io->session->c1; - apr_off_t bblen; + apr_off_t bblen = 0; apr_status_t rv; - + + if (io->is_passing) { + /* recursive call, may be triggered by an H2EOS bucket + * being destroyed and triggering sending more data? */ + AP_DEBUG_ASSERT(0); + ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c, APLOGNO() + "h2_c1_io(%ld): recursive call of h2_c1_io_pass. " + "Denied to prevent output corruption. This " + "points to a bug in the HTTP/2 implementation.", + c->id); + return APR_EGENERAL; + } + io->is_passing = 1; + append_scratch(io); if (flush) { if (!APR_BUCKET_IS_FLUSH(APR_BRIGADE_LAST(io->output))) { @@ -271,13 +284,14 @@ static apr_status_t pass_output(h2_c1_io *io, int flush) } } if (APR_BRIGADE_EMPTY(io->output)) { - return APR_SUCCESS; + rv = APR_SUCCESS; + goto cleanup; } io->unflushed = !APR_BUCKET_IS_FLUSH(APR_BRIGADE_LAST(io->output)); apr_brigade_length(io->output, 0, &bblen); C1_IO_BB_LOG(c, 0, APLOG_TRACE2, "out", io->output); - + rv = ap_pass_brigade(c->output_filters, io->output); if (APR_SUCCESS != rv) goto cleanup; @@ -309,6 +323,7 @@ cleanup: c->id, (long)bblen); } apr_brigade_cleanup(io->output); + io->is_passing = 0; return rv; } diff --git a/modules/http2/h2_c1_io.h b/modules/http2/h2_c1_io.h index d891ffb44d..c4dac38e1e 100644 --- a/modules/http2/h2_c1_io.h +++ b/modules/http2/h2_c1_io.h @@ -44,7 +44,8 @@ typedef struct { apr_off_t buffered_len; apr_off_t flush_threshold; unsigned int is_flushed : 1; - + unsigned int is_passing : 1; + char *scratch; apr_size_t ssize; apr_size_t slen; diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c index 1f37a36bf7..603e47ce85 100644 --- a/modules/http2/h2_session.c +++ b/modules/http2/h2_session.c @@ -1642,10 +1642,6 @@ static void on_stream_state_enter(void *ctx, h2_stream *stream) h2_mplx_c1_stream_cleanup(session->mplx, stream, &session->open_streams); ++session->streams_done; update_child_status(session, SERVER_BUSY_WRITE, "done", stream); - if (session->open_streams == 0) { - h2_session_dispatch_event(session, H2_SESSION_EV_NO_MORE_STREAMS, - 0, "stream done"); - } break; default: break; diff --git a/modules/http2/h2_stream.c b/modules/http2/h2_stream.c index d8b582ac40..635caf7cb3 100644 --- a/modules/http2/h2_stream.c +++ b/modules/http2/h2_stream.c @@ -166,6 +166,7 @@ static int on_frame_recv(h2_stream_state_t state, int frame_type) static int on_event(h2_stream* stream, h2_stream_event_t ev) { + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); if (stream->monitor && stream->monitor->on_event) { stream->monitor->on_event(stream->monitor->ctx, stream, ev); } @@ -392,6 +393,7 @@ void h2_stream_dispatch(h2_stream *stream, h2_stream_event_t ev) { int new_state; + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, stream->session->c1, H2_STRM_MSG(stream, "dispatch event %d"), ev); new_state = on_event(stream, ev); @@ -425,6 +427,7 @@ apr_status_t h2_stream_send_frame(h2_stream *stream, int ftype, int flags, size_ apr_status_t status = APR_SUCCESS; int new_state, eos = 0; + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); new_state = on_frame_send(stream->state, ftype); if (new_state < 0) { ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, stream->session->c1, @@ -474,6 +477,7 @@ apr_status_t h2_stream_recv_frame(h2_stream *stream, int ftype, int flags, size_ apr_status_t status = APR_SUCCESS; int new_state, eos = 0; + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); new_state = on_frame_recv(stream->state, ftype); if (new_state < 0) { ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, stream->session->c1, @@ -528,6 +532,7 @@ apr_status_t h2_stream_recv_DATA(h2_stream *stream, uint8_t flags, h2_session *session = stream->session; apr_status_t status = APR_SUCCESS; + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); stream->in_data_frames++; if (len > 0) { if (APLOGctrace3(session->c1)) { @@ -548,11 +553,38 @@ apr_status_t h2_stream_recv_DATA(h2_stream *stream, uint8_t flags, return status; } +#ifdef AP_DEBUG +static apr_status_t stream_pool_destroy(void *data) +{ + h2_stream *stream = data; + switch (stream->magic) { + case H2_STRM_MAGIC_OK: + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, stream->session->c1, + H2_STRM_MSG(stream, "was not destroyed explicitly")); + AP_DEBUG_ASSERT(0); + break; + case H2_STRM_MAGIC_SDEL: + /* stream has been explicitly destroyed, as it should */ + H2_STRM_ASSIGN_MAGIC(stream, H2_STRM_MAGIC_PDEL); + break; + case H2_STRM_MAGIC_PDEL: + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, stream->session->c1, + H2_STRM_MSG(stream, "already pool destroyed")); + AP_DEBUG_ASSERT(0); + break; + default: + AP_DEBUG_ASSERT(0); + } + return APR_SUCCESS; +} +#endif + h2_stream *h2_stream_create(int id, apr_pool_t *pool, h2_session *session, h2_stream_monitor *monitor, int initiated_on) { h2_stream *stream = apr_pcalloc(pool, sizeof(h2_stream)); - + + H2_STRM_ASSIGN_MAGIC(stream, H2_STRM_MAGIC_OK); stream->id = id; stream->initiated_on = initiated_on; stream->created = apr_time_now(); @@ -560,6 +592,12 @@ h2_stream *h2_stream_create(int id, apr_pool_t *pool, h2_session *session, stream->pool = pool; stream->session = session; stream->monitor = monitor; +#ifdef AP_DEBUG + if (id) { /* stream 0 has special lifetime */ + apr_pool_cleanup_register(pool, stream, stream_pool_destroy, + apr_pool_cleanup_null); + } +#endif #ifdef H2_NG2_LOCAL_WIN_SIZE if (id) { @@ -581,6 +619,7 @@ void h2_stream_cleanup(h2_stream *stream) * end of the in/out notifications get closed. */ ap_assert(stream); + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); if (stream->out_buffer) { apr_brigade_cleanup(stream->out_buffer); } @@ -589,13 +628,16 @@ void h2_stream_cleanup(h2_stream *stream) void h2_stream_destroy(h2_stream *stream) { ap_assert(stream); + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); ap_log_cerror(APLOG_MARK, APLOG_TRACE3, 0, stream->session->c1, H2_STRM_MSG(stream, "destroy")); + H2_STRM_ASSIGN_MAGIC(stream, H2_STRM_MAGIC_SDEL); apr_pool_destroy(stream->pool); } void h2_stream_rst(h2_stream *stream, int error_code) { + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); stream->rst_error = error_code; if (stream->c2) { h2_c2_abort(stream->c2, stream->session->c1); @@ -611,6 +653,7 @@ apr_status_t h2_stream_set_request_rec(h2_stream *stream, h2_request *req; apr_status_t status; + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); ap_assert(stream->request == NULL); ap_assert(stream->rtmp == NULL); if (stream->rst_error) { @@ -632,6 +675,7 @@ apr_status_t h2_stream_set_request_rec(h2_stream *stream, void h2_stream_set_request(h2_stream *stream, const h2_request *r) { + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); ap_assert(stream->request == NULL); ap_assert(stream->rtmp == NULL); stream->rtmp = h2_request_clone(stream->pool, r); @@ -691,6 +735,7 @@ apr_status_t h2_stream_add_header(h2_stream *stream, int error = 0, was_added = 0; apr_status_t status = APR_SUCCESS; + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); if (stream->response) { return APR_EINVAL; } @@ -797,6 +842,7 @@ apr_status_t h2_stream_end_headers(h2_stream *stream, int eos, size_t raw_bytes) int is_http_or_https; h2_request *req = stream->rtmp; + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); status = h2_request_end_headers(req, stream->pool, raw_bytes); if (APR_SUCCESS != status || req->http_status != H2_HTTP_STATUS_UNSET) { goto cleanup; @@ -1045,6 +1091,7 @@ apr_status_t h2_stream_read_to(h2_stream *stream, apr_bucket_brigade *bb, { apr_status_t rv = APR_SUCCESS; + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); if (stream->rst_error) { return APR_ECONNRESET; } @@ -1139,6 +1186,7 @@ apr_status_t h2_stream_submit_pushes(h2_stream *stream, h2_headers *response) apr_array_header_t *pushes; int i; + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); pushes = h2_push_collect_update(stream, stream->request, response); if (pushes && !apr_is_empty_array(pushes)) { ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, stream->session->c1, @@ -1158,6 +1206,7 @@ apr_status_t h2_stream_submit_pushes(h2_stream *stream, h2_headers *response) apr_table_t *h2_stream_get_trailers(h2_stream *stream) { + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); return NULL; } @@ -1169,6 +1218,7 @@ const h2_priority *h2_stream_get_priority(h2_stream *stream, h2_headers *response) #endif { + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); if (response && stream->initiated_on) { const char *ctype = apr_table_get(response->headers, "content-type"); if (ctype) { @@ -1182,6 +1232,7 @@ const h2_priority *h2_stream_get_priority(h2_stream *stream, int h2_stream_is_ready(h2_stream *stream) { /* Have we sent a response or do we have the response in our buffer? */ + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); if (stream->response) { return 1; } @@ -1193,11 +1244,13 @@ int h2_stream_is_ready(h2_stream *stream) int h2_stream_is_at(const h2_stream *stream, h2_stream_state_t state) { + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); return stream->state == state; } int h2_stream_is_at_or_past(const h2_stream *stream, h2_stream_state_t state) { + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); switch (state) { case H2_SS_IDLE: return 1; /* by definition */ @@ -1220,6 +1273,7 @@ apr_status_t h2_stream_in_consumed(h2_stream *stream, apr_off_t amount) { h2_session *session = stream->session; + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); if (amount > 0) { apr_off_t consumed = amount; @@ -1345,6 +1399,7 @@ static ssize_t stream_data_cb(nghttp2_session *ng2s, H2_SSSN_STRM_MSG(session, stream_id, "data_cb, stream not found")); return NGHTTP2_ERR_CALLBACK_FAILURE; } + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); if (!stream->output || !stream->response || !stream->out_buffer) { return NGHTTP2_ERR_DEFERRED; } @@ -1476,6 +1531,7 @@ static apr_status_t stream_do_response(h2_stream *stream) #endif nghttp2_data_provider provider, *pprovider = NULL; + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); ap_assert(!stream->response); ap_assert(stream->out_buffer); @@ -1666,6 +1722,7 @@ void h2_stream_on_output_change(h2_stream *stream) /* stream->pout_recv_write signalled a change. Check what has happend, read * from it and act on seeing a response/data. */ + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); if (!stream->output) { /* c2 has not assigned the output beam to the stream (yet). */ ap_log_cerror(APLOG_MARK, APLOG_WARNING, 0, c1, @@ -1714,6 +1771,7 @@ void h2_stream_on_output_change(h2_stream *stream) void h2_stream_on_input_change(h2_stream *stream) { + H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK); ap_assert(stream->input); h2_beam_report_consumption(stream->input); if (h2_stream_is_at(stream, H2_SS_CLOSED_L) diff --git a/modules/http2/h2_stream.h b/modules/http2/h2_stream.h index 695d56ac5e..638dbdac0d 100644 --- a/modules/http2/h2_stream.h +++ b/modules/http2/h2_stream.h @@ -63,7 +63,22 @@ typedef struct h2_stream_monitor { trigger a state change */ } h2_stream_monitor; +#ifdef AP_DEBUG +#define H2_STRM_MAGIC_OK 0x5354524d +#define H2_STRM_MAGIC_SDEL 0x5344454c +#define H2_STRM_MAGIC_PDEL 0x5044454c + +#define H2_STRM_ASSIGN_MAGIC(s,m) ((s)->magic = m) +#define H2_STRM_ASSERT_MAGIC(s,m) ap_assert((s)->magic == m) +#else +#define H2_STRM_ASSIGN_MAGIC(s,m) ((void)0) +#define H2_STRM_ASSERT_MAGIC(s,m) ((void)0) +#endif + struct h2_stream { +#ifdef AP_DEBUG + uint32_t magic; +#endif int id; /* http2 stream identifier */ int initiated_on; /* initiating stream id (PUSH) or 0 */ apr_pool_t *pool; /* the memory pool for this stream */ diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h index aca4ffe0d5..7de3144ec7 100644 --- a/modules/http2/h2_version.h +++ b/modules/http2/h2_version.h @@ -27,7 +27,7 @@ * @macro * Version number of the http2 module as c string */ -#define MOD_HTTP2_VERSION "2.0.18-git" +#define MOD_HTTP2_VERSION "2.0.19-git" /** * @macro @@ -35,7 +35,7 @@ * release. This is a 24 bit number with 8 bits for major number, 8 bits * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203. */ -#define MOD_HTTP2_VERSION_NUM 0x020012 +#define MOD_HTTP2_VERSION_NUM 0x020013 #endif /* mod_h2_h2_version_h */ -- cgit v1.2.3