Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Allow interface names in IPv6 link-local addresses #35278

Merged
merged 17 commits into from
Jun 15, 2019
Merged

Allow interface names in IPv6 link-local addresses #35278

merged 17 commits into from
Jun 15, 2019

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Feb 12, 2019 •
edited
Loading

Fixes https://github.com/dotnet/corefx/issues/27529
I tried to re-use IPAddress-related stuff in mono and our Dns implementation doesn't hesitate to use fe80::e8b0:63ff:fee8:6b3b%awdl0-like format.

E.g. if I type ipconfig |grep inet in my terminal on OSX I get:

egor-macbook:System egorb$ ifconfig |grep inet
	inet 127.0.0.1 netmask 0xff000000
	inet6 ::1 prefixlen 128
	inet6 fe80::1%lo0 prefixlen 64 scopeid 0x1
	inet 192.168.100.3 netmask 0xffffff00 broadcast 192.168.100.255
	inet6 fe80::e8b0:63ff:fee8:6b3b%awdl0 prefixlen 64 scopeid 0x7
	inet6 fe80::8729:7785:ecd8:ed6%utun0 prefixlen 64 scopeid 0xb
	inet6 fe80::50:41ff:fe00:101%gpd0 prefixlen 64 scopeid 0xc
	inet 100.64.118.134 netmask 0xffffffff broadcast 100.64.118.134

So I expect IPAddress.Parse("fe80::e8b0:63ff:fee8:6b3b%awdl0").ScopeId to return 0x7 instead of a FormatException or at least 0 (ignore that part).

Java supports this format (Inet6Address.getByName("fe80::e8b0:63ff:fee8:6b3b%awdl0")) but throws java.net.UnknownHostException: no such interface fakei123 for unknown interfaces.

/cc @marek-safar
Also cc @MarcoRossignoli who worked on https://github.com/dotnet/corefx/issues/28863

@davidsh davidsh added this to the 3.0 milestone Feb 12, 2019
stephentoub
stephentoub reviewed Feb 13, 2019
View reviewed changes
src/Common/src/Interop/Unix/System.Native/Interop.GetNativeIPInterfaceIndex.cs Outdated
{
internal static partial class Sys
{
[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetNativeIPInterfaceIndex")]
Copy link
Member

Choose a reason for hiding this comment

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

This should use SetLastError = true, since the method uses errno for errors.

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's just a light-weight copy of https://github.com/dotnet/corefx/blob/master/src/Common/src/Interop/BSD/System.Native/Interop.ProtocolStatistics.cs#L159-L160 - should I add SetLastError there too?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally yes, although it looks like the call sites don't currently pay attention to or incorporate errno in their thrown exceptions.

stephentoub
stephentoub approved these changes Feb 23, 2019
View reviewed changes
src/Common/src/Interop/Windows/IpHlpApi/Interop.if_nametoindex.cs Outdated Show resolved Hide resolved
src/Common/src/System/Net/IPv6AddressHelper.Common.cs Outdated
@@ -303,11 +298,14 @@ internal static unsafe void Parse(ReadOnlySpan<char> address, ushort* numbers, i
numberIsValid = false;
}

start = i;
start = i + 1;
Copy link
Member

Choose a reason for hiding this comment

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

Why does this change?

rmkerr
rmkerr suggested changes Mar 20, 2019
View reviewed changes
Copy link
Contributor

@rmkerr rmkerr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

There are some failures showing up in CI for this change:
image
It looks like some of these failures are caused by a UriFormatException being thrown where we were succeeding before. That would definitely be a breaking change, and needs to be addressed before we can merge this.

@ViktorHofer
Copy link
Member

@EgorBo can you please take a look at the failures?

@ViktorHofer
Copy link
Member

/azp run corefx-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidsh
Copy link
Contributor

davidsh commented May 27, 2019

@EgorBo Please hold off on this PR until #37733 and #37734 are merged.

@stephentoub
Copy link
Member

The relevant changes have been merged. @EgorBo, do you want to update this?

@EgorBo
Copy link
Member Author

EgorBo commented Jun 3, 2019

@stephentoub oh, sure!

stephentoub
stephentoub reviewed Jun 12, 2019
View reviewed changes
src/Common/src/Interop/Windows/IpHlpApi/Interop.if_nametoindex.cs
internal static partial class IpHlpApi
{
[DllImport(Interop.Libraries.IpHlpApi, SetLastError = true)]
internal extern static uint if_nametoindex(string name);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this isn't available in UWP, which is why a few of the legs are failing with a BCL0015 error. It'll need to be conditionally used when not building for that configuration.

@davidsh
Copy link
Contributor

davidsh commented Jun 15, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

wfurt
wfurt approved these changes Jun 15, 2019
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.
lo is predictably ifindex 1 on linux if we want test for address with valid interface.

@davidsh davidsh merged commit fef0f81 into dotnet:master Jun 15, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@EgorBo @davidsh
Allow interface names in IPv6 link-local addresses (dotnet/corefx#35278)
1277f6c 
* Handle interface names in ipv6 addresses (link-local)

* fix test projects

* Address feedback

* Add SetLastError to if_nametoindex

* fix tests

* fix tests

* Add more tests

* move native impl to pal_networking.c/h

* add newlines to pal_networking

* fix build

* remove "Vista" comment from Interop.if_nametoindex.cs

* test fix for Uri.cs

* undo changes

* Always return 0 for if_nametoindex on UAP


Commit migrated from dotnet/corefx@fef0f81
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub approved these changes

@rmkerr rmkerr rmkerr requested changes

@davidsh davidsh davidsh approved these changes

@wfurt wfurt wfurt approved these changes

Assignees

@EgorBo EgorBo

@stephentoub stephentoub

@davidsh davidsh

Labels
area-System.Net
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

unable to parse IPv6 link-local address with interface name
6 participants
@EgorBo @ViktorHofer @davidsh @stephentoub @rmkerr @wfurt