Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Oct 20, 2025

Problem

The QuicCryptoInitialize function in crypto.c was initializing Crypto->SparseAckRanges twice:

QuicRangeInitialize(
    QUIC_MAX_RANGE_ALLOC_SIZE,
    &Crypto->SparseAckRanges);  // First initialization (line 130-132)

Crypto->TlsState.BufferAllocLength = SendBufferLength;
Crypto->TlsState.Buffer = CXPLAT_ALLOC_NONPAGED(SendBufferLength, QUIC_POOL_TLS_BUFFER);
if (Crypto->TlsState.Buffer == NULL) {
    QuicTraceEvent(...);
    Status = QUIC_STATUS_OUT_OF_MEMORY;
    goto Exit;
}

QuicRangeInitialize(
    QUIC_MAX_RANGE_ALLOC_SIZE,
    &Crypto->SparseAckRanges);  // Redundant second initialization (line 146-148)

Analysis

The second call to QuicRangeInitialize is completely redundant because:

  1. No usage between calls: Nothing uses or modifies SparseAckRanges between the two initialization calls - only TLS buffer allocation occurs
  2. Idempotent operation: QuicRangeInitialize simply sets structure fields to initial values (UsedLength=0, AllocLength=8, SubRanges=PreAllocSubRanges, MaxAllocSize=parameter)
  3. No cleanup required: The error path doesn't clean up SparseAckRanges because QuicRangeInitialize doesn't allocate dynamic memory - it just points to the pre-allocated PreAllocSubRanges array
  4. No side effects: Re-initializing just resets the same fields to the same values

Solution

Removed the redundant second call to QuicRangeInitialize at lines 146-148.

Impact

This is a safe, minimal change with no behavioral impact:

  • No regression risk - the second call was purely redundant
  • Cleaner, more maintainable code
  • Slight performance improvement by avoiding unnecessary work

Fixes #4444 (if applicable - replace with actual issue number)

Original prompt

This section details on the original issue you should resolve

<issue_title>Redundant initialization of Crypto->SparseAckRanges in QuicCryptoInitialize</issue_title>
<issue_description>### Description
In crypto.c, the function QuicCryptoInitialize initializes the Crypto->SparseAckRanges member twice:

    QuicRangeInitialize(
        QUIC_MAX_RANGE_ALLOC_SIZE,
        &Crypto->SparseAckRanges);

    Crypto->TlsState.BufferAllocLength = SendBufferLength;
    Crypto->TlsState.Buffer = CXPLAT_ALLOC_NONPAGED(SendBufferLength, QUIC_POOL_TLS_BUFFER);
    if (Crypto->TlsState.Buffer == NULL) {
        QuicTraceEvent(
            AllocFailure,
            "Allocation of '%s' failed. (%llu bytes)",
            "crypto send buffer",
            SendBufferLength);
        Status = QUIC_STATUS_OUT_OF_MEMORY;
        goto Exit;
    }

    QuicRangeInitialize(
        QUIC_MAX_RANGE_ALLOC_SIZE,
        &Crypto->SparseAckRanges);

Can we remove one of these?
</issue_description>

<agent_instructions>Be mindful of any unintended behavior change or side effect and highlight if removing one of the initializations could result in a regression.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

Fixes #5389


💡 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 Copilot AI changed the title [WIP] Fix redundant initialization of Crypto->SparseAckRanges in QuicCryptoInitialize Remove redundant QuicRangeInitialize call in QuicCryptoInitialize Oct 20, 2025
@Copilot Copilot AI requested a review from guhetier October 20, 2025 16:49
Copilot finished work on behalf of guhetier October 20, 2025 16:49
@guhetier guhetier marked this pull request as ready for review October 20, 2025 16:55
@guhetier guhetier requested a review from a team as a code owner October 20, 2025 16:55
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.64%. Comparing base (bb6ad40) to head (fffe4e1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5533      +/-   ##
==========================================
- Coverage   84.68%   84.64%   -0.05%     
==========================================
  Files          59       59              
  Lines       18623    18622       -1     
==========================================
- Hits        15771    15762       -9     
- Misses       2852     2860       +8     

☔ 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.

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.

Redundant initialization of Crypto->SparseAckRanges in QuicCryptoInitialize

2 participants