Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit df62491

Browse files
stephentoubstephentoub
authored
Fix Number.ParseNumber to not assume '\0' at the end of a span (#17808) (#17820)
* 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
1 parent a226c35 commit df62491

File tree

1 file changed

+35
-26
lines changed

1 file changed

+35
-26
lines changed

‎src/mscorlib/shared/System/Number.Parsing.cs‎

Lines changed: 35 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -396,8 +396,12 @@ internal static unsafe ulong ParseUInt64(ReadOnlySpan<char> value, NumberStyles
396396
return i;
397397
}
398398

399-
private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref NumberBuffer number, NumberFormatInfo numfmt, bool parseDecimal)
399+
private static unsafe bool ParseNumber(ref char* str, char* strEnd, NumberStyles options, ref NumberBuffer number, NumberFormatInfo numfmt, bool parseDecimal)
400400
{
401+
Debug.Assert(str != null);
402+
Debug.Assert(strEnd != null);
403+
Debug.Assert(str <= strEnd);
404+
401405
const int StateSign = 0x0001;
402406
const int StateParens = 0x0002;
403407
const int StateDigits = 0x0004;
@@ -430,7 +434,7 @@ private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref
430434

431435
int state = 0;
432436
char* p = str;
433-
char ch = *p;
437+
char ch = p < strEnd ? *p : '\0';
434438
char* next;
435439

436440
while (true)
@@ -439,7 +443,7 @@ private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref
439443
// "-Kr 1231.47" is legal but "- 1231.47" is not.
440444
if (!IsWhite(ch) || (options & NumberStyles.AllowLeadingWhite) == 0 || ((state & StateSign) != 0 && ((state & StateCurrency) == 0 && numfmt.NumberNegativePattern != 2)))
441445
{
442-
if ((((options & NumberStyles.AllowLeadingSign) != 0) && (state & StateSign) == 0) && ((next = MatchChars(p, numfmt.PositiveSign)) != null || ((next = MatchChars(p, numfmt.NegativeSign)) != null && (number.sign = true))))
446+
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))))
443447
{
444448
state |= StateSign;
445449
p = next - 1;
@@ -449,7 +453,7 @@ private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref
449453
state |= StateSign | StateParens;
450454
number.sign = true;
451455
}
452-
else if (currSymbol != null && (next = MatchChars(p, currSymbol)) != null)
456+
else if (currSymbol != null && (next = MatchChars(p, strEnd, currSymbol)) != null)
453457
{
454458
state |= StateCurrency;
455459
currSymbol = null;
@@ -462,7 +466,7 @@ private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref
462466
break;
463467
}
464468
}
465-
ch = *++p;
469+
ch = ++p < strEnd ? *p : '\0';
466470
}
467471
int digCount = 0;
468472
int digEnd = 0;
@@ -493,20 +497,20 @@ private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref
493497
number.scale--;
494498
}
495499
}
496-
else if (((options & NumberStyles.AllowDecimalPoint) != 0) && ((state & StateDecimal) == 0) && ((next = MatchChars(p, decSep)) != null || ((parsingCurrency) && (state & StateCurrency) == 0) && (next = MatchChars(p, numfmt.NumberDecimalSeparator)) != null))
500+
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))
497501
{
498502
state |= StateDecimal;
499503
p = next - 1;
500504
}
501-
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))
505+
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))
502506
{
503507
p = next - 1;
504508
}
505509
else
506510
{
507511
break;
508512
}
509-
ch = *++p;
513+
ch = ++p < strEnd ? *p : '\0';
510514
}
511515

512516
bool negExp = false;
@@ -517,14 +521,14 @@ private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref
517521
if ((ch == 'E' || ch == 'e') && ((options & NumberStyles.AllowExponent) != 0))
518522
{
519523
char* temp = p;
520-
ch = *++p;
521-
if ((next = MatchChars(p, numfmt.positiveSign)) != null)
524+
ch = ++p < strEnd ? *p : '\0';
525+
if ((next = MatchChars(p, strEnd, numfmt.positiveSign)) != null)
522526
{
523-
ch = *(p = next);
527+
ch = (p = next) < strEnd ? *p : '\0';
524528
}
525-
else if ((next = MatchChars(p, numfmt.negativeSign)) != null)
529+
else if ((next = MatchChars(p, strEnd, numfmt.negativeSign)) != null)
526530
{
527-
ch = *(p = next);
531+
ch = (p = next) < strEnd ? *p : '\0';
528532
negExp = true;
529533
}
530534
if (ch >= '0' && ch <= '9')
@@ -533,13 +537,13 @@ private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref
533537
do
534538
{
535539
exp = exp * 10 + (ch - '0');
536-
ch = *++p;
540+
ch = ++p < strEnd ? *p : '\0';
537541
if (exp > 1000)
538542
{
539543
exp = 9999;
540544
while (ch >= '0' && ch <= '9')
541545
{
542-
ch = *++p;
546+
ch = ++p < strEnd ? *p : '\0';
543547
}
544548
}
545549
} while (ch >= '0' && ch <= '9');
@@ -552,14 +556,14 @@ private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref
552556
else
553557
{
554558
p = temp;
555-
ch = *p;
559+
ch = p < strEnd ? *p : '\0';
556560
}
557561
}
558562
while (true)
559563
{
560564
if (!IsWhite(ch) || (options & NumberStyles.AllowTrailingWhite) == 0)
561565
{
562-
if (((options & NumberStyles.AllowTrailingSign) != 0 && ((state & StateSign) == 0)) && ((next = MatchChars(p, numfmt.PositiveSign)) != null || (((next = MatchChars(p, numfmt.NegativeSign)) != null) && (number.sign = true))))
566+
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))))
563567
{
564568
state |= StateSign;
565569
p = next - 1;
@@ -568,7 +572,7 @@ private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref
568572
{
569573
state &= ~StateParens;
570574
}
571-
else if (currSymbol != null && (next = MatchChars(p, currSymbol)) != null)
575+
else if (currSymbol != null && (next = MatchChars(p, strEnd, currSymbol)) != null)
572576
{
573577
currSymbol = null;
574578
p = next - 1;
@@ -578,7 +582,7 @@ private static unsafe bool ParseNumber(ref char* str, NumberStyles options, ref
578582
break;
579583
}
580584
}
581-
ch = *++p;
585+
ch = ++p < strEnd ? *p : '\0';
582586
}
583587
if ((state & StateParens) == 0)
584588
{
@@ -859,7 +863,7 @@ private static unsafe void StringToNumber(ReadOnlySpan<char> str, NumberStyles o
859863
fixed (char* stringPointer = &MemoryMarshal.GetReference(str))
860864
{
861865
char* p = stringPointer;
862-
if (!ParseNumber(ref p, options, ref number, info, parseDecimal)
866+
if (!ParseNumber(ref p, p + str.Length, options, ref number, info, parseDecimal)
863867
|| (p - stringPointer < str.Length && !TrailingZeros(str, (int)(p - stringPointer))))
864868
{
865869
throw new FormatException(SR.Format_InvalidString);
@@ -873,7 +877,7 @@ internal static unsafe bool TryStringToNumber(ReadOnlySpan<char> str, NumberStyl
873877
fixed (char* stringPointer = &MemoryMarshal.GetReference(str))
874878
{
875879
char* p = stringPointer;
876-
if (!ParseNumber(ref p, options, ref number, numfmt, parseDecimal)
880+
if (!ParseNumber(ref p, p + str.Length, options, ref number, numfmt, parseDecimal)
877881
|| (p - stringPointer < str.Length && !TrailingZeros(str, (int)(p - stringPointer))))
878882
{
879883
return false;
@@ -897,17 +901,17 @@ private static bool TrailingZeros(ReadOnlySpan<char> s, int index)
897901
return true;
898902
}
899903

900-
private static unsafe char* MatchChars(char* p, string str)
904+
private static unsafe char* MatchChars(char* p, char* pEnd, string str)
901905
{
902906
fixed (char* stringPointer = str)
903907
{
904-
return MatchChars(p, stringPointer);
908+
return MatchChars(p, pEnd, stringPointer);
905909
}
906910
}
907911

908-
private static unsafe char* MatchChars(char* p, char* str)
912+
private static unsafe char* MatchChars(char* p, char* pEnd, char* str)
909913
{
910-
Debug.Assert(p != null && str != null);
914+
Debug.Assert(p != null && pEnd != null && p <= pEnd && str != null);
911915

912916
if (*str == '\0')
913917
{
@@ -917,8 +921,13 @@ private static bool TrailingZeros(ReadOnlySpan<char> s, int index)
917921
// We only hurt the failure case
918922
// This fix is for French or Kazakh cultures. Since a user cannot type 0xA0 as a
919923
// space character we use 0x20 space character instead to mean the same.
920-
while (*p == *str || (*str == '\u00a0' && *p == '\u0020'))
924+
while (true)
921925
{
926+
char cp = p < pEnd ? *p : '\0';
927+
if (cp != *str && !(*str == '\u00a0' && cp == '\u0020'))
928+
{
929+
break;
930+
}
922931
p++;
923932
str++;
924933
if (*str == '\0') return p;

0 commit comments

Comments
 (0)