From 175b7e85a7a3a64b4735f3fca626eb2caf61315e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 5 May 2023 14:33:09 +0200 Subject: [PATCH 1/7] Remove AO from a couple of SpanHelpers methods --- .../System.Private.CoreLib/src/System/SpanHelpers.Byte.cs | 1 - .../System.Private.CoreLib/src/System/SpanHelpers.Char.cs | 1 - 2 files changed, 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs index c3d7cd2cd5071..a50c49bd6a1a0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs +++ b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs @@ -342,7 +342,6 @@ private static void ThrowMustBeNullTerminatedString() // IndexOfNullByte processes memory in aligned chunks, and thus it won't crash even if it accesses memory beyond the null terminator. // This behavior is an implementation detail of the runtime and callers outside System.Private.CoreLib must not depend on it. - [MethodImpl(MethodImplOptions.AggressiveOptimization)] internal static unsafe int IndexOfNullByte(byte* searchSpace) { const int Length = int.MaxValue; diff --git a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Char.cs b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Char.cs index f48659545b9d8..0f19bc197e58f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Char.cs +++ b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Char.cs @@ -422,7 +422,6 @@ public static unsafe int SequenceCompareTo(ref char first, int firstLength, ref // IndexOfNullCharacter processes memory in aligned chunks, and thus it won't crash even if it accesses memory beyond the null terminator. // This behavior is an implementation detail of the runtime and callers outside System.Private.CoreLib must not depend on it. - [MethodImpl(MethodImplOptions.AggressiveOptimization)] public static unsafe int IndexOfNullCharacter(char* searchSpace) { const char value = '\0'; From ec7503a6915f96d671cd61fea9c62af080d23ae7 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 7 May 2023 16:24:34 +0200 Subject: [PATCH 2/7] Address feedback --- .../src/System/String.Manipulation.cs | 50 ++++++++++++------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 7a8dac5644f83..f2d45f8d526cd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -38,14 +38,12 @@ internal static class SearchValuesStorage internal const int StackallocIntBufferSizeLimit = 128; internal const int StackallocCharBufferSizeLimit = 256; - private static void FillStringChecked(string dest, int destPos, string src) + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private static void CopyStringContent(string dest, int destPos, string src) { Debug.Assert(dest != null); Debug.Assert(src != null); - if (src.Length > dest.Length - destPos) - { - throw new IndexOutOfRangeException(); - } + Debug.Assert(src.Length <= dest.Length - destPos); Buffer.Memmove( destination: ref Unsafe.Add(ref dest._firstChar, destPos), @@ -53,6 +51,18 @@ private static void FillStringChecked(string dest, int destPos, string src) elementCount: (uint)src.Length); } + private static void CopyStringContentChecked(string dest, int destPos, string src) + { + Debug.Assert(dest != null); + Debug.Assert(src != null); + if (src.Length > dest.Length - destPos) + { + throw new IndexOutOfRangeException(); + } + + CopyStringContent(dest, destPos, src); + } + public static string Concat(object? arg0) => arg0?.ToString() ?? Empty; @@ -117,7 +127,7 @@ public static string Concat(params object?[] args) Debug.Assert(s != null); Debug.Assert(position <= totalLength - s.Length, "We didn't allocate enough space for the result string!"); - FillStringChecked(result, position, s); + CopyStringContentChecked(result, position, s); position += s.Length; } @@ -255,10 +265,14 @@ public static string Concat(string? str0, string? str1) int str0Length = str0.Length; - string result = FastAllocateString(str0Length + str1.Length); + // totalLength will never overflow to a non-negative number + // and the negative number will trigger OOM in FastAllocateString. + int totalLength = str0Length + str1.Length; + + string result = FastAllocateString(totalLength); - FillStringChecked(result, 0, str0); - FillStringChecked(result, str0Length, str1); + CopyStringContent(result, 0, str0); + CopyStringContent(result, str0Length, str1); return result; } @@ -283,9 +297,9 @@ public static string Concat(string? str0, string? str1, string? str2) int totalLength = str0.Length + str1.Length + str2.Length; string result = FastAllocateString(totalLength); - FillStringChecked(result, 0, str0); - FillStringChecked(result, str0.Length, str1); - FillStringChecked(result, str0.Length + str1.Length, str2); + CopyStringContentChecked(result, 0, str0); + CopyStringContentChecked(result, str0.Length, str1); + CopyStringContentChecked(result, str0.Length + str1.Length, str2); return result; } @@ -315,10 +329,10 @@ public static string Concat(string? str0, string? str1, string? str2, string? st int totalLength = str0.Length + str1.Length + str2.Length + str3.Length; string result = FastAllocateString(totalLength); - FillStringChecked(result, 0, str0); - FillStringChecked(result, str0.Length, str1); - FillStringChecked(result, str0.Length + str1.Length, str2); - FillStringChecked(result, str0.Length + str1.Length + str2.Length, str3); + CopyStringContentChecked(result, 0, str0); + CopyStringContentChecked(result, str0.Length, str1); + CopyStringContentChecked(result, str0.Length + str1.Length, str2); + CopyStringContentChecked(result, str0.Length + str1.Length + str2.Length, str3); return result; } @@ -470,7 +484,7 @@ public static string Concat(params string?[] values) break; } - FillStringChecked(result, copiedLength, value); + CopyStringContentChecked(result, copiedLength, value); copiedLength += valueLen; } } @@ -970,7 +984,7 @@ private static string JoinCore(ReadOnlySpan separator, ReadOnlySpan Date: Mon, 8 May 2023 20:48:01 +0200 Subject: [PATCH 3/7] Address feedback --- .../src/Resources/Strings.resx | 3 + .../src/System/String.Manipulation.cs | 68 +++++++++++-------- .../src/System/ThrowHelper.cs | 6 ++ 3 files changed, 49 insertions(+), 28 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx index 929ee6132d851..da4412098aa6f 100644 --- a/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx +++ b/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx @@ -4220,4 +4220,7 @@ An unexpected state object was encountered. This is usually a sign of a bug in async method custom infrastructure, such as a custom awaiter or IValueTaskSource implementation. + + The resulting string is too long + diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index f2d45f8d526cd..03b7ba1d1f6fd 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -51,18 +51,6 @@ private static void CopyStringContent(string dest, int destPos, string src) elementCount: (uint)src.Length); } - private static void CopyStringContentChecked(string dest, int destPos, string src) - { - Debug.Assert(dest != null); - Debug.Assert(src != null); - if (src.Length > dest.Length - destPos) - { - throw new IndexOutOfRangeException(); - } - - CopyStringContent(dest, destPos, src); - } - public static string Concat(object? arg0) => arg0?.ToString() ?? Empty; @@ -127,8 +115,14 @@ public static string Concat(params object?[] args) Debug.Assert(s != null); Debug.Assert(position <= totalLength - s.Length, "We didn't allocate enough space for the result string!"); - CopyStringContentChecked(result, position, s); + CopyStringContent(result, position, s); position += s.Length; + + // Check for overflow + if (position < 0) + { + ThrowHelper.ThrowOutOfMemoryException_StringTooLong(); + } } return result; @@ -265,10 +259,14 @@ public static string Concat(string? str0, string? str1) int str0Length = str0.Length; - // totalLength will never overflow to a non-negative number - // and the negative number will trigger OOM in FastAllocateString. int totalLength = str0Length + str1.Length; + // Can't overflow to a positive number so just check < 0 + if (totalLength < 0) + { + ThrowHelper.ThrowOutOfMemoryException_StringTooLong(); + } + string result = FastAllocateString(totalLength); CopyStringContent(result, 0, str0); @@ -294,12 +292,19 @@ public static string Concat(string? str0, string? str1, string? str2) return Concat(str0, str1); } - int totalLength = str0.Length + str1.Length + str2.Length; + // It can overflow to a positive number so we accumulate the total length as a long. + long totalLength64 = str0.Length + str1.Length + str2.Length; - string result = FastAllocateString(totalLength); - CopyStringContentChecked(result, 0, str0); - CopyStringContentChecked(result, str0.Length, str1); - CopyStringContentChecked(result, str0.Length + str1.Length, str2); + int totalLength32 = (int)totalLength64; + if (totalLength64 != totalLength32) + { + ThrowHelper.ThrowOutOfMemoryException_StringTooLong(); + } + + string result = FastAllocateString(totalLength32); + CopyStringContent(result, 0, str0); + CopyStringContent(result, str0.Length, str1); + CopyStringContent(result, str0.Length + str1.Length, str2); return result; } @@ -326,13 +331,20 @@ public static string Concat(string? str0, string? str1, string? str2, string? st return Concat(str0, str1, str2); } - int totalLength = str0.Length + str1.Length + str2.Length + str3.Length; + // It can overflow to a positive number so we accumulate the total length as a long. + long totalLength64 = str0.Length + str1.Length + str2.Length + str3.Length; + + int totalLength32 = (int)totalLength64; + if (totalLength64 != totalLength32) + { + ThrowHelper.ThrowOutOfMemoryException_StringTooLong(); + } - string result = FastAllocateString(totalLength); - CopyStringContentChecked(result, 0, str0); - CopyStringContentChecked(result, str0.Length, str1); - CopyStringContentChecked(result, str0.Length + str1.Length, str2); - CopyStringContentChecked(result, str0.Length + str1.Length + str2.Length, str3); + string result = FastAllocateString(totalLength32); + CopyStringContent(result, 0, str0); + CopyStringContent(result, str0.Length, str1); + CopyStringContent(result, str0.Length + str1.Length, str2); + CopyStringContent(result, str0.Length + str1.Length + str2.Length, str3); return result; } @@ -484,7 +496,7 @@ public static string Concat(params string?[] values) break; } - CopyStringContentChecked(result, copiedLength, value); + CopyStringContent(result, copiedLength, value); copiedLength += valueLen; } } @@ -984,7 +996,7 @@ private static string JoinCore(ReadOnlySpan separator, ReadOnlySpan Date: Mon, 8 May 2023 20:48:46 +0200 Subject: [PATCH 4/7] remove [AI] --- .../System.Private.CoreLib/src/System/String.Manipulation.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 03b7ba1d1f6fd..521b199308600 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -38,7 +38,6 @@ internal static class SearchValuesStorage internal const int StackallocIntBufferSizeLimit = 128; internal const int StackallocCharBufferSizeLimit = 256; - [MethodImpl(MethodImplOptions.AggressiveInlining)] private static void CopyStringContent(string dest, int destPos, string src) { Debug.Assert(dest != null); From 8ff9a62e88976fa9a5d55ab0da39d6e285b7a6c2 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 8 May 2023 21:32:43 +0200 Subject: [PATCH 5/7] Address feedback --- .../src/System/String.Manipulation.cs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 521b199308600..d49fb6ea02a73 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -94,7 +94,7 @@ public static string Concat(params object?[] args) if (totalLength < 0) // Check for a positive overflow { - throw new OutOfMemoryException(); + ThrowHelper.ThrowOutOfMemoryException_StringTooLong(); } } @@ -116,12 +116,6 @@ public static string Concat(params object?[] args) CopyStringContent(result, position, s); position += s.Length; - - // Check for overflow - if (position < 0) - { - ThrowHelper.ThrowOutOfMemoryException_StringTooLong(); - } } return result; @@ -292,7 +286,7 @@ public static string Concat(string? str0, string? str1, string? str2) } // It can overflow to a positive number so we accumulate the total length as a long. - long totalLength64 = str0.Length + str1.Length + str2.Length; + long totalLength64 = (long)str0.Length + (long)str1.Length + (long)str2.Length; int totalLength32 = (int)totalLength64; if (totalLength64 != totalLength32) @@ -331,7 +325,7 @@ public static string Concat(string? str0, string? str1, string? str2, string? st } // It can overflow to a positive number so we accumulate the total length as a long. - long totalLength64 = str0.Length + str1.Length + str2.Length + str3.Length; + long totalLength64 = (long)str0.Length + (long)str1.Length + (long)str2.Length + (long)str3.Length; int totalLength32 = (int)totalLength64; if (totalLength64 != totalLength32) From f3687527d5460588289f9f76b5812ed6208c20ea Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 8 May 2023 21:38:28 +0200 Subject: [PATCH 6/7] Address feedback --- .../src/System/String.Manipulation.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index d49fb6ea02a73..00d67b7f8e716 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -326,7 +326,7 @@ public static string Concat(string? str0, string? str1, string? str2, string? st // It can overflow to a positive number so we accumulate the total length as a long. long totalLength64 = (long)str0.Length + (long)str1.Length + (long)str2.Length + (long)str3.Length; - + int totalLength32 = (int)totalLength64; if (totalLength64 != totalLength32) { @@ -466,7 +466,7 @@ public static string Concat(params string?[] values) // If it's too long, fail, or if it's empty, return an empty string. if (totalLengthLong > int.MaxValue) { - throw new OutOfMemoryException(); + ThrowHelper.ThrowOutOfMemoryException_StringTooLong(); } int totalLength = (int)totalLengthLong; if (totalLength == 0) @@ -950,7 +950,7 @@ private static string JoinCore(ReadOnlySpan separator, ReadOnlySpan int.MaxValue) { - ThrowHelper.ThrowOutOfMemoryException(); + ThrowHelper.ThrowOutOfMemoryException_StringTooLong(); } int totalLength = (int)totalSeparatorsLength; @@ -962,7 +962,7 @@ private static string JoinCore(ReadOnlySpan separator, ReadOnlySpan int.MaxValue) - throw new OutOfMemoryException(); + ThrowHelper.ThrowOutOfMemoryException_StringTooLong(); string dst = FastAllocateString((int)dstLength); Span dstSpan = new Span(ref dst._firstChar, dst.Length); From 49509cd50e65e298f7de3aacc88350b354526184 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 8 May 2023 23:41:34 +0200 Subject: [PATCH 7/7] Switch to int.MaxValue --- .../src/System/String.Manipulation.cs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 00d67b7f8e716..77251e080cb55 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -286,15 +286,14 @@ public static string Concat(string? str0, string? str1, string? str2) } // It can overflow to a positive number so we accumulate the total length as a long. - long totalLength64 = (long)str0.Length + (long)str1.Length + (long)str2.Length; + long totalLength = (long)str0.Length + (long)str1.Length + (long)str2.Length; - int totalLength32 = (int)totalLength64; - if (totalLength64 != totalLength32) + if (totalLength > int.MaxValue) { ThrowHelper.ThrowOutOfMemoryException_StringTooLong(); } - string result = FastAllocateString(totalLength32); + string result = FastAllocateString((int)totalLength); CopyStringContent(result, 0, str0); CopyStringContent(result, str0.Length, str1); CopyStringContent(result, str0.Length + str1.Length, str2); @@ -325,15 +324,14 @@ public static string Concat(string? str0, string? str1, string? str2, string? st } // It can overflow to a positive number so we accumulate the total length as a long. - long totalLength64 = (long)str0.Length + (long)str1.Length + (long)str2.Length + (long)str3.Length; + long totalLength = (long)str0.Length + (long)str1.Length + (long)str2.Length + (long)str3.Length; - int totalLength32 = (int)totalLength64; - if (totalLength64 != totalLength32) + if (totalLength > int.MaxValue) { ThrowHelper.ThrowOutOfMemoryException_StringTooLong(); } - string result = FastAllocateString(totalLength32); + string result = FastAllocateString((int)totalLength); CopyStringContent(result, 0, str0); CopyStringContent(result, str0.Length, str1); CopyStringContent(result, str0.Length + str1.Length, str2);