From 9e497d89fb4ecf4ff73425879fe09a3ff3342bac Mon Sep 17 00:00:00 2001 From: Drew Kersnar <18474647+dakersnar@users.noreply.github.com> Date: Tue, 19 Jul 2022 17:19:52 -0500 Subject: [PATCH 1/5] WIP: fix overflow checking --- .../src/System/Number.Formatting.cs | 16 +++++++++++----- .../System.Runtime/tests/System/DoubleTests.cs | 3 +++ 2 files changed, 14 insertions(+), 5 deletions(-) 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 d84220ef8889bc..880fd19854c655 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,25 @@ internal static unsafe char ParseFormatSpecifier(ReadOnlySpan format, out } } - // Fallback for symbol and any length digits. The digits value must be >= 0 && <= 99, + // Fallback for symbol and any length digits. The digits value must be >= 0 && <= 2147483647, // but it can begin with any number of 0s, and thus we may need to check more than two // 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) + // Quickly check if we are about to overflow + if (n >= 214748364) { - throw new FormatException(SR.Argument_BadFormatSpecifier); + // if n is 214748364, we might still have a valid input in the range (2147483640,2147483647), + // so we should continue. Otherwise, throw FormatException. + bool potentialValidFormat = (n == 214748364 && format[i] <= '7'); + if (!potentialValidFormat) + { + 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/tests/System/DoubleTests.cs b/src/libraries/System.Runtime/tests/System/DoubleTests.cs index 63232f8f505457..3a4eabdf7750f3 100644 --- a/src/libraries/System.Runtime/tests/System/DoubleTests.cs +++ b/src/libraries/System.Runtime/tests/System/DoubleTests.cs @@ -830,6 +830,9 @@ public static void ToString_InvalidFormat_ThrowsFormatException() long intMaxPlus1 = (long)int.MaxValue + 1; string intMaxPlus1String = intMaxPlus1.ToString(); Assert.Throws(() => d.ToString("E" + intMaxPlus1String)); + + // Check larger overflowed value + Assert.Throws(() => d.ToString("EG4772185890")); } [Theory] From d753769116402ab82639fc0b4fe1ab40dfe4e9cc Mon Sep 17 00:00:00 2001 From: Drew Kersnar <18474647+dakersnar@users.noreply.github.com> Date: Thu, 21 Jul 2022 17:38:00 -0500 Subject: [PATCH 2/5] Change limit to 9 digits --- .../src/System/Number.Formatting.cs | 17 ++++++----------- .../System.Runtime/tests/System/DoubleTests.cs | 11 ++++++++--- 2 files changed, 14 insertions(+), 14 deletions(-) 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 880fd19854c655..dffd043d7ce533 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs @@ -2318,25 +2318,20 @@ internal static unsafe char ParseFormatSpecifier(ReadOnlySpan format, out } } - // Fallback for symbol and any length digits. The digits value must be >= 0 && <= 2147483647, - // 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])) { - // Quickly check if we are about to overflow - if (n >= 214748364) + // Check if we are about to overflow past our limit of 9 digits + if (n >= 100_000_000) { - // if n is 214748364, we might still have a valid input in the range (2147483640,2147483647), - // so we should continue. Otherwise, throw FormatException. - bool potentialValidFormat = (n == 214748364 && format[i] <= '7'); - if (!potentialValidFormat) - { - throw new FormatException(SR.Argument_BadFormatSpecifier); - } + 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 diff --git a/src/libraries/System.Runtime/tests/System/DoubleTests.cs b/src/libraries/System.Runtime/tests/System/DoubleTests.cs index 3a4eabdf7750f3..a2784cf2aee0d7 100644 --- a/src/libraries/System.Runtime/tests/System/DoubleTests.cs +++ b/src/libraries/System.Runtime/tests/System/DoubleTests.cs @@ -827,12 +827,17 @@ 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)); - - // Check larger overflowed value - Assert.Throws(() => d.ToString("EG4772185890")); + Assert.Throws(() => d.ToString("E4772185890")); + d.ToString("E999999999"); // Should not throw + d.ToString("E00000999999999"); // Should not throw + Assert.Throws(() => d.ToString("E1000000000")); + Assert.Throws(() => d.ToString("E000001000000000")); } [Theory] From d9ab57fa23d750e88bd5c884fc6023422e64134b Mon Sep 17 00:00:00 2001 From: Drew Kersnar <18474647+dakersnar@users.noreply.github.com> Date: Fri, 22 Jul 2022 14:34:15 -0500 Subject: [PATCH 3/5] Fixed the same bug in other places --- .../Globalization/FormatProvider.Number.cs | 10 ++--- .../src/System/Number.Formatting.cs | 1 - .../src/System/Numerics/BigNumber.cs | 24 ++++++------ .../BigInteger/BigIntegerToStringTests.cs | 39 +++++++++++++++++++ .../tests/System/DoubleTests.cs | 14 ++++++- 5 files changed, 68 insertions(+), 20 deletions(-) diff --git a/src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs b/src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs index 8636878c895025..0e11b9b186d42a 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 dffd043d7ce533..f81c6ac437aba2 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs @@ -2331,7 +2331,6 @@ internal static unsafe char ParseFormatSpecifier(ReadOnlySpan format, out 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 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 51643f6b4e4df8..349fc107f4409b 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 35949f76935fd5..f443b5afee2b01 100644 --- a/src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs +++ b/src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs @@ -439,6 +439,45 @@ 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] + 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 a2784cf2aee0d7..c04b698b922a2a 100644 --- a/src/libraries/System.Runtime/tests/System/DoubleTests.cs +++ b/src/libraries/System.Runtime/tests/System/DoubleTests.cs @@ -6,6 +6,7 @@ using System.IO; using System.IO.Tests; using System.Linq; +using System.Numerics; using Xunit; #pragma warning disable xUnit1025 // reporting duplicate test cases due to not distinguishing 0.0 from -0.0, NaN from -NaN @@ -834,12 +835,21 @@ public static void ToString_InvalidFormat_ThrowsFormatException() string intMaxPlus1String = intMaxPlus1.ToString(); Assert.Throws(() => d.ToString("E" + intMaxPlus1String)); Assert.Throws(() => d.ToString("E4772185890")); - d.ToString("E999999999"); // Should not throw - d.ToString("E00000999999999"); // Should not throw Assert.Throws(() => d.ToString("E1000000000")); Assert.Throws(() => d.ToString("E000001000000000")); } + [Fact] + 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] [InlineData(double.NegativeInfinity, false)] // Negative Infinity [InlineData(double.MinValue, true)] // Min Negative Normal From 25a5fd6d22971cb1fe8aba6c460db8cecbcb6a61 Mon Sep 17 00:00:00 2001 From: Drew Kersnar <18474647+dakersnar@users.noreply.github.com> Date: Fri, 22 Jul 2022 14:41:12 -0500 Subject: [PATCH 4/5] Remove mistake include --- src/libraries/System.Runtime/tests/System/DoubleTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Runtime/tests/System/DoubleTests.cs b/src/libraries/System.Runtime/tests/System/DoubleTests.cs index c04b698b922a2a..73ace57d2caeff 100644 --- a/src/libraries/System.Runtime/tests/System/DoubleTests.cs +++ b/src/libraries/System.Runtime/tests/System/DoubleTests.cs @@ -6,7 +6,6 @@ using System.IO; using System.IO.Tests; using System.Linq; -using System.Numerics; using Xunit; #pragma warning disable xUnit1025 // reporting duplicate test cases due to not distinguishing 0.0 from -0.0, NaN from -NaN From 98a1c4f2990589c563e4919f1b4b440ba3040b7c Mon Sep 17 00:00:00 2001 From: Drew Kersnar <18474647+dakersnar@users.noreply.github.com> Date: Tue, 26 Jul 2022 15:23:12 -0500 Subject: [PATCH 5/5] Move expensive tests to OuterLoop --- .../tests/BigInteger/BigIntegerToStringTests.cs | 1 + src/libraries/System.Runtime/tests/System/DoubleTests.cs | 1 + 2 files changed, 2 insertions(+) diff --git a/src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs b/src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs index f443b5afee2b01..78b47e967784c0 100644 --- a/src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs +++ b/src/libraries/System.Runtime.Numerics/tests/BigInteger/BigIntegerToStringTests.cs @@ -463,6 +463,7 @@ public static void ToString_InvalidFormat_ThrowsFormatException() } [Fact] + [OuterLoop("Takes a long time, allocates a lot of memory")] public static void ToString_ValidLargeFormat() { BigInteger b = new BigInteger(123456789000m); diff --git a/src/libraries/System.Runtime/tests/System/DoubleTests.cs b/src/libraries/System.Runtime/tests/System/DoubleTests.cs index 73ace57d2caeff..660fd463f32174 100644 --- a/src/libraries/System.Runtime/tests/System/DoubleTests.cs +++ b/src/libraries/System.Runtime/tests/System/DoubleTests.cs @@ -839,6 +839,7 @@ public static void ToString_InvalidFormat_ThrowsFormatException() } [Fact] + [OuterLoop("Takes a long time, allocates a lot of memory")] public static void ToString_ValidLargeFormat() { double d = 123.0;