From 22ba36d5b0e78c984d8e0b24ae389cabd345ec30 Mon Sep 17 00:00:00 2001 From: Drew Kersnar <18474647+dakersnar@users.noreply.github.com> Date: Tue, 9 Aug 2022 17:13:00 -0500 Subject: [PATCH] Fix overflow check for parsing format strings (#72647) * WIP: fix overflow checking * Change limit to 9 digits * Fixed the same bug in other places * Remove mistake include * Move expensive tests to OuterLoop --- .../Globalization/FormatProvider.Number.cs | 10 ++--- .../src/System/Number.Formatting.cs | 10 ++--- .../src/System/Numerics/BigNumber.cs | 24 +++++------ .../BigInteger/BigIntegerToStringTests.cs | 40 +++++++++++++++++++ .../tests/System/DoubleTests.cs | 18 +++++++++ 5 files changed, 80 insertions(+), 22 deletions(-) diff --git a/src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs b/src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs index 8636878c895..0e11b9b186d 100644 --- a/src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs +++ b/src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs @@ -700,19 +700,19 @@ internal static char ParseFormatSpecifier(ReadOnlySpan format, out int dig } } - // Fallback for symbol and any length digits. The digits value must be >= 0 && <= 99, - // but it can begin with any number of 0s, and thus we may need to check more than two + // Fallback for symbol and any length digits. The digits value must be >= 0 && <= 999_999_999, + // but it can begin with any number of 0s, and thus we may need to check more than 9 // digits. Further, for compat, we need to stop when we hit a null char. int n = 0; int i = 1; while ((uint)i < (uint)format.Length && char.IsAsciiDigit(format[i])) { - int temp = (n * 10) + format[i++] - '0'; - if (temp < n) + // Check if we are about to overflow past our limit of 9 digits + if (n >= 100_000_000) { throw new FormatException(SR.Argument_BadFormatSpecifier); } - n = temp; + n = ((n * 10) + format[i++] - '0'); } // If we're at the end of the digits rather than having stopped because we hit something diff --git a/src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs b/src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs index d84220ef888..f81c6ac437a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs @@ -2318,19 +2318,19 @@ internal static unsafe char ParseFormatSpecifier(ReadOnlySpan format, out } } - // Fallback for symbol and any length digits. The digits value must be >= 0 && <= 99, - // but it can begin with any number of 0s, and thus we may need to check more than two + // Fallback for symbol and any length digits. The digits value must be >= 0 && <= 999_999_999, + // but it can begin with any number of 0s, and thus we may need to check more than 9 // digits. Further, for compat, we need to stop when we hit a null char. int n = 0; int i = 1; while ((uint)i < (uint)format.Length && char.IsAsciiDigit(format[i])) { - int temp = ((n * 10) + format[i++] - '0'); - if (temp < n) + // Check if we are about to overflow past our limit of 9 digits + if (n >= 100_000_000) { throw new FormatException(SR.Argument_BadFormatSpecifier); } - n = temp; + n = ((n * 10) + format[i++] - '0'); } // If we're at the end of the digits rather than having stopped because we hit something diff --git a/src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs b/src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs index 6cad9fe0ffa..ff6fbca6018 100644 --- a/src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs +++ b/src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs @@ -854,7 +854,6 @@ void MultiplyAdd(ref Span currentBuffer, uint multiplier, uint addValue) } } - // This function is consistent with VM\COMNumber.cpp!COMNumber::ParseFormatSpecifier internal static char ParseFormatSpecifier(ReadOnlySpan format, out int digits) { digits = -1; @@ -867,22 +866,23 @@ internal static char ParseFormatSpecifier(ReadOnlySpan format, out int dig char ch = format[i]; if (char.IsAsciiLetter(ch)) { + // The digits value must be >= 0 && <= 999_999_999, + // but it can begin with any number of 0s, and thus we may need to check more than 9 + // digits. Further, for compat, we need to stop when we hit a null char. i++; - int n = -1; - - if (i < format.Length && char.IsAsciiDigit(format[i])) + int n = 0; + while ((uint)i < (uint)format.Length && char.IsAsciiDigit(format[i])) { - n = format[i++] - '0'; - while (i < format.Length && char.IsAsciiDigit(format[i])) + // Check if we are about to overflow past our limit of 9 digits + if (n >= 100_000_000) { - int temp = n * 10 + (format[i++] - '0'); - if (temp < n) - { - throw new FormatException(SR.Argument_BadFormatSpecifier); - } - n = temp; + throw new FormatException(SR.Argument_BadFormatSpecifier); } + n = ((n * 10) + format[i++] - '0'); } + + // If we're at the end of the digits rather than having stopped because we hit something + // other than a digit or overflowed, return the standard format info. if (i >= format.Length || format[i] == '\0') { digits = n; diff --git a/src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs b/src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs index 35949f76935..78b47e96778 100644 --- a/src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs +++ b/src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs @@ -439,6 +439,46 @@ public static void CustomFormatPerMille() RunCustomFormatToStringTests(s_random, "#\u2030000000", CultureInfo.CurrentCulture.NumberFormat.NegativeSign, 6, PerMilleSymbolFormatter); } + [Fact] + public static void ToString_InvalidFormat_ThrowsFormatException() + { + BigInteger b = new BigInteger(123456789000m); + + // Format precision limit is 999_999_999 (9 digits). Anything larger should throw. + // Check ParseFormatSpecifier in FormatProvider.Number.cs with `E` format + Assert.Throws(() => b.ToString("E" + int.MaxValue.ToString())); + long intMaxPlus1 = (long)int.MaxValue + 1; + string intMaxPlus1String = intMaxPlus1.ToString(); + Assert.Throws(() => b.ToString("E" + intMaxPlus1String)); + Assert.Throws(() => b.ToString("E4772185890")); + Assert.Throws(() => b.ToString("E1000000000")); + Assert.Throws(() => b.ToString("E000001000000000")); + + // Check ParseFormatSpecifier in BigNumber.cs with `G` format + Assert.Throws(() => b.ToString("G" + int.MaxValue.ToString())); + Assert.Throws(() => b.ToString("G" + intMaxPlus1String)); + Assert.Throws(() => b.ToString("G4772185890")); + Assert.Throws(() => b.ToString("G1000000000")); + Assert.Throws(() => b.ToString("G000001000000000")); + } + + [Fact] + [OuterLoop("Takes a long time, allocates a lot of memory")] + public static void ToString_ValidLargeFormat() + { + BigInteger b = new BigInteger(123456789000m); + + // Format precision limit is 999_999_999 (9 digits). Anything larger should throw. + + // Check ParseFormatSpecifier in FormatProvider.Number.cs with `E` format + b.ToString("E999999999"); // Should not throw + b.ToString("E00000999999999"); // Should not throw + + // Check ParseFormatSpecifier in BigNumber.cs with `G` format + b.ToString("G999999999"); // Should not throw + b.ToString("G00000999999999"); // Should not throw + } + private static void RunSimpleProviderToStringTests(Random random, string format, NumberFormatInfo provider, int precision, StringFormatter formatter) { string test; diff --git a/src/libraries/System.Runtime/tests/System/DoubleTests.cs b/src/libraries/System.Runtime/tests/System/DoubleTests.cs index 63232f8f505..660fd463f32 100644 --- a/src/libraries/System.Runtime/tests/System/DoubleTests.cs +++ b/src/libraries/System.Runtime/tests/System/DoubleTests.cs @@ -827,9 +827,27 @@ public static void ToString_InvalidFormat_ThrowsFormatException() double d = 123.0; Assert.Throws(() => d.ToString("Y")); // Invalid format Assert.Throws(() => d.ToString("Y", null)); // Invalid format + + // Format precision limit is 999_999_999 (9 digits). Anything larger should throw. + Assert.Throws(() => d.ToString("E" + int.MaxValue.ToString())); long intMaxPlus1 = (long)int.MaxValue + 1; string intMaxPlus1String = intMaxPlus1.ToString(); Assert.Throws(() => d.ToString("E" + intMaxPlus1String)); + Assert.Throws(() => d.ToString("E4772185890")); + Assert.Throws(() => d.ToString("E1000000000")); + Assert.Throws(() => d.ToString("E000001000000000")); + } + + [Fact] + [OuterLoop("Takes a long time, allocates a lot of memory")] + public static void ToString_ValidLargeFormat() + { + double d = 123.0; + + // Format precision limit is 999_999_999 (9 digits). Anything larger should throw. + d.ToString("E999999999"); // Should not throw + d.ToString("E00000999999999"); // Should not throw + } [Theory] -- GitLab