Skip to content

Explicitly check for TTL expiration when using Ping utility #65312

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rzikm merged 3 commits into from
Feb 16, 2022
Merged

Explicitly check for TTL expiration when using Ping utility #65312

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04f2218
Explicitly check for TTL expiration when using Ping utility
rzikm Feb 14, 2022
3d4ae1b
Fixes
rzikm Feb 14, 2022
9e2175d
Improve the implementation
rzikm Feb 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.PingUtility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,30 @@ private async Task<PingReply> SendWithPingUtilityAsync(IPAddress address, byte[]
return CreatePingReply(IPStatus.TimedOut);
}

if (p.ExitCode == 1 || p.ExitCode == 2)
try
{
string? output = null;
// Throw timeout for known failure return codes from ping functions.
return CreatePingReply(IPStatus.TimedOut);
}
if (p.ExitCode == 1 || p.ExitCode == 2)
{
if (options?.Ttl != null)
{
// TTL exceeded may have occured
output = await p.StandardOutput.ReadToEndAsync().ConfigureAwait(false);
if (output.Contains("Time to live exceeded", StringComparison.Ordinal))
Copy link
Member

@wfurt wfurt Feb 15, 2022

Choose a reason for hiding this comment

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

This is going to be problematic IMHO. This message may come out localized. I'm wondering if we should accept that we cannot really distinguish the cases with confidence..

cc: @filipnavara @tmds

Copy link
Member

@wfurt wfurt Feb 15, 2022

Choose a reason for hiding this comment

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

maybe not. It seems like we explicitly set LC_ALL but dependency on particular text string still worries me little bit.

Copy link
Member

@filipnavara filipnavara Feb 15, 2022

Choose a reason for hiding this comment

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

IMHO it's worth a try. We can check strings in the common implementations (FreeBSD, GNU Inetutils, macOS*). Worst case it will behave the same way as it does now, best case we can provide slightly more accurate result.

However, the code needs to do way more input checking than it does now. The .IndexOf calls may fail to find anything and it would throw ArgumentOutOfRangeException.

Copy link
Member Author

@rzikm rzikm Feb 15, 2022

Choose a reason for hiding this comment

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

I realize that the dependency is suboptimal, one of the reason I put up this PR is to check is to check if it gets a hit on CI (I tried both Arch and Ubuntu locally, so I have hopes)

Will add checking of the indices

{
// look for address in "From 172.21.64.1 icmp_seq=1 Time to live exceeded"
int addressStart = output.IndexOf("From ", StringComparison.Ordinal) + 5;
int addressLength = output.IndexOf(' ', addressStart) - addressStart;
address = IPAddress.Parse(output.AsSpan(addressStart, addressLength));
return CreatePingReply(IPStatus.TimeExceeded, address);
}
}

try
{
string output = await p.StandardOutput.ReadToEndAsync().ConfigureAwait(false);
return CreatePingReply(IPStatus.TimedOut);
}

output ??= await p.StandardOutput.ReadToEndAsync().ConfigureAwait(false);
return ParsePingUtilityOutput(address, output);
}
catch (Exception)
Expand All @@ -112,6 +127,7 @@ private async Task<PingReply> SendWithPingUtilityAsync(IPAddress address, byte[]
private PingReply ParsePingUtilityOutput(IPAddress address, string output)
{
long rtt = UnixCommandLinePing.ParseRoundTripTime(output);

return new PingReply(
address,
null, // Ping utility cannot accommodate these, return null to indicate they were ignored.
Expand Down
12 changes: 6 additions & 6 deletions src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ private unsafe SocketConfig GetSocketConfig(IPAddress address, byte[] buffer, in
bool sendIpHeader = ipv4 && options != null && SendIpHeader;
int totalLength = 0;

if (sendIpHeader)
{
if (sendIpHeader)
{
iph.VersionAndLength = 0x45;
totalLength = sizeof(IpHeader) + checked(sizeof(IcmpHeader) + buffer.Length);
totalLength = sizeof(IpHeader) + checked(sizeof(IcmpHeader) + buffer.Length);
// On OSX this strangely must be host byte order.
iph.TotalLength = OperatingSystem.IsFreeBSD() ? (ushort)IPAddress.HostToNetworkOrder((short)totalLength) : (ushort)totalLength;
iph.Protocol = 1; // ICMP
Expand All @@ -46,7 +46,7 @@ private unsafe SocketConfig GetSocketConfig(IPAddress address, byte[] buffer, in
#pragma warning restore 618
// No need to fill in SourceAddress or checksum.
// If left blank, kernel will fill it in - at least on OSX.
}
}

return new SocketConfig(
new IPEndPoint(address, 0), timeout, options,
Expand Down Expand Up @@ -340,11 +340,11 @@ await socket.SendToAsync(
}
}

private static PingReply CreatePingReply(IPStatus status)
private static PingReply CreatePingReply(IPStatus status, IPAddress? address = null, int rtt = 0)
{
// Documentation indicates that you should only pay attention to the IPStatus value when
// its value is not "Success", but the rest of these values match that of the Windows implementation.
return new PingReply(new IPAddress(0), null, status, 0, Array.Empty<byte>());
return new PingReply(address ?? new IPAddress(0), null, status, rtt, Array.Empty<byte>());
}

#if DEBUG
Expand Down