-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
fix ping on OSX when running as root #35070
Conversation
Tagging subscribers to this area: @dotnet/ncl |
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs
Outdated
Show resolved
Hide resolved
@@ -19,6 +19,8 @@ public partial class Ping | |||
private const int IcmpHeaderLengthInBytes = 8; | |||
private const int MinIpHeaderLengthInBytes = 20; | |||
private const int MaxIpHeaderLengthInBytes = 60; | |||
private static bool _sendIpHeader = RuntimeInformation.IsOSPlatform(OSPlatform.OSX); | |||
private static bool _needsConnect = RuntimeInformation.IsOSPlatform(OSPlatform.Linux); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are very fast calls; I'm not sure they need to be cached like this. But if you think it's important, make 'em readonly as suggested.
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs
Show resolved
Hide resolved
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Ping/tests/FunctionalTests/PingTest.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs
Outdated
Show resolved
Hide resolved
/azp run runtime-libraries outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-libraries outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
This is fix for thee distance issues with current implementation.
first is regression on OSX caused by dotnet/corefx#34084
It seems like OSX does connect behind the scenes and Send() files
System.Net.Sockets.SocketException (56): Socket is already connected
even if we callConnect
just once. So I limit that to Linux only.fixes #33430
Second issues is when PingOptions is used. In that case we would always try to set
socket.DontFragment = socketConfig.Options.DontFragment
and that fails on OSX because that is not supported by OS. Since we use RAW socket we have option too include IP header and set DF bit directly. I used Wireshark to verify that DF is properly set based on PingOptions.fixes #34879
Third problem is with IPv6: (currently not tracked by any issue)
Since we know what AF we need, the fix was to turn off DualMode on RAW socket.
I don't think we really need "packet information" and that can be possibly future improvement.
All this happened because we do not have any test coverage for RAW sockets in practice since CI runs as ordinary user. However, some other assemblies use RemoteExcetopr with RunAsSudo to get privileged access. It seems like we should be able to detect in CI. My prototype runs shows exactly the reported issues so it may be worth of getting at least some basic coverage.
https://helix.dot.net/api/2019-06-17/jobs/353faf88-9320-4dfa-8215-534c515b3fa0/workitems/System.Net.Ping.Functional.Tests/console
Downside of this is that the test depends now on sudo for outer loop runs. It may asked for password if executed interactively or it may fail if given user is not in shudders group.
Given the history, it seems like this may be worth oof some hassle.