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

Fix Uri.Host for IPv6 Link-local address #29829

Merged
merged 10 commits into from
May 30, 2018
Merged

Fix Uri.Host for IPv6 Link-local address #29829

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f2ded59
fix
MarcoRossignoli May 21, 2018
2d9edda
skip test on netfx
MarcoRossignoli May 21, 2018
64f3c78
address PR feedback
MarcoRossignoli May 21, 2018
36d25f6
address PR feedbacl
MarcoRossignoli May 22, 2018
b565053
CurlHandler fix attempt
MarcoRossignoli May 24, 2018
6684fbb
address PR feedback
MarcoRossignoli May 24, 2018
0ed1e60
address PR feedback
MarcoRossignoli May 24, 2018
e729a15
address PR feedback
MarcoRossignoli May 25, 2018
192781b
address PR feedback
MarcoRossignoli May 25, 2018
4517b55
address PR feedback
MarcoRossignoli May 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/System.Private.Uri/src/System/IPv6AddressHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ internal static class IPv6AddressHelper

// methods

internal static string ParseCanonicalName(string str, int start, ref bool isLoopback, ref string scopeId)
internal static string ParseCanonicalName(string str, int start, ref bool isLoopback, ref bool linkLocalAddress, ref string scopeId)
{
unsafe
{
Expand All @@ -31,7 +31,8 @@ internal static string ParseCanonicalName(string str, int start, ref bool isLoop
((long*)numbers)[0] = 0L;
((long*)numbers)[1] = 0L;
isLoopback = Parse(str, numbers, start, ref scopeId);
return '[' + CreateCanonicalName(numbers) + ']';
linkLocalAddress = numbers[0] == 0xfe80;
return '[' + CreateCanonicalName(numbers) + (linkLocalAddress ? scopeId : "") + ']';
Copy link
Member

@stephentoub stephentoub May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: the compiler will help fix this up, but it'd be better to use "[" and "]" rather than '[' and ']'.

Copy link
Member Author

Choose a reason for hiding this comment

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

enlighten me...why?

Copy link
Member

@stephentoub stephentoub May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Because we want this to compile down to the efficient string.Concat(string, string, string, string) call. If the compiler didn't help here, this would end up being a call to string.Concat(params object[] args), which would result in an object[] allocation, the chars getting boxed (so two more allocations), and then having ToString called on them (so two more allocations), plus having a string array allocated in the Concat implementation to store the ToString on each input. As of dotnet/roslyn#415, the C# compiler should do the conversion from e.g. '[' to "[" for you, but why make it.

Copy link
Member Author

Choose a reason for hiding this comment

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

understood TYVM!

}
}

Expand Down
11 changes: 9 additions & 2 deletions src/System.Private.Uri/src/System/Uri.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ private enum Flags : ulong
FragmentIriCanonical = 0x40000000000,
IriCanonical = 0x78000000000,
UnixPath = 0x100000000000,
LinkLocalAddress = 0x200000000000
Copy link
Contributor

Choose a reason for hiding this comment

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

"LinkLocalAddress -> "IPv6LinkLocalAddress"

}

private Flags _flags;
Expand Down Expand Up @@ -1193,7 +1194,7 @@ public string DnsSafeHost
if (HostType == Flags.IPv6HostType)
{
ret = ret.Substring(1, ret.Length - 2);
if ((object)_info.ScopeId != null)
if ((object)_info.ScopeId != null && (_flags & Flags.LinkLocalAddress) == 0)
{
ret += _info.ScopeId;
}
Expand Down Expand Up @@ -2565,6 +2566,7 @@ private unsafe void CreateHostString()
private static string CreateHostStringHelper(string str, ushort idx, ushort end, ref Flags flags, ref string scopeId)
{
bool loopback = false;
bool linkLocalAddress = false;
string host;
switch (flags & Flags.HostTypeMask)
{
Expand All @@ -2574,7 +2576,7 @@ private static string CreateHostStringHelper(string str, ushort idx, ushort end,

case Flags.IPv6HostType:
// The helper will return [...] string that is not suited for Dns.Resolve()
host = IPv6AddressHelper.ParseCanonicalName(str, idx, ref loopback, ref scopeId);
host = IPv6AddressHelper.ParseCanonicalName(str, idx, ref loopback, ref linkLocalAddress, ref scopeId);
break;

case Flags.IPv4HostType:
Expand Down Expand Up @@ -2615,7 +2617,12 @@ private static string CreateHostStringHelper(string str, ushort idx, ushort end,
if (loopback)
{
flags |= Flags.LoopbackHost;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be extra whitespace (space or tabs) after this closing brace.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

if (linkLocalAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

add newline above this 'if' statement.

{
flags |= Flags.LinkLocalAddress;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

add newline after closing brace


return host;
}

Expand Down
11 changes: 11 additions & 0 deletions src/System.Private.Uri/tests/FunctionalTests/IdnDnsSafeHostTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,17 @@ public void IdnDnsSafeHost_IPv6Host_ScopeIdButNoBrackets()
Assert.Equal("[::1]", test.Host);
}

[Fact]
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
public void IdnDnsSafeHost_IPv6HostLinkLocalAddress_ScopeIdButNoBrackets()
{
Uri test = new Uri("http://[fe80::e077:c9a3:eeba:b8e9%18]/");
Copy link
Contributor

Choose a reason for hiding this comment

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

You should refactor this duplicated string, "fe80::e077:c9a3:eeba:b8e9%18" into a constant in this test.

Copy link
Member Author

@MarcoRossignoli MarcoRossignoli May 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

i copied the pattern from test above https://github.com/MarcoRossignoli/corefx/blob/2d9eddafdf568afab7a8bb730415bfce9b885a15/src/System.Private.Uri/tests/FunctionalTests/IdnDnsSafeHostTest.cs#L41 do you confirm this refactoring?

Copy link
Contributor

Choose a reason for hiding this comment

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

i copied the pattern from test above https://github.com/MarcoRossignoli/corefx/blob/2d9eddafdf568afab7a8bb730415bfce9b885a15/src/System.Private.Uri/tests/FunctionalTests/IdnDnsSafeHostTest.cs#L41 do you confirm this refactoring?

Following existing code and test patterns can be useful. But it doesn't work in all cases. Sometimes there are bad patterns in the code. And we don't change them all, but try to add better code for new PRs. Also, the pattern you cited uses simpler IP literals. This test uses a long IPv6 literal. So, for code cleanliness and ease of maintenance, it's better to consolidate if possible.

Copy link
Member Author

@MarcoRossignoli MarcoRossignoli May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

done with [Theory] could be useful for future new data, if you like.


Copy link
Contributor

Choose a reason for hiding this comment

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

Use

const string ScopedLiteralIpv6 = "fe80::e077:c9a3:eeba:b8e9%18";

since address and zoneIndex is not used anywhere else in the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yes i missed it sorry

Copy link
Member Author

Choose a reason for hiding this comment

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

done, updated also other test with constants.

Assert.Equal("fe80::e077:c9a3:eeba:b8e9%18", test.DnsSafeHost);
Assert.Equal("fe80::e077:c9a3:eeba:b8e9%18", test.IdnHost);
Assert.Equal("[fe80::e077:c9a3:eeba:b8e9%18]", test.Host);
}

[Fact]
public void IdnDnsSafeHost_MixedCase_ToLowerCase()
{
Expand Down
28 changes: 28 additions & 0 deletions src/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,34 @@ private void ParseBadIPv4Address(string badIpv4String)

#region IPv6

[Theory]
[InlineData("fe80::e077:c9a3:eeba:b8e9", "%18")]
[InlineData("Fe80::e077:c9a3:eeba:b8e9", "%18")]
[InlineData("fE80::e077:c9a3:eeba:b8e9", "%18")]
[InlineData("FE80::e077:c9a3:eeba:b8e9", "%18")]
[InlineData("FE80::e077:c9a3:eeba:b8e9", "%eth10")]
[InlineData("FE80::e077:c9a3:eeba:b8e9", "")]
[InlineData("FE80::e077:c9a3:eeba:b8e9", "%")]
[InlineData("fe80::e077:c9a3:eeba:b8e9", "%\u30AF\u20E7")]
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
public void Host_IPv6LinkLocalAddress_HasScopeId(string address, string zoneIndex)
{
string scopedLiteralIpv6 = "[" + address + zoneIndex + "]";
Copy link
Contributor

Choose a reason for hiding this comment

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

"scopedLiteralIpv6" -> "scopedLiteralIpv6Brackets" to be more consistent with variable naming in the tests above.

string literalIpV6Uri = "http://" + scopedLiteralIpv6;
var uri = new Uri(literalIpV6Uri);
Assert.Equal(scopedLiteralIpv6, uri.Host, ignoreCase: true);
}

[Theory]
[InlineData("fe81::e077:c9a3:eeba:b8e9", "%18")]
public void Host_NonIPv6LinkLocalAddress_NoScopeId(string address, string zoneIndex)
{
string scopedLiteralIpv6 = "[" + address + zoneIndex + "]";
Copy link
Contributor

Choose a reason for hiding this comment

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

"scopedLiteralIpv6" -> "scopedLiteralIpv6Brackets" to be more consistent with variable naming in the tests above.

string literalIpV6Uri = "http://" + scopedLiteralIpv6;
var uri = new Uri(literalIpV6Uri);
Assert.Equal("[" + address + "]", uri.Host);
}

[Fact]
public void UriIPv6Host_CanonicalCollonHex_Success()
{
Expand Down