From bd29b80912e5db536ed5ec50c6c854fe2c77c585 Mon Sep 17 00:00:00 2001 From: Guillaume Hetier Date: Mon, 17 Nov 2025 17:37:58 -0800 Subject: [PATCH 1/4] Fix double deref in connection pool error path --- src/core/connection_pool.c | 58 ++++++++++++++++++++++++++++++++------ 1 file changed, 49 insertions(+), 9 deletions(-) diff --git a/src/core/connection_pool.c b/src/core/connection_pool.c index 07499eabaa..203f67e7f9 100644 --- a/src/core/connection_pool.c +++ b/src/core/connection_pool.c @@ -236,6 +236,39 @@ QuicConnPoolGetInterfaceIndexForLocalAddress( return Status; } +// +// Queue a close operation on the connection worker thread, +// optionally waiting for its completion. +// +static +_IRQL_requires_max_(PASSIVE_LEVEL) +void +QuicConnPoolQueueConnectionClose( + _In_ QUIC_CONNECTION* Connection, + _In_ const BOOLEAN WaitForCompletion + ) +{ + Connection->CloseOper.Type = QUIC_OPER_TYPE_API_CALL; + Connection->CloseOper.FreeAfterProcess = FALSE; + Connection->CloseOper.API_CALL.Context = &Connection->CloseApiContext; + Connection->CloseApiContext.Type = QUIC_API_TYPE_CONN_CLOSE; + Connection->CloseApiContext.Status = NULL; + + CXPLAT_EVENT CompletionEvent = {0}; + if (WaitForCompletion) { + CxPlatEventInitialize(&CompletionEvent, TRUE, FALSE); + Connection->CloseApiContext.Completed = &CompletionEvent; + } + + QuicConnQueueOper(Connection, &Connection->CloseOper); + + if (WaitForCompletion) { + CxPlatEventWaitForever(CompletionEvent); + CxPlatEventUninitialize(CompletionEvent); + } +} + + static _IRQL_requires_max_(PASSIVE_LEVEL) QUIC_STATUS @@ -359,11 +392,12 @@ QuicConnPoolTryCreateConnection( if (QUIC_FAILED(Status) && *Connection != NULL) { // - // This connection has never left MsQuic back to the application, - // so don't send any notifications to the application on close. + // This connection has never left MsQuic back to the application. + // Mark it as internally owned so no notification is sent to the app, + // the closing logic will handle the final deref. // (*Connection)->State.ExternalOwner = FALSE; - MsQuicConnectionClose((HQUIC)*Connection); + QuicConnPoolQueueConnectionClose(*Connection, FALSE); *Connection = NULL; } @@ -673,13 +707,19 @@ MsQuicConnectionPoolCreate( } CleanUpConnections: -if (QUIC_FAILED(Status) && - (Config->Flags & QUIC_CONNECTION_POOL_FLAG_CLOSE_ON_FAILURE) != 0) { - for (uint32_t i = 0; i < CreatedConnections; i++) { - MsQuicConnectionClose((HQUIC)Connections[i]); - Connections[i] = NULL; + if (QUIC_FAILED(Status) && + (Config->Flags & QUIC_CONNECTION_POOL_FLAG_CLOSE_ON_FAILURE) != 0) { + + // + // Close every connection that was created. + // The application will receive the shutdown notification. + // + for (uint32_t i = 0; i < CreatedConnections; i++) { + QuicConnPoolQueueConnectionClose(Connections[i], TRUE); + QuicConnRelease(Connections[i], QUIC_CONN_REF_HANDLE_OWNER); + Connections[i] = NULL; + } } -} Error: if (ServerNameCopy != NULL) { From 8b6a41267b64ffe0ea69963f7b7fb18d566bd2f5 Mon Sep 17 00:00:00 2001 From: Guillaume Hetier Date: Tue, 18 Nov 2025 10:20:07 -0800 Subject: [PATCH 2/4] Move variable decl to top of function --- src/core/connection_pool.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/connection_pool.c b/src/core/connection_pool.c index 203f67e7f9..e044f74d48 100644 --- a/src/core/connection_pool.c +++ b/src/core/connection_pool.c @@ -248,13 +248,14 @@ QuicConnPoolQueueConnectionClose( _In_ const BOOLEAN WaitForCompletion ) { + CXPLAT_EVENT CompletionEvent = {0}; + Connection->CloseOper.Type = QUIC_OPER_TYPE_API_CALL; Connection->CloseOper.FreeAfterProcess = FALSE; Connection->CloseOper.API_CALL.Context = &Connection->CloseApiContext; Connection->CloseApiContext.Type = QUIC_API_TYPE_CONN_CLOSE; Connection->CloseApiContext.Status = NULL; - CXPLAT_EVENT CompletionEvent = {0}; if (WaitForCompletion) { CxPlatEventInitialize(&CompletionEvent, TRUE, FALSE); Connection->CloseApiContext.Completed = &CompletionEvent; From 3f547f90339cbc8a70ee0c970a28bc1d1a5b534e Mon Sep 17 00:00:00 2001 From: Guillaume Hetier Date: Tue, 18 Nov 2025 10:44:49 -0800 Subject: [PATCH 3/4] Improve comment --- src/core/connection_pool.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/core/connection_pool.c b/src/core/connection_pool.c index e044f74d48..1504ef82ba 100644 --- a/src/core/connection_pool.c +++ b/src/core/connection_pool.c @@ -714,6 +714,8 @@ MsQuicConnectionPoolCreate( // // Close every connection that was created. // The application will receive the shutdown notification. + // Wait for the task to be complete so that when this function returns to the app, + // all connections are closed (since the shutdown notification is visible). // for (uint32_t i = 0; i < CreatedConnections; i++) { QuicConnPoolQueueConnectionClose(Connections[i], TRUE); From 5e8f7bb652729cc53126c58de3b60d9a3cd50f52 Mon Sep 17 00:00:00 2001 From: Guillaume Hetier Date: Tue, 18 Nov 2025 10:45:24 -0800 Subject: [PATCH 4/4] Fix typo --- src/core/connection_pool.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/connection_pool.c b/src/core/connection_pool.c index 1504ef82ba..5e35002db4 100644 --- a/src/core/connection_pool.c +++ b/src/core/connection_pool.c @@ -714,8 +714,8 @@ MsQuicConnectionPoolCreate( // // Close every connection that was created. // The application will receive the shutdown notification. - // Wait for the task to be complete so that when this function returns to the app, - // all connections are closed (since the shutdown notification is visible). + // Wait for the task to complete so that when this function returns to the app, + // all connections are already closed (since the shutdown notification is visible). // for (uint32_t i = 0; i < CreatedConnections; i++) { QuicConnPoolQueueConnectionClose(Connections[i], TRUE);