quic: make ch_cleanup() idempotent and simplify channel error path

ch_init() calls ch_cleanup() on its own failure, after which
port_make_channel() may still call ossl_quic_channel_free() (which calls
ch_cleanup() again). The second call double-freed fields such as
ch->qlog_title.

To handle this, ch_cleanup() now NULLs every owned pointer after its
free and clears the have_statm / have_qsm flags after their destructors,
making it safe to invoke twice on the same channel.

With ch_cleanup() idempotent, port_make_channel() no longer needs the
ch_cleaned flag and the bare OPENSSL_free(ch) branch: the error path
unconditionally calls ossl_quic_channel_free() regardless of whether
ch_init() succeeded, partially initialized the channel, or already ran
ch_cleanup() on itself.

Signed-off-by: Nikola Pajkovsky <nikolap@openssl.org>
Reviewed-by: Saša Nedvědický <sashan@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.foundation>
Reviewed-by: Matt Caswell <matt@openssl.foundation>
Reviewed-by: Norbert Pocs <norbertp@openssl.org>
MergeDate: Thu May 28 07:26:22 2026
(Merged from https://github.com/openssl/openssl/pull/31177)

diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c
index 4c299a53e5be8af5e9af343b296d51af307b2a8e..07258f1a9b30d67aee9b4dbaba5b4d668b69baaa 100644 (file)
--- a/ssl/quic/quic_channel.c
+++ b/ssl/quic/quic_channel.c
@@ -357,6 +357,11 @@ err:
     return 0;
 }
 
+/*
+ * ch_cleanup() is idempotent: every owned pointer is NULL'd after its free,
+ * and every "have_*" flag is reset after its destructor runs. Calling this
+ * twice on the same channel is safe.
+ */
 static void ch_cleanup(QUIC_CHANNEL *ch)
 {
     uint32_t pn_space;
@@ -374,33 +379,53 @@ static void ch_cleanup(QUIC_CHANNEL *ch)
         ossl_quic_srtm_cull(ch->srtm, ch);
 
     ossl_quic_tx_packetiser_free(ch->txp);
+    ch->txp = NULL;
     ossl_quic_txpim_free(ch->txpim);
+    ch->txpim = NULL;
     ossl_quic_cfq_free(ch->cfq);
+    ch->cfq = NULL;
     ossl_qtx_free(ch->qtx);
-    if (ch->cc_data != NULL)
+    ch->qtx = NULL;
+    if (ch->cc_data != NULL) {
         ch->cc_method->free(ch->cc_data);
-    if (ch->have_statm)
+        ch->cc_data = NULL;
+    }
+    if (ch->have_statm) {
         ossl_statm_destroy(&ch->statm);
+        ch->have_statm = 0;
+    }
     ossl_ackm_free(ch->ackm);
+    ch->ackm = NULL;
 
-    if (ch->have_qsm)
+    if (ch->have_qsm) {
         ossl_quic_stream_map_cleanup(&ch->qsm);
+        ch->have_qsm = 0;
+    }
 
     for (pn_space = QUIC_PN_SPACE_INITIAL; pn_space < QUIC_PN_SPACE_NUM; ++pn_space) {
         ossl_quic_sstream_free(ch->crypto_send[pn_space]);
+        ch->crypto_send[pn_space] = NULL;
         ossl_quic_rstream_free(ch->crypto_recv[pn_space]);
+        ch->crypto_recv[pn_space] = NULL;
     }
 
     ossl_qrx_pkt_release(ch->qrx_pkt);
     ch->qrx_pkt = NULL;
 
     ossl_quic_tls_free(ch->qtls);
+    ch->qtls = NULL;
     ossl_qrx_free(ch->qrx);
+    ch->qrx = NULL;
     OPENSSL_free(ch->local_transport_params);
+    ch->local_transport_params = NULL;
     OPENSSL_free((char *)ch->terminate_cause.reason);
+    ch->terminate_cause.reason = NULL;
     OSSL_ERR_STATE_free(ch->err_state);
+    ch->err_state = NULL;
     OPENSSL_free(ch->ack_range_scratch);
+    ch->ack_range_scratch = NULL;
     OPENSSL_free(ch->pending_new_token);
+    ch->pending_new_token = NULL;
 
     if (ch->on_port_list) {
         ossl_list_ch_remove(&ch->port->channel_list, ch);
@@ -412,7 +437,9 @@ static void ch_cleanup(QUIC_CHANNEL *ch)
         ossl_qlog_flush(ch->qlog); /* best effort */
 
     OPENSSL_free(ch->qlog_title);
+    ch->qlog_title = NULL;
     ossl_qlog_free(ch->qlog);
+    ch->qlog = NULL;
 #endif
 }
 

diff --git a/ssl/quic/quic_port.c b/ssl/quic/quic_port.c
index a1b134d9b163a08c90f716b56583e06fa7e34464..42b121103d788af9278846df2d0c8a82958f1fbd 100644 (file)
--- a/ssl/quic/quic_port.c
+++ b/ssl/quic/quic_port.c
@@ -588,7 +588,6 @@ static QUIC_CHANNEL *port_make_channel(QUIC_PORT *port, SSL *tls, OSSL_QRX *qrx,
     QUIC_CHANNEL_ARGS args = { 0 };
     QUIC_CHANNEL *ch;
     SSL *user_ssl = NULL;
-    int ch_cleaned = 0;
 
     args.port = port;
     args.is_server = is_server;
@@ -658,10 +657,8 @@ static QUIC_CHANNEL *port_make_channel(QUIC_PORT *port, SSL *tls, OSSL_QRX *qrx,
     /*
      * And finally init the channel struct
      */
-    if (!ossl_quic_channel_init(ch)) {
-        ch_cleaned = 1;
+    if (!ossl_quic_channel_init(ch))
         goto err;
-    }
 
     ossl_qtx_set_bio(ch->qtx, port->net_wbio);
     return ch;
@@ -670,11 +667,7 @@ err:
     if (user_ssl != NULL)
         ((QUIC_CONNECTION *)user_ssl)->ch = NULL;
 
-    if (ch_cleaned)
-        OPENSSL_free(ch);
-    else
-        ossl_quic_channel_free(ch);
-
+    ossl_quic_channel_free(ch);
     SSL_free(user_ssl);
 
     return NULL;

diff --git a/test/quic_memfail_test.c b/test/quic_memfail_test.c
index 8c0dfaca6b6908a04567dbdae72b79d048d1d4e1..6e5b1be176012163289583c87a11c73a28d761ee 100644 (file)
--- a/test/quic_memfail_test.c
+++ b/test/quic_memfail_test.c
@@ -20,6 +20,7 @@
 #include "internal/quic_ssl.h"
 #include "internal/ssl_unwrap.h"
 #include "../ssl/quic/quic_local.h"
+#include "../ssl/quic/quic_port_local.h"
 
 #include "testutil.h"
 
@@ -80,9 +81,67 @@ err:
     return ret;
 }
 
+static int test_ch_cleanup_idempotent(void)
+{
+    SSL_CTX *ctx = NULL;
+    SSL *listener = NULL;
+    QUIC_LISTENER *ql;
+    QUIC_CHANNEL_ARGS args = { 0 };
+    QUIC_CHANNEL *ch = NULL;
+    int alloc_failed = 0;
+    int ret = 0;
+    OSSL_LIB_CTX *lctx = NULL;
+
+    if (!TEST_ptr(lctx = OSSL_LIB_CTX_new()))
+        goto err;
+    ctx = SSL_CTX_new_ex(lctx, NULL, OSSL_QUIC_server_method());
+    if (!TEST_ptr(ctx))
+        goto err;
+
+    listener = SSL_new_listener(ctx, SSL_LISTENER_FLAG_NO_VALIDATE);
+    if (!TEST_ptr(listener))
+        goto err;
+    ql = QUIC_LISTENER_FROM_SSL(listener);
+
+    args.port = ql->port;
+    args.lcidm = ql->port->lcidm;
+    args.srtm = ql->port->srtm;
+    args.is_server = 1;
+    args.is_tserver_ch = 1;
+    args.use_qlog = 1;
+    args.qlog_title = "qlog";
+
+    MFAIL_start();
+    ch = ossl_quic_channel_alloc(&args);
+    if (ch == NULL) {
+        alloc_failed = 1;
+    } else {
+        if (!ossl_quic_channel_init(ch))
+            alloc_failed = 1;
+
+        /*
+         * Whether init succeeded or failed, ossl_quic_channel_free() runs
+         * ch_cleanup(). On the failure path that's the second ch_cleanup()
+         * for this channel and must not crash or double-free.
+         */
+        ossl_quic_channel_free(ch);
+        ch = NULL;
+    }
+    MFAIL_end();
+
+    ret = alloc_failed ? 0 : 1;
+
+err:
+    SSL_free(listener);
+    SSL_CTX_free(ctx);
+    OSSL_LIB_CTX_free(lctx);
+    return ret;
+}
+
 int setup_tests(void)
 {
     ADD_MFAIL_TEST(test_ossl_quic_port_create_incoming);
+    ADD_MFAIL_TEST(test_ch_cleanup_idempotent);
 
     return 1;
 }