From 92b5ba86f68b200c0f2549ec21a5edaaf2da4735 Mon Sep 17 00:00:00 2001 From: Joshua Rogers Date: Sun, 12 Oct 2025 21:30:50 +0800 Subject: [PATCH] quic: remove redundant free of inner TLS in accept_connection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SSL_free(conn_ssl) for a QCSO enters ossl_quic_free, which calls qc_cleanup. qc_cleanup already frees qc->tls via SSL_free(qc->tls) and then frees qc->ch. The additional SSL_free(ossl_quic_channel_get0_tls(new_ch)) releases the same TLS a second time, which is redundant. We also replace some of the pure condition checks with ossl_assert() checks as these conditions cannot really fail. Signed-off-by: Joshua Rogers Reviewed-by: Saša Nedvědický Reviewed-by: Tim Hudson Reviewed-by: Tomas Mraz MergeDate: Mon Jan 12 18:54:07 2026 (Merged from https://github.com/openssl/openssl/pull/28920) --- ssl/quic/quic_impl.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c index 46a34a1063..04004980db 100644 --- a/ssl/quic/quic_impl.c +++ b/ssl/quic/quic_impl.c @@ -4785,25 +4785,34 @@ SSL *ossl_quic_accept_connection(SSL *ssl, uint64_t flags) * created channel, so once we pop the new channel from the port above * we just need to extract it */ - if (new_ch == NULL - || (conn_ssl = ossl_quic_channel_get0_tls(new_ch)) == NULL - || (conn = SSL_CONNECTION_FROM_SSL(conn_ssl)) == NULL - || (conn_ssl = SSL_CONNECTION_GET_USER_SSL(conn)) == NULL) + if (new_ch == NULL) goto out; + + /* + * All objects below must exist, because new_ch != NULL. The objects are + * bound to new_ch. If channel constructor fails to create any item here + * it just fails to create channel. + */ + if (!ossl_assert((conn_ssl = ossl_quic_channel_get0_tls(new_ch)) != NULL) + || !ossl_assert((conn = SSL_CONNECTION_FROM_SSL(conn_ssl)) != NULL) + || !ossl_assert((conn_ssl = SSL_CONNECTION_GET_USER_SSL(conn)) != NULL)) + goto out; + qc = (QUIC_CONNECTION *)conn_ssl; - qc->listener = ctx.ql; qc->pending = 0; if (!SSL_up_ref(&ctx.ql->obj.ssl)) { + /* + * You might expect ossl_quic_channel_free() to be called here. Be + * assured it happens, The process goes as follows: + * - The SSL_free() here is being handled by ossl_quic_free(). + * - The very last step of ossl_quic_free() is call to qc_cleanup() + * where channel gets freed. + */ SSL_free(conn_ssl); - SSL_free(ossl_quic_channel_get0_tls(new_ch)); - conn_ssl = NULL; } + qc->listener = ctx.ql; out: - if (conn_ssl == NULL && new_ch != NULL) { - ossl_quic_channel_free(new_ch); - new_ch = NULL; - } qctx_unlock(&ctx); return conn_ssl;