Skip to content
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

Jump to bottom

Use unprivileged socket to send ping on macOS/iOS/tvOS/Mac Catalyst #52240

Merged
merged 4 commits into from
May 7, 2021
Merged

Use unprivileged socket to send ping on macOS/iOS/tvOS/Mac Catalyst #52240

merged 4 commits into from
May 7, 2021

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented May 4, 2021 •
edited
Loading

Revival of #42006 with some feedback incorporated.

This improves Ping behavior on macOS/iOS family. For unprivileged users, it allows to send/receive buffer with custom content and removes craft around spawning new process. (uses this example).

It adds new macOS/iOS/tvOS targets for the assembly that exclude the process creation code.

Fixes #36941
Fixes #51395

cc: @wfurt @marek-safar

@ghost
Copy link

ghost commented May 4, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Revival of #42006 with some feedback incorporated.

This improves Ping behavior on macOS/iOS family. For unprivileged users, it allows to send/receive buffer with custom content and removes craft around spawning new process. (uses this example).

It adds new macOS/iOS/tvOS targets for the assembly that exclude the process creation code.

Fixes #36941
Fixes #36890

cc: @wfurt @marek-safar

Author: filipnavara
Assignees: -
Labels:

area-System.Net
Milestone: -

@marek-safar marek-safar requested a review from wfurt May 4, 2021 11:12
@filipnavara filipnavara changed the title Use unpriviledged socket to send ping on macOS/iOS/tvOS/Mac Catalyst Use unprivileged socket to send ping on macOS/iOS/tvOS/Mac Catalyst May 4, 2021
@filipnavara filipnavara marked this pull request as ready for review May 4, 2021 15:25
marek-safar
marek-safar reviewed May 6, 2021
View reviewed changes
src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs Outdated
{
int ipHeaderLength = socketConfig.IsIpv4 ? MinIpHeaderLengthInBytes : 0;

await socket.SendToAsync(
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this pass the timeout in too?

Copy link
Member Author

Choose a reason for hiding this comment

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

It passes the timeout to the socket options which should apply here.

wfurt
wfurt approved these changes May 7, 2021
View reviewed changes
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks for dragging this to completion @filipnavara
I was not sure if we want to give up ping utility on OSX and if the capability to for example send custom patterns is equivalent. But having it consistent on top of sockets is nice.

@wfurt
Copy link
Member

wfurt commented May 7, 2021

runtime-libraries-coreclr outerloop-linux

@wfurt
Copy link
Member

wfurt commented May 7, 2021

/azp run runtime-libraries-coreclr outerloop-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member

wfurt commented May 7, 2021

/azp run runtime-libraries-coreclr outerloop-osx

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@filipnavara filipnavara mentioned this pull request May 7, 2021
Closed
27 tasks
@akoeplinger
Copy link
Member

The outerloop tests on a950b48 were good, just unrelated failures: https://dev.azure.com/dnceng/public/_build/results?buildId=1126913&view=results

Merging :)

@akoeplinger akoeplinger merged commit e98614e into dotnet:main May 7, 2021
@filipnavara filipnavara deleted the ios-ping branch May 7, 2021 14:19
@karelz karelz mentioned this pull request May 10, 2021
Closed
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
spouliot pushed a commit to spouliot/xamarin-macios that referenced this pull request Jun 16, 2021
[tests][link sdk] Re-enable test for Ping for dotnet
8f5f412 
ref: dotnet/runtime#41355

Unlike what we have today for legacy support for Ping was added in
dotnet/runtime#52240
@spouliot spouliot mentioned this pull request Jun 16, 2021
Merged
spouliot added a commit to xamarin/xamarin-macios that referenced this pull request Jun 16, 2021
@spouliot
[tests][link sdk] Re-enable test for Ping for dotnet (#11951)
974aad0 
ref: dotnet/runtime#41355

Unlike what we have today for legacy support for Ping was added in
dotnet/runtime#52240
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marek-safar marek-safar marek-safar approved these changes

@akoeplinger akoeplinger akoeplinger approved these changes

@wfurt wfurt wfurt approved these changes

Assignees
No one assigned
Labels
area-System.Net
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

System.Net.Ping.Functional.Tests Fails on iOS, tvOS Ping does not work on iOS devices
5 participants
@filipnavara @wfurt @akoeplinger @marek-safar @karelz