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)

diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c
index 09c010ebba8406f0d5d68ef255700fbf3092f226..e5f5eea7b45a6180bd44cc9c39c013a20ecb07d5 100644 (file)
--- a/ssl/quic/quic_impl.c
+++ b/ssl/quic/quic_impl.c
@@ -3241,6 +3241,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;
@@ -3248,13 +3249,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)) {