Skip to content

Commit ac83e94

Browse files
authored
[CP] Fix double deref in connection pool error path (#5597) (#5601)
## Description When creating a connection pool, if a `QuicConnStart` fails: - the connection was marked as `ExternalOwner` to prevent it from sending notification to the app - but this also mean that the closing logic will take care of releasing the owner refcount, since the application is not the owner yet - the connection was closed using `MsQuicConnectionClose`, which release the refcount of the application This caused a double release, triggering an assertion. We should not call APIs from internal call (it makes logging confusing and breaks some assumptions), so queue the connection close manually instead. Fixes #5550. ## Testing C/I. Need to consider if there is a simple way to deterministically test the connection pool failure paths.
1 parent f11bf0e commit ac83e94

File tree

2 files changed

+54
-9
lines changed

2 files changed

+54
-9
lines changed

src/core/connection.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -559,6 +559,8 @@ typedef struct QUIC_CONNECTION {
559559
QUIC_OPERATION BackUpOper;
560560
QUIC_API_CONTEXT BackupApiContext;
561561
uint16_t BackUpOperUsed;
562+
QUIC_OPERATION CloseOper;
563+
QUIC_API_CONTEXT CloseApiContext;
562564

563565
//
564566
// The status code used for indicating transport closed notifications.

src/core/connection_pool.c

Lines changed: 52 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,40 @@ QuicConnPoolGetInterfaceIndexForLocalAddress(
219219
return Status;
220220
}
221221

222+
//
223+
// Queue a close operation on the connection worker thread,
224+
// optionally waiting for its completion.
225+
//
226+
static
227+
_IRQL_requires_max_(PASSIVE_LEVEL)
228+
void
229+
QuicConnPoolQueueConnectionClose(
230+
_In_ QUIC_CONNECTION* Connection,
231+
_In_ const BOOLEAN WaitForCompletion
232+
)
233+
{
234+
CXPLAT_EVENT CompletionEvent = {0};
235+
236+
Connection->CloseOper.Type = QUIC_OPER_TYPE_API_CALL;
237+
Connection->CloseOper.FreeAfterProcess = FALSE;
238+
Connection->CloseOper.API_CALL.Context = &Connection->CloseApiContext;
239+
Connection->CloseApiContext.Type = QUIC_API_TYPE_CONN_CLOSE;
240+
Connection->CloseApiContext.Status = NULL;
241+
242+
if (WaitForCompletion) {
243+
CxPlatEventInitialize(&CompletionEvent, TRUE, FALSE);
244+
Connection->CloseApiContext.Completed = &CompletionEvent;
245+
}
246+
247+
QuicConnQueueOper(Connection, &Connection->CloseOper);
248+
249+
if (WaitForCompletion) {
250+
CxPlatEventWaitForever(CompletionEvent);
251+
CxPlatEventUninitialize(CompletionEvent);
252+
}
253+
}
254+
255+
222256
static
223257
_IRQL_requires_max_(PASSIVE_LEVEL)
224258
QUIC_STATUS
@@ -342,11 +376,12 @@ QuicConnPoolTryCreateConnection(
342376

343377
if (QUIC_FAILED(Status) && *Connection != NULL) {
344378
//
345-
// This connection has never left MsQuic back to the application,
346-
// so don't send any notifications to the application on close.
379+
// This connection has never left MsQuic back to the application.
380+
// Mark it as internally owned so no notification is sent to the app,
381+
// the closing logic will handle the final deref.
347382
//
348383
(*Connection)->State.ExternalOwner = FALSE;
349-
MsQuicConnectionClose((HQUIC)*Connection);
384+
QuicConnPoolQueueConnectionClose(*Connection, FALSE);
350385
*Connection = NULL;
351386
}
352387

@@ -656,13 +691,21 @@ MsQuicConnectionPoolCreate(
656691
}
657692

658693
CleanUpConnections:
659-
if (QUIC_FAILED(Status) &&
660-
(Config->Flags & QUIC_CONNECTION_POOL_FLAG_CLOSE_ON_FAILURE) != 0) {
661-
for (uint32_t i = 0; i < CreatedConnections; i++) {
662-
MsQuicConnectionClose((HQUIC)Connections[i]);
663-
Connections[i] = NULL;
694+
if (QUIC_FAILED(Status) &&
695+
(Config->Flags & QUIC_CONNECTION_POOL_FLAG_CLOSE_ON_FAILURE) != 0) {
696+
697+
//
698+
// Close every connection that was created.
699+
// The application will receive the shutdown notification.
700+
// Wait for the task to complete so that when this function returns to the app,
701+
// all connections are already closed (since the shutdown notification is visible).
702+
//
703+
for (uint32_t i = 0; i < CreatedConnections; i++) {
704+
QuicConnPoolQueueConnectionClose(Connections[i], TRUE);
705+
QuicConnRelease(Connections[i], QUIC_CONN_REF_HANDLE_OWNER);
706+
Connections[i] = NULL;
707+
}
664708
}
665-
}
666709

667710
Error:
668711
if (ServerNameCopy != NULL) {

0 commit comments

Comments
 (0)