Skip to content

Fix memory leak in PerfServer DelayWorker initialization failure#5213

Merged
guhetier merged 3 commits into
mainfrom
copilot/fix-5206
Jul 10, 2025
Merged

Fix memory leak in PerfServer DelayWorker initialization failure#5213
guhetier merged 3 commits into
mainfrom
copilot/fix-5206

Conversation

Copilot AI commented Jul 1, 2025

Copy link
Copy Markdown
Contributor

Problem

In PerfServer::Init(), when DelayWorkers[i].Initialize() fails during the initialization loop, the function returns QUIC_STATUS_INTERNAL_ERROR without cleaning up the already allocated DelayWorkers array, causing a memory leak.

The problematic code:

DelayWorkers = new (std::nothrow) DelayWorker[ProcCount];
for (uint16_t i = 0; i < ProcCount; ++i) {
    if (!DelayWorkers[i].Initialize(this, i)) {
        WriteOutput("Failed to init delay workers.\n");
        return QUIC_STATUS_INTERNAL_ERROR;  // Memory leak here!
    }
}

Solution

This PR implements a comprehensive fix that addresses both the reported issue and additional edge cases:

  1. Allocation failure handling: Added null check after new (std::nothrow) to handle allocation failure gracefully
  2. Memory leak fix: Added proper cleanup in the initialization failure path:
    • Calls Shutdown() on all DelayWorkers (safe for both initialized and uninitialized workers)
    • Deletes the DelayWorkers array with delete[] DelayWorkers
    • Sets DelayWorkers = nullptr
  3. Pattern consistency: Cleanup logic exactly matches the existing destructor implementation
  4. Appropriate error codes: Returns QUIC_STATUS_OUT_OF_MEMORY for allocation failure, QUIC_STATUS_INTERNAL_ERROR for initialization failure

Changes Made

ProcCount = (uint16_t)CxPlatProcCount();
DelayWorkers = new (std::nothrow) DelayWorker[ProcCount];
+if (!DelayWorkers) {
+    WriteOutput("Failed to allocate delay workers.\n");
+    return QUIC_STATUS_OUT_OF_MEMORY;
+}
for (uint16_t i = 0; i < ProcCount; ++i) {
    if (!DelayWorkers[i].Initialize(this, i)) {
+        for (uint16_t j = 0; j < ProcCount; ++j) {
+            DelayWorkers[j].Shutdown();
+        }
+        delete[] DelayWorkers;
+        DelayWorkers = nullptr;
        WriteOutput("Failed to init delay workers.\n");
        return QUIC_STATUS_INTERNAL_ERROR;
    }
}

Verification

Created and tested a mock implementation that simulates the initialization failure scenario, confirming that:

  • Successfully initialized DelayWorkers are properly shut down
  • Uninitialized DelayWorkers are safely handled by Shutdown()
  • Memory is properly deallocated
  • No crashes or undefined behavior occurs

The fix is more comprehensive than the original suggestion as it also handles allocation failure and follows the existing destructor cleanup pattern for consistency.

Fixes #5206.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: guhetier <15261469+guhetier@users.noreply.github.com>
Copilot AI changed the title [WIP] Potential Memory Leak on DelayWorker Initialization Failure Fix memory leak in PerfServer DelayWorker initialization failure Jul 1, 2025
Copilot AI requested a review from guhetier July 1, 2025 16:20
@guhetier
guhetier marked this pull request as ready for review July 1, 2025 16:29
@guhetier
guhetier requested a review from a team as a code owner July 1, 2025 16:29
@codecov

codecov Bot commented Jul 1, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.64%. Comparing base (2623c07) to head (d519690).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5213      +/-   ##
==========================================
- Coverage   86.74%   85.64%   -1.11%     
==========================================
  Files          59       59              
  Lines       18330    18330              
==========================================
- Hits        15901    15698     -203     
- Misses       2429     2632     +203     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/perf/lib/PerfServer.cpp Outdated
Co-authored-by: csujedihy <6022514+csujedihy@users.noreply.github.com>
Copilot AI requested a review from csujedihy July 1, 2025 17:14
@guhetier
guhetier merged commit 01c7f3c into main Jul 10, 2025
493 of 500 checks passed
@guhetier
guhetier deleted the copilot/fix-5206 branch July 10, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Potential Memory Leak on DelayWorker Initialization Failure

4 participants