Fix Number.ParseNumber to not assume '\0' at the end of a span (#17808)

* Fix Number.ParseNumber to not assume '\0' at the end of a span

This routine was written for parsing strings, which are implicitly null-terminated, and it doesn't factor in string length but instead uses tricks to exit loops when the next character is null.  Now that the routine is also used for spans, this is very problematic, as spans need not be null terminated, and generally aren't when they represent slices, and expecting a null termination like this can result in walking off the end of valid memory.

I would like to see all of this code rewritten to use span.  In the interim, though, as a short-term fix I've changed all dereferences of the current position to compare against the length of the span (or, rather, a pointer to the end), and pretend that a null terminator was found if we've hit the end.

* Address PR feedback

diff --git a/src/mscorlib/shared/System/Number.Parsing.cs b/src/mscorlib/shared/System/Number.Parsing.cs
index 2ae0618..936a826 100644 (file)
--- a/src/mscorlib/shared/System/Number.Parsing.cs
+++ b/src/mscorlib/shared/System/Number.Parsing.cs
@@ -396,8 +396,12 @@ namespace System
             return i;
         }
 
-        private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref NumberBuffer number, NumberFormatInfo numfmt, bool parseDecimal)
+        private static unsafe bool ParseNumber(ref char* str, char* strEnd, NumberStyles options, ref NumberBuffer number, NumberFormatInfo numfmt, bool parseDecimal)
         {
+            Debug.Assert(str != null);
+            Debug.Assert(strEnd != null);
+            Debug.Assert(str <= strEnd);
+
             const int StateSign = 0x0001;
             const int StateParens = 0x0002;
             const int StateDigits = 0x0004;
@@ -430,7 +434,7 @@ namespace System
 
             int state = 0;
             char* p = str;
-            char ch = *p;
+            char ch = p < strEnd ? *p : '\0';
             char* next;
 
             while (true)
@@ -439,7 +443,7 @@ namespace System
                 // "-Kr 1231.47" is legal but "- 1231.47" is not.
                 if (!IsWhite(ch) || (options & NumberStyles.AllowLeadingWhite) == 0 || ((state & StateSign) != 0 && ((state & StateCurrency) == 0 && numfmt.NumberNegativePattern != 2)))
                 {
-                    if ((((options & NumberStyles.AllowLeadingSign) != 0) && (state & StateSign) == 0) && ((next = MatchChars(p, numfmt.PositiveSign)) != null || ((next = MatchChars(p, numfmt.NegativeSign)) != null && (number.sign = true))))
+                    if ((((options & NumberStyles.AllowLeadingSign) != 0) && (state & StateSign) == 0) && ((next = MatchChars(p, strEnd, numfmt.PositiveSign)) != null || ((next = MatchChars(p, strEnd, numfmt.NegativeSign)) != null && (number.sign = true))))
                     {
                         state |= StateSign;
                         p = next - 1;
@@ -449,7 +453,7 @@ namespace System
                         state |= StateSign | StateParens;
                         number.sign = true;
                     }
-                    else if (currSymbol != null && (next = MatchChars(p, currSymbol)) != null)
+                    else if (currSymbol != null && (next = MatchChars(p, strEnd, currSymbol)) != null)
                     {
                         state |= StateCurrency;
                         currSymbol = null;
@@ -462,7 +466,7 @@ namespace System
                         break;
                     }
                 }
-                ch = *++p;
+                ch = ++p < strEnd ? *p : '\0';
             }
             int digCount = 0;
             int digEnd = 0;
@@ -493,12 +497,12 @@ namespace System
                         number.scale--;
                     }
                 }
-                else if (((options & NumberStyles.AllowDecimalPoint) != 0) && ((state & StateDecimal) == 0) && ((next = MatchChars(p, decSep)) != null || ((parsingCurrency) && (state & StateCurrency) == 0) && (next = MatchChars(p, numfmt.NumberDecimalSeparator)) != null))
+                else if (((options & NumberStyles.AllowDecimalPoint) != 0) && ((state & StateDecimal) == 0) && ((next = MatchChars(p, strEnd, decSep)) != null || ((parsingCurrency) && (state & StateCurrency) == 0) && (next = MatchChars(p, strEnd, numfmt.NumberDecimalSeparator)) != null))
                 {
                     state |= StateDecimal;
                     p = next - 1;
                 }
-                else if (((options & NumberStyles.AllowThousands) != 0) && ((state & StateDigits) != 0) && ((state & StateDecimal) == 0) && ((next = MatchChars(p, groupSep)) != null || ((parsingCurrency) && (state & StateCurrency) == 0) && (next = MatchChars(p, numfmt.NumberGroupSeparator)) != null))
+                else if (((options & NumberStyles.AllowThousands) != 0) && ((state & StateDigits) != 0) && ((state & StateDecimal) == 0) && ((next = MatchChars(p, strEnd, groupSep)) != null || ((parsingCurrency) && (state & StateCurrency) == 0) && (next = MatchChars(p, strEnd, numfmt.NumberGroupSeparator)) != null))
                 {
                     p = next - 1;
                 }
@@ -506,7 +510,7 @@ namespace System
                 {
                     break;
                 }
-                ch = *++p;
+                ch = ++p < strEnd ? *p : '\0';
             }
 
             bool negExp = false;
@@ -517,14 +521,14 @@ namespace System
                 if ((ch == 'E' || ch == 'e') && ((options & NumberStyles.AllowExponent) != 0))
                 {
                     char* temp = p;
-                    ch = *++p;
-                    if ((next = MatchChars(p, numfmt.positiveSign)) != null)
+                    ch = ++p < strEnd ? *p : '\0';
+                    if ((next = MatchChars(p, strEnd, numfmt.positiveSign)) != null)
                     {
-                        ch = *(p = next);
+                        ch = (p = next) < strEnd ? *p : '\0';
                     }
-                    else if ((next = MatchChars(p, numfmt.negativeSign)) != null)
+                    else if ((next = MatchChars(p, strEnd, numfmt.negativeSign)) != null)
                     {
-                        ch = *(p = next);
+                        ch = (p = next) < strEnd ? *p : '\0';
                         negExp = true;
                     }
                     if (ch >= '0' && ch <= '9')
@@ -533,13 +537,13 @@ namespace System
                         do
                         {
                             exp = exp * 10 + (ch - '0');
-                            ch = *++p;
+                            ch = ++p < strEnd ? *p : '\0';
                             if (exp > 1000)
                             {
                                 exp = 9999;
                                 while (ch >= '0' && ch <= '9')
                                 {
-                                    ch = *++p;
+                                    ch = ++p < strEnd ? *p : '\0';
                                 }
                             }
                         } while (ch >= '0' && ch <= '9');
@@ -552,14 +556,14 @@ namespace System
                     else
                     {
                         p = temp;
-                        ch = *p;
+                        ch = p < strEnd ? *p : '\0';
                     }
                 }
                 while (true)
                 {
                     if (!IsWhite(ch) || (options & NumberStyles.AllowTrailingWhite) == 0)
                     {
-                        if (((options & NumberStyles.AllowTrailingSign) != 0 && ((state & StateSign) == 0)) && ((next = MatchChars(p, numfmt.PositiveSign)) != null || (((next = MatchChars(p, numfmt.NegativeSign)) != null) && (number.sign = true))))
+                        if (((options & NumberStyles.AllowTrailingSign) != 0 && ((state & StateSign) == 0)) && ((next = MatchChars(p, strEnd, numfmt.PositiveSign)) != null || (((next = MatchChars(p, strEnd, numfmt.NegativeSign)) != null) && (number.sign = true))))
                         {
                             state |= StateSign;
                             p = next - 1;
@@ -568,7 +572,7 @@ namespace System
                         {
                             state &= ~StateParens;
                         }
-                        else if (currSymbol != null && (next = MatchChars(p, currSymbol)) != null)
+                        else if (currSymbol != null && (next = MatchChars(p, strEnd, currSymbol)) != null)
                         {
                             currSymbol = null;
                             p = next - 1;
@@ -578,7 +582,7 @@ namespace System
                             break;
                         }
                     }
-                    ch = *++p;
+                    ch = ++p < strEnd ? *p : '\0';
                 }
                 if ((state & StateParens) == 0)
                 {
@@ -859,7 +863,7 @@ namespace System
             fixed (char* stringPointer = &MemoryMarshal.GetReference(str))
             {
                 char* p = stringPointer;
-                if (!ParseNumber(ref p, options, ref number, info, parseDecimal)
+                if (!ParseNumber(ref p, p + str.Length, options, ref number, info, parseDecimal)
                     || (p - stringPointer < str.Length && !TrailingZeros(str, (int)(p - stringPointer))))
                 {
                     throw new FormatException(SR.Format_InvalidString);
@@ -873,7 +877,7 @@ namespace System
             fixed (char* stringPointer = &MemoryMarshal.GetReference(str))
             {
                 char* p = stringPointer;
-                if (!ParseNumber(ref p, options, ref number, numfmt, parseDecimal)
+                if (!ParseNumber(ref p, p + str.Length, options, ref number, numfmt, parseDecimal)
                     || (p - stringPointer < str.Length && !TrailingZeros(str, (int)(p - stringPointer))))
                 {
                     return false;
@@ -897,17 +901,17 @@ namespace System
             return true;
         }
 
-        private static unsafe char* MatchChars(char* p, string str)
+        private static unsafe char* MatchChars(char* p, char* pEnd, string str)
         {
             fixed (char* stringPointer = str)
             {
-                return MatchChars(p, stringPointer);
+                return MatchChars(p, pEnd, stringPointer);
             }
         }
 
-        private static unsafe char* MatchChars(char* p, char* str)
+        private static unsafe char* MatchChars(char* p, char* pEnd, char* str)
         {
-            Debug.Assert(p != null && str != null);
+            Debug.Assert(p != null && pEnd != null && p <= pEnd && str != null);
 
             if (*str == '\0')
             {
@@ -917,8 +921,13 @@ namespace System
             // We only hurt the failure case
             // This fix is for French or Kazakh cultures. Since a user cannot type 0xA0 as a
             // space character we use 0x20 space character instead to mean the same.
-            while (*p == *str || (*str == '\u00a0' && *p == '\u0020'))
+            while (true)
             {
+                char cp = p < pEnd ? *p : '\0';
+                if (cp != *str && !(*str == '\u00a0' && cp == '\u0020'))
+                {
+                    break;
+                }
                 p++;
                 str++;
                 if (*str == '\0') return p;