support for fragmentation Ping option on Unix by wfurt · Pull Request #34435 · dotnet/corefx

wfurt · 2019-01-08T20:53:01Z

fixes #9350 [Ping] Unix - System.Net.Ping ignores PingOptions

This is similar to #30324 with caveat that Busybox ping used on Alpine by default does not support options for fragmentation. When such condition is detected, option will be silently ignored as it is right now. One option on Alpine is to install "normal" ping via package manager. It should always work when using RAW socket implementation.

This change also adds new tests to verify integration on Unix. If we pass option ping implementation does not like (like I found with Busybox) this show blow up.

I also added on test to external server to actually test Ttl processing. I found one case where virtualization sets TTL back to 64 when NATing outbound packets and test fails. We can disable that test if this become problematic in future. It works fine in Azure and on my physical machines.

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

stephentoub · 2019-01-09T14:45:25Z

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

+            int resultLength = ReadLink(pingBinary, buffer, 4000);
+
+            // If pingBinary is not link resultLength will be -1
+            if (resultLength > 0 && resultLength < 4000)


Where is this 4000 number coming from? It's used multiple times, so it'd be good to put it in a named constant, but I'm unclear as to its origin. Is it guaranteed to be big enough? Is it larger than is actually needed? Do we need to try again with ReadLink if the buffer size was insufficient?

My memory failed me. I was thinking about MAXPATHLEN but that is only 1024 on Linux. That should be sufficient for any case. And probably bigger than we need as well. It is always possible that some system or particular installation would do something weird with ping and we may fail. When ping is part of os and it is link to something else, it would normally live in /[usr/]/[local]/[s]bin If it is not, it is probably not busybox.
Also note, that the code does not (and cannot) detect hard links. It will detect normal scenarios like Alpine and it may fail to detect corner cases.

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

stephentoub · 2019-01-09T14:50:42Z

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

+                        sb.Append(" -D ");
+                    }
+                }
+                else if (!s_isBusybox.Value)  // busybox implementation does not support fragmentation option.


Are there possibly other things beyond the "busybox" implementation that don't support this? What happens if we come across such an implementation... this will cause all pings to fail by default, right? Is there any risk there?

Yes, there is some risk. I added test with primary intent to catch such case. (ping loopback with this on/off) We should hit this path only if the option was specified e.g. simple p = new Ping() ; p.Send() will not hit this. Also the privileged path with RAW socket will not hit this.
The risk is contained to set of specific conditions.
Also for example on Alpine, one can choose to install full ping, it just is not the default. So if we come across distribution with some unusual implementation we can revisit the detection or mitigation.

We should hit this path only if the option was specified

Won't we hit it if any option was specified, not just this one? Maybe that's what you meant.

Yes. The PingOptions only has Ttl and DontFragment. So it is simple ping with no (default) options or options.

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

stephentoub · 2019-01-09T15:22:06Z

Thanks, @wfurt. Generally looks good, but some issues to be addressed.

…rag_9350

… ping_frag_9350

wfurt · 2019-01-10T23:16:04Z

OSX failed in DNS tests. Unrelated to this.

@dotnet-bot test OSX x64 Debug Build

wfurt · 2019-01-11T23:34:56Z

@dotnet-bot test OSX x64 Debug Build
@dotnet-bot test Windows x64 Debug Build

stephentoub

It still makes me nervous that we might be using options that aren't supported. We should continue to think about other options, e.g. whether we could detect that a failure is due to a bad option and then retry without the option and remember that for future usage, or re-investigate alternatives to shelling out to the command-line when not elevated, etc. Thanks.

stephentoub · 2019-01-17T14:33:13Z

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

+                return true;
+            }
+
+            return false;


Nit: this can just be:

return linkedNamed != null && linkedName.EndsWith("busybox", StringComparison.Ordinal);

stephentoub · 2019-01-17T14:33:36Z

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

+        {
+            string linkedName = Interop.Sys.ReadLink(pingBinary);
+
+            // If pingBinary is not link linkedName will be null


Nit: comma after "link"

stephentoub · 2019-01-17T14:36:46Z

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

+                if (s_isBSD)
+                {
+                    // The bit is off by default on OSX & FreeBSD
+                    if (fragmentOption == PingFragmentOptions.Dont) {


Nit: move brace to next line

wfurt · 2019-01-17T17:55:23Z

I understand @stephentoub. That was one reason why I did not do it together with Ttl. There at least two other approaches we can take. We can spawn one ping to loopback and see if that works with that option. We can check for rapid failure when used first time (or parse output on non-zero return code) and try it again without it.

* add support for fragmentation Ping option on Unix * add missing project file * small adjustments * fix typo * feedback from review * remove no longer needed import * remove changes to HttpCookieProtocolTests pulled in by mistake Commit migrated from dotnet/corefx@8fea941

Tomas Weinfurt and others added 4 commits January 8, 2019 12:24

add support for fragmentation Ping option on Unix

871082c

add missing project file

c81f73f

small adjustments

9eba636

fix typo

74e01f0

wfurt added the area-System.Net label Jan 8, 2019

wfurt added this to the 3.0 milestone Jan 8, 2019

wfurt self-assigned this Jan 8, 2019

wfurt requested a review from a team January 8, 2019 20:53

davidsh added the os-linux Linux OS (any supported distro) label Jan 8, 2019

stephentoub reviewed Jan 9, 2019

View reviewed changes

src/Common/src/System/Net/NetworkInformation/UnixCommandLinePing.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jan 9, 2019

View reviewed changes

src/Common/src/System/Net/NetworkInformation/UnixCommandLinePing.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jan 9, 2019

View reviewed changes

src/Common/src/System/Net/NetworkInformation/UnixCommandLinePing.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jan 9, 2019

View reviewed changes

src/Common/src/System/Net/NetworkInformation/UnixCommandLinePing.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jan 9, 2019

View reviewed changes

src/Common/src/System/Net/NetworkInformation/UnixCommandLinePing.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jan 9, 2019

View reviewed changes

src/System.Net.Ping/tests/FunctionalTests/PingTest.cs Outdated Show resolved Hide resolved

wfurt added 6 commits January 9, 2019 10:59

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

82a5a9f

…rag_9350

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

c2751f7

…rag_9350

feedback from review

1f74388

Merge branch 'ping_frag_9350' of https://github.com/wfurt/corefx into…

d990ea4

… ping_frag_9350

remove no longer needed import

89ccaa9

remove changes to HttpCookieProtocolTests pulled in by mistake

e56bf26

stephentoub approved these changes Jan 17, 2019

View reviewed changes

wfurt merged commit 8fea941 into dotnet:master Jan 24, 2019

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

Conversation

wfurt commented Jan 8, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stephentoub commented Jan 9, 2019

Uh oh!

wfurt commented Jan 10, 2019

Uh oh!

wfurt commented Jan 11, 2019

Uh oh!

stephentoub left a comment

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 Jan 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stephentoub Jan 9, 2019 •

edited

Loading