From: Stephen Toub Date: Fri, 27 Apr 2018 22:37:09 +0000 (-0700) Subject: Fix Number.ParseNumber to not assume '\0' at the end of a span (dotnet/coreclr#17808) X-Git-Tag: submit/tizen/20210909.063632~11030^2~4927 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=bbf025f464c523e1451454b4ed454309340d9e56;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Fix Number.ParseNumber to not assume '\0' at the end of a span (dotnet/coreclr#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 Commit migrated from https://github.com/dotnet/coreclr/commit/d0a55af49fb03d064f7654ef6ee664b3eb384268 --- diff --git a/src/coreclr/src/mscorlib/shared/System/Number.Parsing.cs b/src/coreclr/src/mscorlib/shared/System/Number.Parsing.cs index 2ae06180d26..936a826c937 100644 --- a/src/coreclr/src/mscorlib/shared/System/Number.Parsing.cs +++ b/src/coreclr/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;