From a8a6ffdb9258677e8bd89b133ccd731888477eba Mon Sep 17 00:00:00 2001 From: Shane Date: Tue, 18 Sep 2018 18:10:38 -0700 Subject: [PATCH] ICU-20037 Fixing ScientificMatcher integer overflow. (#138) Also restricts parsing to read only one exponent per string. --- icu4c/source/i18n/numparse_scientific.cpp | 5 ++++ icu4c/source/test/intltest/numfmtst.cpp | 23 +++++++++++++++++++ icu4c/source/test/intltest/numfmtst.h | 1 + .../impl/number/parse/ScientificMatcher.java | 5 ++++ .../icu/dev/test/format/NumberFormatTest.java | 22 ++++++++++++++++++ 5 files changed, 56 insertions(+) diff --git a/icu4c/source/i18n/numparse_scientific.cpp b/icu4c/source/i18n/numparse_scientific.cpp index 611695e57d4..de389574408 100644 --- a/icu4c/source/i18n/numparse_scientific.cpp +++ b/icu4c/source/i18n/numparse_scientific.cpp @@ -56,6 +56,11 @@ bool ScientificMatcher::match(StringSegment& segment, ParsedNumber& result, UErr return false; } + // Only accept one exponent per string. + if (0 != (result.flags & FLAG_HAS_EXPONENT)) { + return false; + } + // First match the scientific separator, and then match another number after it. // NOTE: This is guarded by the smoke test; no need to check fExponentSeparatorString length again. int overlap1 = segment.getCommonPrefixLength(fExponentSeparatorString); diff --git a/icu4c/source/test/intltest/numfmtst.cpp b/icu4c/source/test/intltest/numfmtst.cpp index db8d22728b1..77d4283441b 100644 --- a/icu4c/source/test/intltest/numfmtst.cpp +++ b/icu4c/source/test/intltest/numfmtst.cpp @@ -215,6 +215,7 @@ void NumberFormatTest::runIndexedTest( int32_t index, UBool exec, const char* &n TESTCASE_AUTO(Test13763_FieldPositionIteratorOffset); TESTCASE_AUTO(Test13777_ParseLongNameNonCurrencyMode); TESTCASE_AUTO(Test13804_EmptyStringsWhenParsing); + TESTCASE_AUTO(Test20037_ScientificIntegerOverflow); TESTCASE_AUTO(Test13840_ParseLongStringCrash); TESTCASE_AUTO(Test13850_EmptyStringCurrency); TESTCASE_AUTO_END; @@ -9185,6 +9186,28 @@ void NumberFormatTest::Test13804_EmptyStringsWhenParsing() { } } +void NumberFormatTest::Test20037_ScientificIntegerOverflow() { + IcuTestErrorCode status(*this, "Test20037_ScientificIntegerOverflow"); + + LocalPointer nf(NumberFormat::createInstance(status)); + Formattable result; + + // Test overflow of exponent + nf->parse(u"1E-2147483648", result, status); + StringPiece sp = result.getDecimalNumber(status); + assertEquals(u"Should snap to zero", + u"0", + {sp.data(), sp.length(), US_INV}); + + // Test edge case overflow of exponent + result = Formattable(); + nf->parse(u"1E-2147483647E-1", result, status); + sp = result.getDecimalNumber(status); + assertEquals(u"Should not overflow and should parse only the first exponent", + u"1E-2147483647", + {sp.data(), sp.length(), US_INV}); +} + void NumberFormatTest::Test13840_ParseLongStringCrash() { IcuTestErrorCode status(*this, "Test13840_ParseLongStringCrash"); diff --git a/icu4c/source/test/intltest/numfmtst.h b/icu4c/source/test/intltest/numfmtst.h index 9f7a5c08353..65e0ffe8f33 100644 --- a/icu4c/source/test/intltest/numfmtst.h +++ b/icu4c/source/test/intltest/numfmtst.h @@ -279,6 +279,7 @@ class NumberFormatTest: public CalendarTimeZoneTest { void Test13763_FieldPositionIteratorOffset(); void Test13777_ParseLongNameNonCurrencyMode(); void Test13804_EmptyStringsWhenParsing(); + void Test20037_ScientificIntegerOverflow(); void Test13840_ParseLongStringCrash(); void Test13850_EmptyStringCurrency(); diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/ScientificMatcher.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/ScientificMatcher.java index 1904ea414a2..866d2dad8eb 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/ScientificMatcher.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/parse/ScientificMatcher.java @@ -54,6 +54,11 @@ public class ScientificMatcher implements NumberParseMatcher { return false; } + // Only accept one exponent per string. + if (0 != (result.flags & ParsedNumber.FLAG_HAS_EXPONENT)) { + return false; + } + // First match the scientific separator, and then match another number after it. // NOTE: This is guarded by the smoke test; no need to check exponentSeparatorString length again. int overlap1 = segment.getCommonPrefixLength(exponentSeparatorString); diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java index 5e67d35952d..d276f1fc0b2 100644 --- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java +++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/format/NumberFormatTest.java @@ -6294,6 +6294,28 @@ public class NumberFormatTest extends TestFmwk { } } + @Test + public void Test20037_ScientificIntegerOverflow() throws ParseException { + NumberFormat nf = NumberFormat.getInstance(ULocale.US); + + // Test overflow of exponent + Number result = nf.parse("1E-2147483648"); + assertEquals("Should snap to zero", + "0", result.toString()); + + // Test edge case overflow of exponent + // Note: the behavior is different from C++; this is probably due to the + // intermediate BigDecimal form, which has its own restrictions + result = nf.parse("1E-2147483647E-1"); + assertEquals("Should not overflow and should parse only the first exponent", + "0.0", result.toString()); + + // For Java, we should get *pretty close* to 2^31. + result = nf.parse("1E-547483647"); + assertEquals("Should *not* snap to zero", + "1E-547483647", result.toString()); + } + @Test public void test13840_ParseLongStringCrash() throws ParseException { NumberFormat nf = NumberFormat.getInstance(ULocale.ENGLISH); -- 2.40.0