Fix Ping RoundtripTime returning 0 for TtlExpired replies#124424
Open
lufen wants to merge 2 commits intodotnet:mainfrom
Open
Fix Ping RoundtripTime returning 0 for TtlExpired replies#124424lufen wants to merge 2 commits intodotnet:mainfrom
lufen wants to merge 2 commits intodotnet:mainfrom
Conversation
Contributor
Author
@dotnet-policy-service agree |
Contributor
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the Windows Ping implementation so PingReply.RoundtripTime is preserved for TTL-expired / time-exceeded ICMP replies (matching the behavior of underlying Windows ICMP APIs and Unix raw-socket behavior), and adds a regression test for it.
Changes:
- Windows: populate RTT from
ICMP_ECHO_REPLY/ICMPV6_ECHO_REPLYforIPStatus.TtlExpiredandIPStatus.TimeExceeded(instead of hard-coding 0). - Tests: add an outer-loop functional test that exercises low-TTL ping behavior and asserts RTT is non-zero on supported code paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs | Preserve RTT for TTL-expired / time-exceeded replies in IPv4 and IPv6 Windows reply creation. |
| src/libraries/System.Net.Ping/tests/FunctionalTests/PingTest.cs | Adds an outer-loop regression test validating RTT behavior for TTL-expired / time-exceeded replies. |
src/libraries/System.Net.Ping/tests/FunctionalTests/PingTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs
Outdated
Show resolved
Hide resolved
b4a0b80 to
c011e09
Compare
3 tasks
stephentoub
reviewed
Mar 4, 2026
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs
Outdated
Show resolved
Hide resolved
19f7683 to
ef52814
Compare
Member
|
@lufen there are build errors after the last commit, can you please take a look? |
ef52814 to
47edb25
Compare
The ICMP_ECHO_REPLY.RoundTripTime field is always populated by the Windows ICMP API for any received reply, regardless of IP status. Previously this was discarded and hardcoded to 0 for all non-Success statuses. Now we always read the RTT from the reply struct, which matches the behavior of the raw socket implementation on Unix. This simplification is supported by Wine's open-source IcmpSendEcho implementation which unconditionally sets RoundTripTime = recv_time - send_time for every valid reply, and by empirical evidence from the underlying Windows API (see issue discussion). Fixes dotnet#118150 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
47edb25 to
82aa081
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs
Show resolved
Hide resolved
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Windows.cs
Show resolved
Hide resolved
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.
When a Ping results in TtlExpired or TimeExceeded, the underlying ICMP_ECHO_REPLY struct contains a valid round-trip time from the intermediate router. Previously this was discarded and hardcoded to 0 for all non-Success statuses.
This change reads the RTT from the reply for TtlExpired and TimeExceeded statuses in both IPv4 and IPv6 Windows code paths, matching the behavior of the raw socket implementation on Unix.
Fixes #118150