Fix parsing of ping error replies on raw sockets (#61592)

* Fix parsing of ping error replies on raw sockets

* Re-enable ping tests

* Update the SendPingToExternalHostWithLowTtlTest test validation

diff --git a/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs b/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs
index 341c328..fa57126 100644 (file)
--- a/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs
+++ b/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs
@@ -18,6 +18,7 @@ namespace System.Net.NetworkInformation
         private const int IcmpHeaderLengthInBytes = 8;
         private const int MinIpHeaderLengthInBytes = 20;
         private const int MaxIpHeaderLengthInBytes = 60;
+        private const int IpV6HeaderLengthInBytes = 40;
 
         private unsafe SocketConfig GetSocketConfig(IPAddress address, byte[] buffer, int timeout, PingOptions? options)
         {
@@ -123,22 +124,98 @@ namespace System.Net.NetworkInformation
             }
 
             int icmpHeaderOffset = ipHeaderLength;
+            int dataOffset = ipHeaderLength + IcmpHeaderLengthInBytes;
 
             // Skip IP header.
             IcmpHeader receivedHeader = MemoryMarshal.Read<IcmpHeader>(receiveBuffer.AsSpan(icmpHeaderOffset));
+            ushort identifier = 0;
             type = receivedHeader.Type;
             code = receivedHeader.Code;
 
-            if (socketConfig.Identifier != receivedHeader.Identifier
-                || type == (byte)IcmpV4MessageType.EchoRequest
-                || type == (byte)IcmpV6MessageType.EchoRequest) // Echo Request, ignore
+            // Validate the ICMP header and get the identifier
+            if (socketConfig.IsIpv4)
+            {
+                if (type == (byte)IcmpV4MessageType.EchoReply)
+                {
+                    // Reply packet has the identifier in the ICMP header.
+                    identifier = receivedHeader.Identifier;
+                }
+                else if (type == (byte)IcmpV4MessageType.DestinationUnreachable ||
+                         type == (byte)IcmpV4MessageType.TimeExceeded ||
+                         type == (byte)IcmpV4MessageType.ParameterProblemBadIPHeader ||
+                         type == (byte)IcmpV4MessageType.SourceQuench ||
+                         type == (byte)IcmpV4MessageType.RedirectMessage)
+                {
+                    // Original IP+ICMP request is in the payload. Read the ICMP header from
+                    // the payload to get identifier.
+
+                    if (dataOffset + MinIpHeaderLengthInBytes + IcmpHeaderLengthInBytes > bytesReceived)
+                    {
+                        return false;
+                    }
+
+                    byte ihl = (byte)(receiveBuffer[dataOffset] & 0x0f); // Internet Header Length
+                    int payloadIpHeaderLength = 4 * ihl;
+
+                    if (bytesReceived - dataOffset - payloadIpHeaderLength < IcmpHeaderLengthInBytes)
+                    {
+                        return false; // Not enough bytes to reconstruct actual IP header + ICMP header.
+                    }
+
+                    IcmpHeader originalRequestHeader = MemoryMarshal.Read<IcmpHeader>(receiveBuffer.AsSpan(dataOffset + payloadIpHeaderLength));
+                    identifier = originalRequestHeader.Identifier;
+
+                    // Update the date offset to point past the payload IP+ICMP headers. While the specification
+                    // doesn't indicate there should be any additional data the reality is that we often get the
+                    // original packet data back.
+                    dataOffset += payloadIpHeaderLength + IcmpHeaderLengthInBytes;
+                }
+                else
+                {
+                    return false;
+                }
+            }
+            else
+            {
+                if (type == (byte)IcmpV6MessageType.EchoReply)
+                {
+                    // Reply packet has the identifier in the ICMP header.
+                    identifier = receivedHeader.Identifier;
+                }
+                else if (type == (byte)IcmpV6MessageType.DestinationUnreachable ||
+                         type == (byte)IcmpV6MessageType.TimeExceeded ||
+                         type == (byte)IcmpV6MessageType.ParameterProblem ||
+                         type == (byte)IcmpV6MessageType.PacketTooBig)
+                {
+                    // Original IP+ICMP request is in the payload. Read the ICMP header from
+                    // the payload to get identifier.
+
+                    if (bytesReceived - dataOffset < IpV6HeaderLengthInBytes + IcmpHeaderLengthInBytes)
+                    {
+                        return false; // Not enough bytes to reconstruct actual IP header + ICMP header.
+                    }
+
+                    IcmpHeader originalRequestHeader = MemoryMarshal.Read<IcmpHeader>(receiveBuffer.AsSpan(dataOffset + IpV6HeaderLengthInBytes));
+                    identifier = originalRequestHeader.Identifier;
+
+                    // Update the date offset to point past the payload IP+ICMP headers. While the specification
+                    // doesn't indicate there should be any additional data the reality is that we often get the
+                    // original packet data back.
+                    dataOffset += IpV6HeaderLengthInBytes + IcmpHeaderLengthInBytes;
+                }
+                else
+                {
+                    return false;
+                }
+            }
+
+            if (socketConfig.Identifier != identifier)
             {
                 return false;
             }
 
             sw.Stop();
             long roundTripTime = sw.ElapsedMilliseconds;
-            int dataOffset = ipHeaderLength + IcmpHeaderLengthInBytes;
             // We want to return a buffer with the actual data we sent out, not including the header data.
             byte[] dataBuffer = new byte[bytesReceived - dataOffset];
             Buffer.BlockCopy(receiveBuffer, dataOffset, dataBuffer, 0, dataBuffer.Length);
@@ -162,7 +239,7 @@ namespace System.Net.NetworkInformation
                 {
                     socket.SendTo(socketConfig.SendBuffer, SocketFlags.None, socketConfig.EndPoint);
 
-                    byte[] receiveBuffer = new byte[MaxIpHeaderLengthInBytes + IcmpHeaderLengthInBytes + buffer.Length];
+                    byte[] receiveBuffer = new byte[2 * (MaxIpHeaderLengthInBytes + IcmpHeaderLengthInBytes) + buffer.Length];
 
                     long elapsed;
                     Stopwatch sw = Stopwatch.StartNew();
@@ -213,7 +290,7 @@ namespace System.Net.NetworkInformation
                         timeoutTokenSource.Token)
                         .ConfigureAwait(false);
 
-                    byte[] receiveBuffer = new byte[MaxIpHeaderLengthInBytes + IcmpHeaderLengthInBytes + buffer.Length];
+                    byte[] receiveBuffer = new byte[2 * (MaxIpHeaderLengthInBytes + IcmpHeaderLengthInBytes) + buffer.Length];
 
                     Stopwatch sw = Stopwatch.StartNew();
                     // Read from the socket in a loop. We may receive messages that are not echo replies, or that are not in response

diff --git a/src/libraries/System.Net.Ping/tests/FunctionalTests/PingTest.cs b/src/libraries/System.Net.Ping/tests/FunctionalTests/PingTest.cs
index b206e1e..8b649c1 100644 (file)
--- a/src/libraries/System.Net.Ping/tests/FunctionalTests/PingTest.cs
+++ b/src/libraries/System.Net.Ping/tests/FunctionalTests/PingTest.cs
@@ -124,7 +124,6 @@ namespace System.Net.NetworkInformation.Tests
         [Theory]
         [InlineData(AddressFamily.InterNetwork)]
         [InlineData(AddressFamily.InterNetworkV6)]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public void SendPingWithIPAddress(AddressFamily addressFamily)
         {
             IPAddress localIpAddress = TestSettings.GetLocalIPAddress(addressFamily);
@@ -145,7 +144,6 @@ namespace System.Net.NetworkInformation.Tests
         [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
         [InlineData(AddressFamily.InterNetwork)]
         [InlineData(AddressFamily.InterNetworkV6)]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendPingAsyncWithIPAddress(AddressFamily addressFamily)
         {
             IPAddress localIpAddress = await TestSettings.GetLocalIPAddressAsync(addressFamily);
@@ -166,7 +164,6 @@ namespace System.Net.NetworkInformation.Tests
         [Theory]
         [InlineData(AddressFamily.InterNetwork)]
         [InlineData(AddressFamily.InterNetworkV6)]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public void SendPingWithIPAddress_AddressAsString(AddressFamily addressFamily)
         {
             IPAddress localIpAddress = TestSettings.GetLocalIPAddress(addressFamily);
@@ -185,7 +182,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendPingAsyncWithIPAddress_AddressAsString()
         {
             IPAddress localIpAddress = await TestSettings.GetLocalIPAddressAsync();
@@ -199,7 +195,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public void SendPingWithIPAddressAndTimeout()
         {
             IPAddress localIpAddress = TestSettings.GetLocalIPAddress();
@@ -213,7 +208,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendPingAsyncWithIPAddressAndTimeout()
         {
             IPAddress localIpAddress = await TestSettings.GetLocalIPAddressAsync();
@@ -260,7 +254,6 @@ namespace System.Net.NetworkInformation.Tests
 
         [PlatformSpecific(TestPlatforms.AnyUnix)]  // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
         [Fact]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public void SendPingWithIPAddressAndTimeoutAndBuffer_Unix()
         {
             byte[] buffer = TestSettings.PayloadAsBytes;
@@ -286,7 +279,6 @@ namespace System.Net.NetworkInformation.Tests
 
         [PlatformSpecific(TestPlatforms.AnyUnix)]  // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendPingAsyncWithIPAddressAndTimeoutAndBuffer_Unix()
         {
             byte[] buffer = TestSettings.PayloadAsBytes;
@@ -350,7 +342,6 @@ namespace System.Net.NetworkInformation.Tests
         [Theory]
         [InlineData(AddressFamily.InterNetwork)]
         [InlineData(AddressFamily.InterNetworkV6)]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public void SendPingWithIPAddressAndTimeoutAndBufferAndPingOptions_Unix(AddressFamily addressFamily)
         {
             IPAddress localIpAddress = TestSettings.GetLocalIPAddress(addressFamily);
@@ -383,7 +374,6 @@ namespace System.Net.NetworkInformation.Tests
         [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
         [InlineData(AddressFamily.InterNetwork)]
         [InlineData(AddressFamily.InterNetworkV6)]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendPingAsyncWithIPAddressAndTimeoutAndBufferAndPingOptions_Unix(AddressFamily addressFamily)
         {
             IPAddress localIpAddress = await TestSettings.GetLocalIPAddressAsync(addressFamily);
@@ -413,7 +403,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public void SendPingWithHost()
         {
             IPAddress[] localIpAddresses = TestSettings.GetLocalIPAddresses();
@@ -427,7 +416,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendPingAsyncWithHost()
         {
             IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
@@ -441,7 +429,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public void SendPingWithHostAndTimeout()
         {
             IPAddress[] localIpAddresses = TestSettings.GetLocalIPAddresses();
@@ -455,7 +442,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendPingAsyncWithHostAndTimeout()
         {
             IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
@@ -502,7 +488,6 @@ namespace System.Net.NetworkInformation.Tests
 
         [PlatformSpecific(TestPlatforms.AnyUnix)]  // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
         [Fact]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public void SendPingWithHostAndTimeoutAndBuffer_Unix()
         {
             IPAddress[] localIpAddresses = TestSettings.GetLocalIPAddresses();
@@ -528,7 +513,6 @@ namespace System.Net.NetworkInformation.Tests
 
         [PlatformSpecific(TestPlatforms.AnyUnix)]  // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendPingAsyncWithHostAndTimeoutAndBuffer_Unix()
         {
             IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
@@ -588,7 +572,6 @@ namespace System.Net.NetworkInformation.Tests
 
         [PlatformSpecific(TestPlatforms.AnyUnix)]  // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
         [Fact]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public void SendPingWithHostAndTimeoutAndBufferAndPingOptions_Unix()
         {
             IPAddress[] localIpAddresses = TestSettings.GetLocalIPAddresses();
@@ -614,7 +597,6 @@ namespace System.Net.NetworkInformation.Tests
 
         [PlatformSpecific(TestPlatforms.AnyUnix)]  // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendPingAsyncWithHostAndTimeoutAndBufferAndPingOptions_Unix()
         {
             IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
@@ -663,7 +645,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendPings_ReuseInstance_Hostname()
         {
             IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
@@ -679,7 +660,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task Sends_ReuseInstance_Hostname()
         {
             IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
@@ -695,7 +675,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendAsyncs_ReuseInstance_Hostname()
         {
             IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
@@ -746,7 +725,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public static void Ping_DisposeAfterSend_Success()
         {
             Ping p = new Ping();
@@ -755,7 +733,6 @@ namespace System.Net.NetworkInformation.Tests
         }
 
         [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public static async Task PingAsync_DisposeAfterSend_Success()
         {
             Ping p = new Ping();
@@ -832,7 +809,6 @@ namespace System.Net.NetworkInformation.Tests
         [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
         [InlineData(true)]
         [InlineData(false)]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
         public async Task SendPingAsyncWithHostAndTtlAndFragmentPingOptions(bool fragment)
         {
             IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
@@ -878,7 +854,8 @@ namespace System.Net.NetworkInformation.Tests
             options.Ttl = 1;
             // This should always fail unless host is one IP hop away.
             pingReply = await ping.SendPingAsync(host, TestSettings.PingTimeout, TestSettings.PayloadAsBytesShort, options);
-            Assert.NotEqual(IPStatus.Success, pingReply.Status);
+            Assert.Equal(IPStatus.TimeExceeded, pingReply.Status);
+            Assert.NotEqual(IPAddress.Any, pingReply.Address);
         }
 
         [Fact]