Close small race condition on error raising in QUIC

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ý <sashan@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/28642)

(cherry picked from commit 1e70e8080a5b8766cb5f8b81127f90db9081e856)

diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c
index c44e6b33c2a8647dae28bd31b33a6cbbbece172b..cec05d5bd37b2587af24a35eaec1a0e471cd120a 100644 (file)
--- 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)) {