From ddd57b5b1ee8daac72a3736bc7b998fccd4986a0 Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Tue, 23 Sep 2025 05:40:06 -0400 Subject: [PATCH] Close small race condition on error raising in QUIC MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Github issue #28501 reported an odd condition in which a double free was occuring when a given thread was popping entries of its error stack. It was hypothesized that, because a few places in the quic stack save error state to a shared structure (ch->err_state, port->error_state, qtls->error_state), that multiple threads may attempt to mutate the shared structure during error save/restore in parallel. Investigation showed that all paths which led to such mutations were done under lock, so that shouldn't occur. Except for one case, which this PR addresses. In ossl_quic_conn_stream_conclude, we unlock our protecting mutex, prior to calling QUIC_RAISE_NON_NORMAL_ERROR. If that function is called with an reason code of SHUTDOWN, it attempts to restore the channel error state. Given that the lock was released first, this creates a small race condition in which two threads may manipulate the shared error state in the channel struct in parallel. According to the reporter, applying this patch prevents the reported error from occuring again. Reviewed-by: Saša Nedvědický Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/28642) (cherry picked from commit 1e70e8080a5b8766cb5f8b81127f90db9081e856) --- ssl/quic/quic_impl.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c index c44e6b33c2a..cec05d5bd37 100644 --- a/ssl/quic/quic_impl.c +++ b/ssl/quic/quic_impl.c @@ -3197,6 +3197,7 @@ int ossl_quic_conn_stream_conclude(SSL *s) QCTX ctx; QUIC_STREAM *qs; int err; + int ret; if (!expect_quic_with_stream_lock(s, /*remote_init=*/0, /*io=*/0, &ctx)) return 0; @@ -3204,13 +3205,15 @@ int ossl_quic_conn_stream_conclude(SSL *s) qs = ctx.xso->stream; if (!quic_mutation_allowed(ctx.qc, /*req_active=*/1)) { + ret = QUIC_RAISE_NON_NORMAL_ERROR(&ctx, SSL_R_PROTOCOL_IS_SHUTDOWN, NULL); qctx_unlock(&ctx); - return QUIC_RAISE_NON_NORMAL_ERROR(&ctx, SSL_R_PROTOCOL_IS_SHUTDOWN, NULL); + return ret; } if (!quic_validate_for_write(ctx.xso, &err)) { + ret = QUIC_RAISE_NON_NORMAL_ERROR(&ctx, err, NULL); qctx_unlock(&ctx); - return QUIC_RAISE_NON_NORMAL_ERROR(&ctx, err, NULL); + return ret; } if (ossl_quic_sstream_get_final_size(qs->sstream, NULL)) { -- 2.47.3