[CP] Fix QuicOperationFree using CXPLAT_FREE on pool-allocated QUIC_RECV_CHUNK (#5881)#6085
Draft
guhetier wants to merge 2 commits into
Draft
[CP] Fix QuicOperationFree using CXPLAT_FREE on pool-allocated QUIC_RECV_CHUNK (#5881)#6085guhetier wants to merge 2 commits into
guhetier wants to merge 2 commits into
Conversation
…HUNK (#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>
On release/2.5, QuicRecvChunkFree takes (QUIC_RECV_BUFFER*, QUIC_RECV_CHUNK*) and discriminates pool-vs-direct allocation via Chunk->AppOwnedBuffer. The single-argument form (which uses Chunk->AllocatedFromPool) was introduced by PR #5079 ("Receive buffer simplification [4/4]"), which is on main but not on release/2.5. The upstream fix in #5881 used the post-#5079 one-argument form, which conflicts at link time with the existing two-argument definition in recv_buffer.c. The orphan chunks freed by the cleanup paths in MsQuicStreamProvideReceiveBuffers (api.c) and QuicOperationFree (operation.c) are always allocated from Connection->Partition->AppBufferChunkPool and initialized with AppOwnedBuffer = TRUE (api.c around line 1472, 1482). They are never assigned to any RecvBuffer, so calling CxPlatPoolFree(Chunk) directly is semantically identical to what release/2.5's QuicRecvChunkFree would do for them (recv_buffer.c:181-195). Changes: - recv_buffer.h: Drop the imported one-argument QuicRecvChunkFree declaration (it conflicts with the implementation and is not needed; QuicRecvChunkFree remains a file-local helper of recv_buffer.c). - operation.c: Use CxPlatPoolFree directly in the STRM_PROVIDE_RECV_BUFFERS cleanup loop. - api.c: Use CxPlatPoolFree directly in the error cleanup loop. Note: collapsing the per-iteration double CxPlatListRemoveHead pattern that existed in api.c's cleanup loop is part of the upstream #5881 fix itself (applied by the preceding cherry-pick commit), not of this adaptation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #5841
QuicOperationFreewas callingCXPLAT_FREEonQUIC_RECV_CHUNKentries left in aSTRM_PROVIDE_RECV_BUFFERSoperation's cleanup path. These chunks are allocated viaCxPlatPoolAlloc, which returns an interior pointer (malloc_ptr + sizeof(CXPLAT_POOL_HEADER)) — passing that tofree()is invalid and detected by ASAN as a bad free.Testing
CI.
Documentation
No documentation impact.