From 270f8f8ae202f5ca760daacc6b183837b3d0662b Mon Sep 17 00:00:00 2001 From: William Godbe Date: Fri, 31 May 2019 09:34:10 -0700 Subject: [PATCH] Fix IPv6Address parsing (dotnet/corefx#37734) * Fix IPv6Address parsing * Update src/System.Net.Primitives/src/System/Net/IPAddressParser.cs Co-Authored-By: Karel Zikmund * Use System.Diagnostics, Remove duplicate function, fix method header in Fake * Fix deletion of call to Parse() * Fix call to Parse() * Fix tests to account for NetFx runs * Fix Syntax Commit migrated from https://github.com/dotnet/corefx/commit/f8c382f76955e8da9f0cf314d5945a5af34d72b7 --- .../src/System/Net/IPv6AddressHelper.Common.cs | 15 +++++++- .../src/System/Net/IPAddress.cs | 4 +-- .../src/System/Net/IPAddressParser.cs | 10 +++--- .../tests/FunctionalTests/IPAddressParsing.cs | 22 ++++++++++++ .../tests/UnitTests/Fakes/IPv6AddressHelper.cs | 4 +-- .../src/System/IPv6AddressHelper.cs | 42 +++++----------------- .../tests/FunctionalTests/UriIpHostTest.cs | 29 ++++++++++++++- 7 files changed, 82 insertions(+), 44 deletions(-) diff --git a/src/libraries/Common/src/System/Net/IPv6AddressHelper.Common.cs b/src/libraries/Common/src/System/Net/IPv6AddressHelper.Common.cs index beb5e86..03a90f8 100644 --- a/src/libraries/Common/src/System/Net/IPv6AddressHelper.Common.cs +++ b/src/libraries/Common/src/System/Net/IPv6AddressHelper.Common.cs @@ -2,6 +2,8 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Diagnostics; + namespace System { internal static partial class IPv6AddressHelper @@ -107,6 +109,17 @@ namespace System { start++; needsClosingBracket = true; + + // IsValidStrict() is only called if there is a ':' in the name string, i.e. + // it is a possible IPv6 address. So, if the string starts with a '[' and + // the pointer is advanced here there are still more characters to parse. + Debug.Assert(start < end); + } + + // Starting with a colon character is only valid if another colon follows. + if (name[start] == ':' && (start + 1 >= end || name[start + 1] != ':')) + { + return false; } int i; @@ -278,7 +291,7 @@ namespace System // Nothing // - internal static unsafe void Parse(ReadOnlySpan address, ushort* numbers, int start, ref string scopeId) + internal static void Parse(ReadOnlySpan address, Span numbers, int start, ref string scopeId) { int number = 0; int index = 0; diff --git a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs index faa856f..083128a 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs @@ -146,10 +146,10 @@ namespace System.Net PrivateScopeId = (uint)scopeid; } - internal unsafe IPAddress(ushort* numbers, int numbersLength, uint scopeid) + internal IPAddress(ReadOnlySpan numbers, uint scopeid) { Debug.Assert(numbers != null); - Debug.Assert(numbersLength == NumberOfLabels); + Debug.Assert(numbers.Length == NumberOfLabels); var arr = new ushort[NumberOfLabels]; for (int i = 0; i < arr.Length; i++) diff --git a/src/libraries/System.Net.Primitives/src/System/Net/IPAddressParser.cs b/src/libraries/System.Net.Primitives/src/System/Net/IPAddressParser.cs index fc82078..eb94c38 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/IPAddressParser.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/IPAddressParser.cs @@ -14,17 +14,17 @@ namespace System.Net { private const int MaxIPv4StringLength = 15; // 4 numbers separated by 3 periods, with up to 3 digits per number - internal static unsafe IPAddress Parse(ReadOnlySpan ipSpan, bool tryParse) + internal static IPAddress Parse(ReadOnlySpan ipSpan, bool tryParse) { if (ipSpan.Contains(':')) { // The address is parsed as IPv6 if and only if it contains a colon. This is valid because // we don't support/parse a port specification at the end of an IPv4 address. - ushort* numbers = stackalloc ushort[IPAddressParserStatics.IPv6AddressShorts]; - new Span(numbers, IPAddressParserStatics.IPv6AddressShorts).Clear(); + Span numbers = stackalloc ushort[IPAddressParserStatics.IPv6AddressShorts]; + numbers.Clear(); if (Ipv6StringToAddress(ipSpan, numbers, IPAddressParserStatics.IPv6AddressShorts, out uint scope)) { - return new IPAddress(numbers, IPAddressParserStatics.IPv6AddressShorts, scope); + return new IPAddress(numbers, scope); } } else if (Ipv4StringToAddress(ipSpan, out long address)) @@ -193,7 +193,7 @@ namespace System.Net } } - public static unsafe bool Ipv6StringToAddress(ReadOnlySpan ipSpan, ushort* numbers, int numbersLength, out uint scope) + public static unsafe bool Ipv6StringToAddress(ReadOnlySpan ipSpan, Span numbers, int numbersLength, out uint scope) { Debug.Assert(numbers != null); Debug.Assert(numbersLength >= IPAddressParserStatics.IPv6AddressShorts); diff --git a/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs b/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs index a57aac8..4baf57e 100644 --- a/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs +++ b/src/libraries/System.Net.Primitives/tests/FunctionalTests/IPAddressParsing.cs @@ -346,6 +346,7 @@ namespace System.Net.Primitives.Functional.Tests public static IEnumerable InvalidIpv6Addresses() { + yield return new object[] { "[:]" }; // malformed yield return new object[] { ":::4df" }; yield return new object[] { "4df:::" }; yield return new object[] { "0:::4df" }; @@ -368,6 +369,24 @@ namespace System.Net.Primitives.Functional.Tests yield return new object[] { "Fe08::1]]" }; // two trailing brackets yield return new object[] { "[Fe08::1]]" }; // one leading and two trailing brackets yield return new object[] { ":1" }; // leading single colon + yield return new object[] { ":1:2" }; // leading single colon + yield return new object[] { ":1:2:3" }; // leading single colon + yield return new object[] { ":1:2:3:4" }; // leading single colon + yield return new object[] { ":1:2:3:4:5" }; // leading single colon + yield return new object[] { ":1:2:3:4:5:6" }; // leading single colon + yield return new object[] { ":1:2:3:4:5:6:7" }; // leading single colon + yield return new object[] { ":1:2:3:4:5:6:7:8" }; // leading single colon + yield return new object[] { ":1:2:3:4:5:6:7:8:9" }; // leading single colon + yield return new object[] { "::1:2:3:4:5:6:7:8" }; // compressor with too many number groups + yield return new object[] { "1::2:3:4:5:6:7:8" }; // compressor with too many number groups + yield return new object[] { "1:2::3:4:5:6:7:8" }; // compressor with too many number groups + yield return new object[] { "1:2:3::4:5:6:7:8" }; // compressor with too many number groups + yield return new object[] { "1:2:3:4::5:6:7:8" }; // compressor with too many number groups + yield return new object[] { "1:2:3:4:5::6:7:8" }; // compressor with too many number groups + yield return new object[] { "1:2:3:4:5:6::7:8" }; // compressor with too many number groups + yield return new object[] { "1:2:3:4:5:6:7::8" }; // compressor with too many number groups + yield return new object[] { "1:2:3:4:5:6:7:8::" }; // compressor with too many number groups + yield return new object[] { "::1:2:3:4:5:6:7:8:9" }; // compressor with too many number groups yield return new object[] { "1:" }; // trailing single colon yield return new object[] { " ::1" }; // leading whitespace yield return new object[] { "::1 " }; // trailing whitespace @@ -420,6 +439,9 @@ namespace System.Net.Primitives.Functional.Tests new object[] { "%12" }, // just scope new object[] { "[192.168.0.1]" }, // raw v4 new object[] { "[1]" }, // incomplete + new object[] { "" }, // malformed + new object[] { "[" }, // malformed + new object[] { "[]" }, // malformed }; [Theory] diff --git a/src/libraries/System.Net.Primitives/tests/UnitTests/Fakes/IPv6AddressHelper.cs b/src/libraries/System.Net.Primitives/tests/UnitTests/Fakes/IPv6AddressHelper.cs index c46e30e..079a5d1 100644 --- a/src/libraries/System.Net.Primitives/tests/UnitTests/Fakes/IPv6AddressHelper.cs +++ b/src/libraries/System.Net.Primitives/tests/UnitTests/Fakes/IPv6AddressHelper.cs @@ -10,8 +10,8 @@ namespace System { internal unsafe static (int longestSequenceStart, int longestSequenceLength) FindCompressionRange( ReadOnlySpan numbers) => (-1, -1); - internal unsafe static bool ShouldHaveIpv4Embedded(ushort[] numbers) => false; + internal unsafe static bool ShouldHaveIpv4Embedded(ReadOnlySpan numbers) => false; internal unsafe static bool IsValidStrict(char* name, int start, ref int end) => false; - internal static unsafe bool Parse(ReadOnlySpan ipSpan, ushort* numbers, int start, ref string scopeId) => false; + internal static unsafe bool Parse(ReadOnlySpan ipSpan, Span numbers, int start, ref string scopeId) => false; } } diff --git a/src/libraries/System.Private.Uri/src/System/IPv6AddressHelper.cs b/src/libraries/System.Private.Uri/src/System/IPv6AddressHelper.cs index f0f2750..c35daca 100644 --- a/src/libraries/System.Private.Uri/src/System/IPv6AddressHelper.cs +++ b/src/libraries/System.Private.Uri/src/System/IPv6AddressHelper.cs @@ -15,12 +15,9 @@ namespace System internal static unsafe string ParseCanonicalName(string str, int start, ref bool isLoopback, ref string scopeId) { - ushort* numbersPtr = stackalloc ushort[NumberOfLabels]; - // optimized zeroing of 8 shorts = 2 longs - ((long*)numbersPtr)[0] = 0L; - ((long*)numbersPtr)[1] = 0L; - Span numbers = new Span(numbersPtr, NumberOfLabels); - Parse(str, numbersPtr, start, ref scopeId); + Span numbers = stackalloc ushort[NumberOfLabels]; + numbers.Clear(); + Parse(str, numbers, start, ref scopeId); isLoopback = IsLoopback(numbers); // RFC 5952 Sections 4 & 5 - Compressed, lower case, with possible embedded IPv4 addresses. @@ -118,33 +115,6 @@ namespace System || (numbers[5] == 0xFFFF)))); } - // Returns true if the IPv6 address should be formated with an embedded IPv4 address: - // ::192.168.1.1 - private static unsafe bool ShouldHaveIpv4Embedded(ushort* numbers) - { - // 0:0 : 0:0 : x:x : x.x.x.x - if (numbers[0] == 0 && numbers[1] == 0 && numbers[2] == 0 && numbers[3] == 0 && numbers[6] != 0) - { - // RFC 5952 Section 5 - 0:0 : 0:0 : 0:[0 | FFFF] : x.x.x.x - if (numbers[4] == 0 && (numbers[5] == 0 || numbers[5] == 0xFFFF)) - { - return true; - } - // SIIT - 0:0 : 0:0 : FFFF:0 : x.x.x.x - else if (numbers[4] == 0xFFFF && numbers[5] == 0) - { - return true; - } - } - // ISATAP - if (numbers[4] == 0 && numbers[5] == 0x5EFE) - { - return true; - } - - return false; - } - // // InternalIsValid // @@ -188,6 +158,12 @@ namespace System bool expectingNumber = true; int lastSequence = 1; + // Starting with a colon character is only valid if another colon follows. + if (name[start] == ':' && (start + 1 >= end || name[start + 1] != ':')) + { + return false; + } + int i; for (i = start; i < end; ++i) { diff --git a/src/libraries/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs b/src/libraries/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs index 42a8711..35d2258 100644 --- a/src/libraries/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs +++ b/src/libraries/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs @@ -313,7 +313,25 @@ namespace System.PrivateUri.Tests [InlineData("")] [InlineData(" ")] [InlineData("1")] - [InlineData(":1")] + [InlineData(":")] // leading single colon + [InlineData(":1")] // leading single colon + [InlineData(":1:2")] // leading single colon + [InlineData(":1:2:3")] // leading single colon + [InlineData(":1:2:3:4")] // leading single colon + [InlineData(":1:2:3:4:5")] // leading single colon + [InlineData(":1:2:3:4:5:6")] // leading single colon + [InlineData(":1:2:3:4:5:6:7")] // leading single colon + [InlineData(":1:2:3:4:5:6:7:8:9")] // leading single colon + [InlineData("::1:2:3:4:5:6:7:8")] // compressor with too many number groups + [InlineData("1::2:3:4:5:6:7:8")] // compressor with too many number groups + [InlineData("1:2::3:4:5:6:7:8")] // compressor with too many number groups + [InlineData("1:2:3::4:5:6:7:8")] // compressor with too many number groups + [InlineData("1:2:3:4::5:6:7:8")] // compressor with too many number groups + [InlineData("1:2:3:4:5::6:7:8")] // compressor with too many number groups + [InlineData("1:2:3:4:5:6::7:8")] // compressor with too many number groups + [InlineData("1:2:3:4:5:6:7::8")] // compressor with too many number groups + [InlineData("1:2:3:4:5:6:7:8::")] // compressor with too many number groups + [InlineData("::1:2:3:4:5:6:7:8:9")] // compressor with too many number groups [InlineData("1:")] [InlineData("::1 ")] [InlineData(" ::1")] @@ -333,6 +351,15 @@ namespace System.PrivateUri.Tests ParseBadIPv6Address(address); } + [Theory] + [InlineData(":1:2:3:4:5:6:7:8")] // leading single colon + [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Full framework machines are not yet patched with the fix for this")] + public void UriIPv6Host_BadAddress_SkipOnFramework(string address) + { + ParseBadIPv6Address(address); + } + + #region Helpers private void ParseIPv6Address(string ipv6String) -- 2.7.4