add option to handle Ping TTL for raw socket and system utility by wfurt · Pull Request #30324 · dotnet/corefx

wfurt · 2018-06-12T16:32:15Z

related to #9350

This adds TTL option to Unix Ping implementation. It handles both - raw socket as well as system command line utility. I did some testing and -t seems pretty standard even on busybox on Alpine.

Don't fragment will need some more research and testing. It is less common (like busybox does not have it). Also PMTU discovery is default now on Linux so "don't fragment is set by default".

I tested the fix running tests as normal user and root. All existing tests are passing.
To really test this, we would need Outerloop test with some predictable and stable target.

wfurt · 2018-06-12T16:32:24Z

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot test Outerloop OSX x64 Debug Build

stephentoub · 2018-06-12T18:27:13Z

src/Common/src/System/Net/NetworkInformation/UnixCommandLinePing.cs

+                }
+                else
+                {
+                    // Linux uses -t ttl for both IPv4 & IPv6


And macOS uses -t for ipv4?

yes. It is TTL for IPv4 and Hop Limit for IPv6.

stephentoub · 2018-06-12T18:28:22Z

src/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs

            }

-            string processArgs = UnixCommandLinePing.ConstructCommandLine(buffer.Length, address.ToString(), isIpv4);
+            string processArgs = UnixCommandLinePing.ConstructCommandLine(buffer.Length, address.ToString(), isIpv4, options?.Ttl ?? 0);


Is there a way to test this?

test if the option is supported by ping?

Sorry, I didn't mean to comment on this line specifically... I meant test that this PR is doing what was intended, that the TTL is being respected now.

yes. I did run wireshark when running on my Ubuntu VM. I'll re-run the test on OSX.
As far as unit test, we would need stable server with distance greater than 1.
Than we could craft outerloop test and verify results with different TTL.

stephentoub · 2018-06-12T18:32:32Z

src/Common/src/System/Net/NetworkInformation/UnixCommandLinePing.cs


        private static readonly string s_discoveredPing4UtilityPath = GetPingUtilityPath(ipv4: true);
        private static readonly string s_discoveredPing6UtilityPath = GetPingUtilityPath(ipv4: false);
+        private static readonly bool s_isOSX = Environment.OSVersion.Platform == PlatformID.MacOSX;


Did you test this and it worked? I thought we returned PlatformID.Unix for macOS.

wfurt · 2018-06-12T18:46:51Z

cc: @janvorli for any other platform thoughts.

wfurt · 2018-06-20T18:04:14Z

Merged with upstream changes and correct some issues.
The OS detection indeed did not work right but all test passed because of changes for #15018.
The other problem is that "localhost" normally only resolves to 127.0.0.1 on Unix.
/etc/host does not allow multiple addresses for the same name. (I check on Ubuntu, Centos and OSX)

So I refactored tests little bit so we try basic address tests for both IPv4 and IPv6 - in similar way how we test sockets.

As I mentioned originally, more work will be needed for "don't fragment" as busybox implementation on Alpine does not support it.

stephentoub · 2018-06-22T02:45:21Z

src/System.Net.Ping/tests/FunctionalTests/PingTest.cs

+        public void SendPingWithIPAddress_AddressAsString(AddressFamily addressFamily)
        {
            IPAddress localIpAddress = TestSettings.GetLocalIPAddress();
+             if (localIpAddress == null)


I assume addressFamily should be passed to GetLocalIPAddress here.

yes, it is. I don't know why github does not show it.

I don't know why github does not show it

Because it's not there :-) I just pulled down your change, and it's:

IPAddress localIpAddress = TestSettings.GetLocalIPAddress(); if (localIpAddress == null)

yes, I found it. I was looking at wrong lines. sorry.

it so happened line 128 has almost exact (and right code). but the broken was on 146.
Updated and pushed change.

stephentoub · 2018-06-22T02:45:42Z

src/System.Net.Ping/tests/FunctionalTests/PingTest.cs

        {
-            IPAddress localIpAddress = TestSettings.GetLocalIPAddress();
+            IPAddress localIpAddress = TestSettings.GetLocalIPAddress(addressFamily);
+


Nit: the other tests don't have this blank line

stephentoub · 2018-06-22T02:45:52Z

src/System.Net.Ping/tests/FunctionalTests/PingTest.cs

+        public void SendPingWithIPAddress_AddressAsString(AddressFamily addressFamily)
        {
            IPAddress localIpAddress = TestSettings.GetLocalIPAddress();
+             if (localIpAddress == null)


Nit: extra space before if

stephentoub · 2018-06-22T02:46:01Z

src/System.Net.Ping/tests/FunctionalTests/PingTest.cs

        {
-            IPAddress localIpAddress = TestSettings.GetLocalIPAddress();
+            IPAddress localIpAddress = TestSettings.GetLocalIPAddress(addressFamily);
+


Nit: remove blank line

stephentoub · 2018-06-22T02:48:38Z

src/System.Net.Ping/tests/FunctionalTests/TestSettings.cs

            }

-            throw new InvalidOperationException("Unable to discover any addresses for the local host.");
+            return addressFamily == AddressFamily.InterNetwork ? IPAddress.Parse("127.0.0.1") : IPAddress.Parse("::1");


IPAddress.Loopback and IPAddress.IPv6Loopback?

…et/corefx#30324) * add option to handle TTL for raw socket and system utility * add missing blank line * incorporate feedback and merge with upstream * feedback from review Commit migrated from dotnet/corefx@3035218

add option to handle TTL for raw socket and system utility

b3f30d0

wfurt added bug Product bug (most likely) os-linux Linux OS (any supported distro) labels Jun 12, 2018

wfurt self-assigned this Jun 12, 2018

davidsh added the area-System.Net label Jun 12, 2018

davidsh added this to the 3.0 milestone Jun 12, 2018

add missing blank line

dc6df38

stephentoub reviewed Jun 12, 2018

View reviewed changes

wfurt changed the title ~~add option to handle TTL for raw socket and system utility~~ add option to handle Ping TTL for raw socket and system utility Jun 12, 2018

wfurt requested a review from a team June 12, 2018 18:46

stephentoub mentioned this pull request Jun 13, 2018

Adding support for synchronous Ping #30000

Merged

wfurt added 2 commits June 20, 2018 10:00

Merge branch 'master' of https://github.com/dotnet/corefx into fix_9350

d4f67a4

incorporate feedback and merge with upstream

bbefd9f

stephentoub reviewed Jun 22, 2018

View reviewed changes

stephentoub approved these changes Jun 22, 2018

View reviewed changes

feedback from review

161303e

wfurt merged commit 3035218 into dotnet:master Jun 26, 2018

wfurt mentioned this pull request Jan 8, 2019

support for fragmentation Ping option on Unix #34435

Merged

wfurt deleted the fix_9350 branch June 15, 2020 18:21

Conversation

wfurt commented Jun 12, 2018

Uh oh!

wfurt commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wfurt commented Jun 12, 2018

Uh oh!

wfurt commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants