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.

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 parsing of ping error replies on raw sockets #61592

Merged
merged 3 commits into from
Nov 30, 2021

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Nov 15, 2021

Fixes #61465

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 15, 2021
@ghost
Copy link

ghost commented Nov 15, 2021

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

Issue Details

Fixes #61465

Needs more testing. The ICMPv4 code path was tested locally, ICMPv6 was not tested at all.

Author: filipnavara
Assignees: -
Labels:

area-System.Net, community-contribution

Milestone: -

@wfurt
Copy link
Member

wfurt commented Nov 17, 2021

LGTM

ifconfig en0
en0: flags=8863<UP,BROADCAST,SMART,RUNNING,SIMPLEX,MULTICAST> mtu 1500
	options=10b<RXCSUM,TXCSUM,VLAN_HWTAGGING,AV>
	ether 68:5b:35:ce:4a:d8
	inet6 fe80::1c62:813b:8b0d:4b5%en0 prefixlen 64 secured scopeid 0x7
	inet 10.127.72.87 netmask 0xfffffc00 broadcast 10.127.75.255
	inet6 2001:4898:f0:20:1c02:4dba:a6b5:e6c0 prefixlen 64 autoconf secured

$  /Users/presenter/Downloads/ping/bin/Debug/net6.0/osx-x64/publish/ping
Status: TimeExceeded Address: 2001:4898:f0:20:ff::2

it would be great IMHO to add (outerloop) test. It would need to ignore cases when there is no answer and perhaps simply test that the address is not ANY.
I would add two entries (for v4 & v6) similar to

public static string PingHost => GetValue("DOTNET_TEST_NET_PING_HOST", "www.microsoft.com");
with 8.8.8.8 and 2001:4860:4860::8888 as defaults.

@filipnavara
Copy link
Member Author

Thanks for testing it. Apparently there's already similar test (SendPingToExternalHostWithLowTtlTest) in outer loop, it just doesn't validate ICMPv6 and the resulting Status/Address.

@filipnavara filipnavara marked this pull request as ready for review November 17, 2021 11:01
@karelz karelz requested a review from wfurt November 22, 2021 09:31
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 @filipnavara

@wfurt wfurt merged commit 439ffd2 into dotnet:main Nov 30, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2021
@karelz karelz added this to the 7.0.0 milestone Jan 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class Ping not reading response when ICMP Time-to-live exceeded
3 participants