From 0f62ec51f5af289b1ddd4e1069d1d72ef78d1555 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Mon, 21 Aug 2017 11:53:11 -0400 Subject: [PATCH] Mitigate regressions in IPAddress Commit migrated from https://github.com/dotnet/corefx/commit/a13c1ddd4945aaee96d1ab16e7218472572afa8f --- .../src/System/Net/IPAddress.cs | 123 +++++++----------- .../src/System/Net/IPAddressParser.cs | 54 +++----- 2 files changed, 61 insertions(+), 116 deletions(-) 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 3a59243b9f1455..1dafaf0d8bbb14 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/IPAddress.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.Net.Sockets; +using System.Runtime.CompilerServices; namespace System.Net { @@ -122,7 +123,7 @@ public IPAddress(long newAddress) /// /// public IPAddress(byte[] address, long scopeid) : - this(new ReadOnlySpan(address ?? throw new ArgumentNullException(nameof(address))), scopeid) + this(new ReadOnlySpan(address ?? ThrowAddressNullException()), scopeid) { } @@ -150,27 +151,6 @@ public IPAddress(ReadOnlySpan address, long scopeid) PrivateScopeId = (uint)scopeid; } - internal unsafe IPAddress(byte* address, int addressLength, long scopeid) - { - Debug.Assert(address != null); - Debug.Assert(addressLength == IPAddressParserStatics.IPv6AddressBytes); - - // Consider: Since scope is only valid for link-local and site-local - // addresses we could implement some more robust checking here - if (scopeid < 0 || scopeid > 0x00000000FFFFFFFF) - { - throw new ArgumentOutOfRangeException(nameof(scopeid)); - } - - _numbers = new ushort[NumberOfLabels]; - for (int i = 0; i < NumberOfLabels; i++) - { - _numbers[i] = (ushort)(address[i * 2] * 256 + address[i * 2 + 1]); - } - - PrivateScopeId = (uint)scopeid; - } - internal unsafe IPAddress(ushort* numbers, int numbersLength, uint scopeid) { Debug.Assert(numbers != null); @@ -201,22 +181,17 @@ private IPAddress(ushort[] numbers, uint scopeid) /// /// public IPAddress(byte[] address) : - this(new ReadOnlySpan(address ?? throw new ArgumentNullException(nameof(address)))) + this(new ReadOnlySpan(address ?? ThrowAddressNullException())) { } public IPAddress(ReadOnlySpan address) { - if (address.Length != IPAddressParserStatics.IPv4AddressBytes && address.Length != IPAddressParserStatics.IPv6AddressBytes) - { - throw new ArgumentException(SR.dns_bad_ip_address, nameof(address)); - } - if (address.Length == IPAddressParserStatics.IPv4AddressBytes) { PrivateAddress = (uint)((address[3] << 24 | address[2] << 16 | address[1] << 8 | address[0]) & 0x0FFFFFFFF); } - else + else if (address.Length == IPAddressParserStatics.IPv6AddressBytes) { _numbers = new ushort[NumberOfLabels]; @@ -225,27 +200,9 @@ public IPAddress(ReadOnlySpan address) _numbers[i] = (ushort)(address[i * 2] * 256 + address[i * 2 + 1]); } } - } - - internal unsafe IPAddress(byte* address, int addressLength) - { - Debug.Assert(address != null); - Debug.Assert(addressLength > 0); - Debug.Assert( - addressLength == IPAddressParserStatics.IPv4AddressBytes || - addressLength == IPAddressParserStatics.IPv6AddressBytes); - - if (addressLength == IPAddressParserStatics.IPv4AddressBytes) - { - PrivateAddress = (uint)((address[3] << 24 | address[2] << 16 | address[1] << 8 | address[0]) & 0x0FFFFFFFF); - } else { - _numbers = new ushort[NumberOfLabels]; - for (int i = 0; i < NumberOfLabels; i++) - { - _numbers[i] = (ushort)(address[i * 2] * 256 + address[i * 2 + 1]); - } + throw new ArgumentException(SR.dns_bad_ip_address, nameof(address)); } } @@ -269,7 +226,7 @@ public static bool TryParse(string ipString, out IPAddress address) return false; } - address = IPAddressParser.Parse(ipString, tryParse: true); + address = IPAddressParser.Parse(ipString.AsReadOnlySpan(), tryParse: true); return (address != null); } @@ -286,7 +243,7 @@ public static IPAddress Parse(string ipString) throw new ArgumentNullException(nameof(ipString)); } - return IPAddressParser.Parse(ipString, tryParse: false); + return IPAddressParser.Parse(ipString.AsReadOnlySpan(), tryParse: false); } public static IPAddress Parse(ReadOnlySpan ipSpan) @@ -298,21 +255,13 @@ public bool TryWriteBytes(Span destination, out int bytesWritten) { if (IsIPv6) { - Debug.Assert(_numbers != null && _numbers.Length == NumberOfLabels); - if (destination.Length < IPAddressParserStatics.IPv6AddressBytes) { bytesWritten = 0; return false; } - int j = 0; - for (int i = 0; i < NumberOfLabels; i++) - { - destination[j++] = (byte)((_numbers[i] >> 8) & 0xFF); - destination[j++] = (byte)((_numbers[i]) & 0xFF); - } - + WriteIPv6Bytes(destination); bytesWritten = IPAddressParserStatics.IPv6AddressBytes; } else @@ -323,20 +272,35 @@ public bool TryWriteBytes(Span destination, out int bytesWritten) return false; } - uint address = PrivateAddress; - unchecked - { - destination[0] = (byte)(address); - destination[1] = (byte)(address >> 8); - destination[2] = (byte)(address >> 16); - destination[3] = (byte)(address >> 24); - } - + WriteIPv4Bytes(destination); bytesWritten = IPAddressParserStatics.IPv4AddressBytes; } return true; } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void WriteIPv6Bytes(Span destination) + { + Debug.Assert(_numbers != null && _numbers.Length == NumberOfLabels); + int j = 0; + for (int i = 0; i < NumberOfLabels; i++) + { + destination[j++] = (byte)((_numbers[i] >> 8) & 0xFF); + destination[j++] = (byte)((_numbers[i]) & 0xFF); + } + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void WriteIPv4Bytes(Span destination) + { + uint address = PrivateAddress; + destination[0] = (byte)(address); + destination[1] = (byte)(address >> 8); + destination[2] = (byte)(address >> 16); + destination[3] = (byte)(address >> 24); + } + /// /// /// Provides a copy of the IPAddress internals as an array of bytes. @@ -347,16 +311,16 @@ public byte[] GetAddressBytes() if (IsIPv6) { Debug.Assert(_numbers != null && _numbers.Length == NumberOfLabels); + byte[] bytes = new byte[IPAddressParserStatics.IPv6AddressBytes]; + WriteIPv6Bytes(bytes); + return bytes; + } + else + { + byte[] bytes = new byte[IPAddressParserStatics.IPv4AddressBytes]; + WriteIPv4Bytes(bytes); + return bytes; } - - int length = IsIPv6 ? IPAddressParserStatics.IPv6AddressBytes : IPAddressParserStatics.IPv4AddressBytes; - var bytes = new byte[length]; - - bool result = TryWriteBytes(new Span(bytes), out int bytesWritten); - - Debug.Assert(result); - - return bytes; } public AddressFamily AddressFamily @@ -476,7 +440,7 @@ public static bool IsLoopback(IPAddress address) { if (address == null) { - throw new ArgumentNullException(nameof(address)); + ThrowAddressNullException(); } if (address.IsIPv6) @@ -704,5 +668,8 @@ public IPAddress MapToIPv4() return new IPAddress(address); } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static byte[] ThrowAddressNullException() => throw new ArgumentNullException("address"); } } 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 825aa865666b9e..ba02a6278db191 100644 --- a/src/libraries/System.Net.Primitives/src/System/Net/IPAddressParser.cs +++ b/src/libraries/System.Net.Primitives/src/System/Net/IPAddressParser.cs @@ -11,43 +11,23 @@ namespace System.Net { internal class IPAddressParser { - internal static IPAddress Parse(string ipString, bool tryParse) - { - if (ipString == null) - { - if (tryParse) - { - return null; - } - throw new ArgumentNullException(nameof(ipString)); - } - - return ParseHelper(ipString.AsReadOnlySpan(), tryParse); - } + private const int MaxIPv4StringLength = 15; // 4 numbers separated by 3 periods, with up to 3 digits per number - internal static IPAddress Parse(ReadOnlySpan ipSpan, bool tryParse) - { - return ParseHelper(ipSpan, tryParse); - } - - internal static unsafe IPAddress ParseHelper(ReadOnlySpan ipSpan, bool tryParse) + internal static unsafe IPAddress Parse(ReadOnlySpan ipSpan, bool tryParse) { if (ipSpan.IndexOf(':') >= 0) { - // If the address string contains the colon character then it can only be an IPv6 address. - // This is valid because we don't support/parse a port specification at the end of an IPv4 address. + // 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]; if (Ipv6StringToAddress(ipSpan, numbers, IPAddressParserStatics.IPv6AddressShorts, out uint scope)) { return new IPAddress(numbers, IPAddressParserStatics.IPv6AddressShorts, scope); } } - else + else if (Ipv4StringToAddress(ipSpan, out long address)) { - if (Ipv4StringToAddress(ipSpan, out long address)) - { - return new IPAddress(address); - } + return new IPAddress(address); } if (tryParse) @@ -60,30 +40,28 @@ internal static unsafe IPAddress ParseHelper(ReadOnlySpan ipSpan, bool try internal static unsafe string IPv4AddressToString(uint address) { - const int MaxLength = 15; - char* addressString = stackalloc char[MaxLength]; - - int charsWritten = IPv4AddressToStringHelper(address, new Span(addressString, MaxLength)); - + char* addressString = stackalloc char[MaxIPv4StringLength]; + int charsWritten = IPv4AddressToStringHelper(address, addressString); return new string(addressString, 0, charsWritten); } internal static unsafe bool IPv4AddressToString(uint address, Span formatted, out int charsWritten) { - const int MaxLength = 15; - - if (formatted.Length < MaxLength) + if (formatted.Length < MaxIPv4StringLength) { charsWritten = 0; return false; } - - charsWritten = IPv4AddressToStringHelper(address, formatted); + + fixed (char* formattedPtr = &formatted.DangerousGetPinnableReference()) + { + charsWritten = IPv4AddressToStringHelper(address, formattedPtr); + } return true; } - internal static int IPv4AddressToStringHelper(uint address, Span addressString) + private static unsafe int IPv4AddressToStringHelper(uint address, char* addressString) { int offset = 0; @@ -162,7 +140,7 @@ internal static StringBuilder IPv6AddressToStringHelper(ushort[] address, uint s return buffer; } - private static void FormatIPv4AddressNumber(int number, Span addressString, ref int offset) + private static unsafe void FormatIPv4AddressNumber(int number, char* addressString, ref int offset) { // Math.DivRem has no overload for byte, assert here for safety Debug.Assert(number < 256);