Skip to content

Commit f93efb7

Browse files
Copilotguhetier
andauthored
Fix QuicOperationFree using CXPLAT_FREE on pool-allocated QUIC_RECV_CHUNK (#5881)
## Description Fixes #5841 `QuicOperationFree` was calling `CXPLAT_FREE` on `QUIC_RECV_CHUNK` entries left in a `STRM_PROVIDE_RECV_BUFFERS` operation's cleanup path. These chunks are allocated via `CxPlatPoolAlloc`, which returns an interior pointer (`malloc_ptr + sizeof(CXPLAT_POOL_HEADER)`) — passing that to `free()` is invalid and detected by ASAN as a bad free. **Changes:** - **`src/core/recv_buffer.h`**: Expose `QuicRecvChunkFree()` declaration so it can be called outside `recv_buffer.c`. - **`src/core/operation.c`**: Replace `CXPLAT_FREE(..., QUIC_POOL_RECVBUF)` with `QuicRecvChunkFree()` in the `STRM_PROVIDE_RECV_BUFFERS` cleanup loop. `QuicRecvChunkFree` already handles both pool-allocated (`CxPlatPoolFree`) and directly-allocated (`CXPLAT_FREE`) chunks via the `AllocatedFromPool` flag. ## Testing No existing tests cover this specific cleanup path. The issue includes a repro that triggers the bad free under ASAN by providing buffers whose total length overflows `UINT32_MAX`, causing `QuicRecvBufferProvideChunks` to return early without consuming the chunks, leaving them to be freed by `QuicOperationFree`. ## Documentation No documentation impact. <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> ---- *This section details on the original issue you should resolve* <issue_title>`QuicOperationFree` uses CXPLAT_FREE for pool-allocated `QUIC_RECV_CHUNK` in `STRM_PROVIDE_RECV_BUFFERS` cleanup</issue_title> <issue_description>### Describe the bug In `src/core/operation.c:122-130`, `QuicOperationFree` cleans up leftover `QUIC_RECV_CHUNK` entries from a `STRM_PROVIDE_RECV_BUFFERS` operation using `CXPLAT_FREE`: ```c } else if (ApiCtx->Type == QUIC_API_TYPE_STRM_PROVIDE_RECV_BUFFERS) { while (!CxPlatListIsEmpty(&ApiCtx->STRM_PROVIDE_RECV_BUFFERS.Chunks)) { CXPLAT_FREE( CXPLAT_CONTAINING_RECORD( CxPlatListRemoveHead(&ApiCtx->STRM_PROVIDE_RECV_BUFFERS.Chunks), QUIC_RECV_CHUNK, Link), QUIC_POOL_RECVBUF); } ``` These chunks are allocated via `CxPlatPoolAlloc` in `MsQuicStreamProvideReceiveBuffers` (`api.c:1514`) and initialized with `AllocatedFromPool = TRUE` (`api.c:1524`). `CxPlatPoolAlloc` returns an interior pointer (`malloc_ptr + sizeof(CXPLAT_POOL_HEADER)`), so calling `CXPLAT_FREE` → `free()` on that pointer is **invalid** — it frees an address that was not returned by `malloc`. The correct free function is `QuicRecvChunkFree()` (`recv_buffer.c:181-195`), which checks `Chunk->AllocatedFromPool` and calls `CxPlatPoolFree` for pool-allocated chunks or `CXPLAT_FREE` for directly-allocated ones. ### Affected OS - [x] Windows - [x] Linux - [ ] macOS - [ ] Other (specify below) ### Additional OS information _No response_ ### MsQuic version main ### Steps taken to reproduce bug ## Test code to verify the bug ``` // // Regression test for a bug in QuicOperationFree (operation.c:122-130) // where CXPLAT_FREE is used on pool-allocated QUIC_RECV_CHUNK entries in the // STRM_PROVIDE_RECV_BUFFERS operation cleanup. These chunks are allocated via // CxPlatPoolAlloc (which returns an interior pointer), so calling CXPLAT_FREE // (free()) on them is invalid. The correct function is QuicRecvChunkFree(). // // Trigger: provide buffers whose total length exceeds UINT32_MAX so that // QuicRecvBufferProvideChunks returns QUIC_STATUS_INVALID_PARAMETER without // consuming the chunks. The chunks remain in the operation and are cleaned up // by QuicOperationFree after QuicConnFatalError tears down the connection. // void QuicTestOperationFreeChunkCleanup( ) { MsQuicRegistration Registration(true); TEST_QUIC_SUCCEEDED(Registration.GetInitStatus()); MsQuicConfiguration ServerConfiguration( Registration, "MsQuicTest", MsQuicSettings().SetPeerBidiStreamCount(1), ServerSelfSignedCredConfig); TEST_QUIC_SUCCEEDED(ServerConfiguration.GetInitStatus()); MsQuicConfiguration ClientConfiguration( Registration, "MsQuicTest", MsQuicCredentialConfig()); TEST_QUIC_SUCCEEDED(ClientConfiguration.GetInitStatus()); CxPlatWatchdog Watchdog(5000); NthAllocFailTestContext RecvContext {}; MsQuicAutoAcceptListener Listener( Registration, ServerConfiguration, NthAllocFailTestContext::ConnCallback, &RecvContext); TEST_QUIC_SUCCEEDED(Listener.GetInitStatus()); TEST_QUIC_SUCCEEDED(Listener.Start("MsQuicTest")); QuicAddr ServerLocalAddr; TEST_QUIC_SUCCEEDED(Listener.GetLocalAddr(ServerLocalAddr)); MsQuicConnection Connection(Registration); TEST_QUIC_SUCCEEDED(Connection.GetInitStatus()); TEST_QUIC_SUCCEEDED(Connection.Start( ClientConfiguration, ServerLocalAddr.GetFamily(), QUIC_TEST_LOOPBACK_FOR_AF(ServerLocalAddr.GetFamily()), ServerLocalAddr.GetPort())); TEST_TRUE(Connection.HandshakeCompleteEvent.WaitTimeout(TestWaitTimeout)); MsQuicStream Stream( Connection, QUIC_STREAM_OPEN_FLAG_APP_OWNED_BUFFERS, CleanUpManual); TEST_QUIC_SUCCEEDED(Stream.GetInitStatus()); // // First call: provide a small buffer so the recv buffer is initialized // with a valid chunk and the worker processes it normally. // uint8_t SmallBuf[64]; QUIC_BUFFER InitBuffer[1] = { { sizeof(SmallBuf), SmallBuf } }; Stream.ProvideReceiveBuffers(1, InitBuffer); CxPlatSleep(50); // // Second call: provide buffers whose total AllocLength (when added to // existing buffer) overflows UINT32_MAX. QuicRecvBufferProvideChunks // returns QUIC_STATUS_INVALID_PARAMETER without consuming the chunks. // The chunks remain in the operation's Chunks list. // // After the failure, QuicConnFatalError aborts the connection. The // subsequent teardown calls QuicOperationFree, which uses CXPLAT_FREE // on the pool-allocated chunks — an invalid free detectable by ASAN. // uint8_t Buf1[64], Buf2[64]; QUIC_BUFFER OverflowBuffers[2] = { { UINT32_MAX, Buf1 }, { 1024, Buf2 } }; Stream.ProvideReceiveBuffers(2, OverflowBuffers); // // Wait for the worker to process the operation, hit the error, // c... </details> <!-- START COPILOT CODING AGENT SUFFIX --> - <!-- START COPILOT CODING AGENT TIPS --> --- 📱 Kick off Copilot coding agent tasks wherever you are with [GitHub Mobile](https://gh.io/cca-mobile-docs), available on iOS and Android. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: guhetier <15261469+guhetier@users.noreply.github.com> Co-authored-by: Guillaume Hetier <guhetier@microsoft.com>
1 parent 21e7483 commit f93efb7

3 files changed

Lines changed: 9 additions & 10 deletions

File tree

src/core/api.c

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1571,14 +1571,8 @@ MsQuicStreamProvideReceiveBuffers(
15711571
//
15721572
while (!CxPlatListIsEmpty(&ChunkList)) {
15731573
CXPLAT_DBG_ASSERT(Connection != NULL);
1574-
CxPlatPoolFree(
1574+
QuicRecvChunkFree(
15751575
CXPLAT_CONTAINING_RECORD(CxPlatListRemoveHead(&ChunkList), QUIC_RECV_CHUNK, Link));
1576-
CXPLAT_FREE(
1577-
CXPLAT_CONTAINING_RECORD(
1578-
CxPlatListRemoveHead(&ChunkList),
1579-
QUIC_RECV_CHUNK,
1580-
Link),
1581-
QUIC_POOL_RECVBUF);
15821576
}
15831577

15841578
QuicTraceEvent(

src/core/operation.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -121,12 +121,11 @@ QuicOperationFree(
121121
QuicStreamRelease(ApiCtx->STRM_RECV_SET_ENABLED.Stream, QUIC_STREAM_REF_OPERATION);
122122
} else if (ApiCtx->Type == QUIC_API_TYPE_STRM_PROVIDE_RECV_BUFFERS) {
123123
while (!CxPlatListIsEmpty(&ApiCtx->STRM_PROVIDE_RECV_BUFFERS.Chunks)) {
124-
CXPLAT_FREE(
124+
QuicRecvChunkFree(
125125
CXPLAT_CONTAINING_RECORD(
126126
CxPlatListRemoveHead(&ApiCtx->STRM_PROVIDE_RECV_BUFFERS.Chunks),
127127
QUIC_RECV_CHUNK,
128-
Link),
129-
QUIC_POOL_RECVBUF);
128+
Link));
130129
}
131130
QuicStreamRelease(ApiCtx->STRM_PROVIDE_RECV_BUFFERS.Stream, QUIC_STREAM_REF_OPERATION);
132131
}

src/core/recv_buffer.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@ QuicRecvChunkInitialize(
3939
_In_ BOOLEAN BufferAllocatedFromPool
4040
);
4141

42+
_IRQL_requires_max_(DISPATCH_LEVEL)
43+
void
44+
QuicRecvChunkFree(
45+
_In_ QUIC_RECV_CHUNK* Chunk
46+
);
47+
4248
typedef struct QUIC_RECV_BUFFER {
4349

4450
//

0 commit comments

Comments
 (0)