Skip to content

Fix division by zero in cubic.c NetStats bandwidth calculation when SmoothedRtt is 0#5888

Open
Copilot wants to merge 4 commits intomainfrom
copilot/fix-division-by-zero-netstats-again
Open

Fix division by zero in cubic.c NetStats bandwidth calculation when SmoothedRtt is 0#5888
Copilot wants to merge 4 commits intomainfrom
copilot/fix-division-by-zero-netstats-again

Conversation

Copy link
Contributor

Copilot AI commented Mar 23, 2026

Description

Bandwidth = CongestionWindow / SmoothedRtt in cubic.c crashes with STATUS_INTEGER_DIVIDE_BY_ZERO when an ACK is processed before the first RTT sample is available (SmoothedRtt == 0). This affects two code paths:

  • CubicCongestionControlGetNetworkStatistics — queried bandwidth when RTT not yet sampled
  • CubicCongestionControlOnDataAcknowledgedNetStatsEventEnabled path on early ACK

Both sites now return Bandwidth = 0 when SmoothedRtt == 0:

// Before
NetworkStatistics->Bandwidth = Cubic->CongestionWindow / Path->SmoothedRtt;

// After
NetworkStatistics->Bandwidth = Path->SmoothedRtt == 0 ? 0 : Cubic->CongestionWindow / Path->SmoothedRtt;

Testing

Two new unit tests were added to src/core/unittest/CubicTest.cpp to cover both fixed paths:

  • CubicTest.GetNetworkStatistics_ZeroSmoothedRtt_BandwidthIsZero — calls GetNetworkStatistics with SmoothedRtt == 0 and asserts Bandwidth == 0 without crashing.
  • CubicTest.OnDataAcknowledged_NetStats_ZeroSmoothedRtt_BandwidthIsZero — fires an ACK with NetStatsEventEnabled = TRUE and SmoothedRtt == 0 and asserts no divide-by-zero crash occurs.

All 52 CubicTest cases pass.

Documentation

No documentation impact.

Original prompt

This section details on the original issue you should resolve

<issue_title>[DeepTest] Division by zero in OnDataAcknowledged NetStats event path</issue_title>
<issue_description>### Describe the bug
Location: src/core/cubic.c, line 701
Event.NETWORK_STATISTICS.Bandwidth = Cubic->CongestionWindow / Path->SmoothedRtt;

Root cause: Same division-by-zero pattern as #5833, but in the OnDataAcknowledged function's NetStatsEventEnabled path. If NetStatsEventEnabled is TRUE and an ACK is processed before SmoothedRtt is set, the same crash occurs.

Affected OS

  • Windows
  • Linux
  • macOS
  • Other (specify below)

Additional OS information

No response

MsQuic version

main

Steps taken to reproduce bug

  1. Add the following test in src/core/unittest/CubicTest.cpp
TEST(CubicTest, DeepTest_Bug_NetStatsEventDivByZero)
{
    QUIC_CONNECTION Connection;
    QUIC_SETTINGS_INTERNAL Settings{};
    Settings.InitialWindowPackets = 10;
    Settings.SendIdleTimeoutMs = 1000;

    InitializeMockConnection(Connection, 1280);
    Connection.Settings.NetStatsEventEnabled = TRUE;
    Connection.ClientCallbackHandler = DummyConnectionCallback;
    // SmoothedRtt is 0 by default

    CubicCongestionControlInitialize(&Connection.CongestionControl, &Settings);

    QUIC_CONGESTION_CONTROL_CUBIC* Cubic = &Connection.CongestionControl.Cubic;
    Cubic->BytesInFlight = 5000;

    QUIC_ACK_EVENT AckEvent;
    CxPlatZeroMemory(&AckEvent, sizeof(AckEvent));
    AckEvent.TimeNow = 1000000;
    AckEvent.LargestAck = 5;
    AckEvent.LargestSentPacketNumber = 10;
    AckEvent.NumRetransmittableBytes = 1000;
    AckEvent.NumTotalAckedRetransmittableBytes = 1000;
    AckEvent.SmoothedRtt = 0; // No RTT sample
    AckEvent.MinRtt = 0;
    AckEvent.MinRttValid = FALSE;
    AckEvent.AckedPackets = NULL;

    // This will crash with STATUS_INTEGER_DIVIDE_BY_ZERO
    // because Path->SmoothedRtt=0 and line 701 does: CongestionWindow / SmoothedRtt
    Connection.CongestionControl.QuicCongestionControlOnDataAcknowledged(
        &Connection.CongestionControl, &AckEvent);
}
  1. Build and run the test with filter CubicTest.DeepTest_Bug_NetStatsEventDivByZero

Expected behavior

Should not crash.

Actual outcome

STATUS_INTEGER_DIVIDE_BY_ZERO crash

Additional details

No response</issue_description>

<agent_instructions>Fix this issue. </agent_instructions>

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


⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.

Copilot AI changed the title [WIP] Fix division by zero in OnDataAcknowledged NetStats event path Fix division by zero in cubic.c NetStats bandwidth calculation when SmoothedRtt is 0 Mar 23, 2026
Copilot AI requested a review from saikat107 March 23, 2026 18:03
@saikat107
Copy link
Contributor

@copilot add test case(s) in the src/core/unittest/CubicTest.cpp to illustrate the change

Copy link
Contributor Author

Copilot AI commented Mar 23, 2026

@copilot add test case(s) in the src/core/unittest/CubicTest.cpp to illustrate the change

Added two test cases in commit 2bb5c96:

  • CubicTest.GetNetworkStatistics_ZeroSmoothedRtt_BandwidthIsZero — calls GetNetworkStatistics with SmoothedRtt == 0 and asserts Bandwidth == 0 (no crash).
  • CubicTest.OnDataAcknowledged_NetStats_ZeroSmoothedRtt_BandwidthIsZero — fires an ACK with NetStatsEventEnabled = TRUE and SmoothedRtt == 0 and asserts no divide-by-zero crash occurs.

Both tests are placed alongside the existing GetNetworkStatistics_RetrieveStats test and all 52 CubicTest cases pass.

@saikat107 saikat107 marked this pull request as ready for review March 23, 2026 18:55
@saikat107 saikat107 requested a review from a team as a code owner March 23, 2026 18:55
@codecov
Copy link

codecov bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.78%. Comparing base (0071278) to head (4fe4893).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5888      +/-   ##
==========================================
- Coverage   86.23%   84.78%   -1.45%     
==========================================
  Files          60       60              
  Lines       18732    18732              
==========================================
- Hits        16153    15882     -271     
- Misses       2579     2850     +271     

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

guhetier
guhetier previously approved these changes Mar 26, 2026
Copy link
Collaborator

@guhetier guhetier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we can ever reach that point in the code without having at least one RTT sample (since we got an Ack, we likely updated the RTT right before), but since the path is not trivial, the validation won't hurt and maybe avoid an issue in a corner case.

Thanks for the PR

@guhetier guhetier enabled auto-merge (squash) March 26, 2026 22:38
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.

[DeepTest] Division by zero in OnDataAcknowledged NetStats event path

3 participants