From 303b7e81d7139e7cdc3f4fa27f1a4e49820b8fbf Mon Sep 17 00:00:00 2001 From: Frank Tang Date: Wed, 18 Sep 2024 13:44:08 -0700 Subject: [PATCH] ICU-22897 Fix memory leak and int overflow 1. Rewrite to use LocalPointer to prevent memory leak 2. Rewrite the if/else to switch to make the logic clear 3. Delete the rule if not remember inside the rule set to fix memory leak. 4. Check base value calculation to avoid int64_t overflow. 5. Add memory leak test --- icu4c/source/i18n/nfrs.cpp | 50 ++++++++++++++++----------- icu4c/source/i18n/nfrule.cpp | 30 ++++++++++------ icu4c/source/test/intltest/itrbnf.cpp | 9 +++++ icu4c/source/test/intltest/itrbnf.h | 1 + 4 files changed, 58 insertions(+), 32 deletions(-) diff --git a/icu4c/source/i18n/nfrs.cpp b/icu4c/source/i18n/nfrs.cpp index b8bd7a31cbfe..5f0d6b3c1da3 100644 --- a/icu4c/source/i18n/nfrs.cpp +++ b/icu4c/source/i18n/nfrs.cpp @@ -267,27 +267,35 @@ NFRuleSet::parseRules(UnicodeString& description, UErrorCode& status) * @param rule The rule to set. */ void NFRuleSet::setNonNumericalRule(NFRule *rule) { - int64_t baseValue = rule->getBaseValue(); - if (baseValue == NFRule::kNegativeNumberRule) { - delete nonNumericalRules[NEGATIVE_RULE_INDEX]; - nonNumericalRules[NEGATIVE_RULE_INDEX] = rule; - } - else if (baseValue == NFRule::kImproperFractionRule) { - setBestFractionRule(IMPROPER_FRACTION_RULE_INDEX, rule, true); - } - else if (baseValue == NFRule::kProperFractionRule) { - setBestFractionRule(PROPER_FRACTION_RULE_INDEX, rule, true); - } - else if (baseValue == NFRule::kDefaultRule) { - setBestFractionRule(DEFAULT_RULE_INDEX, rule, true); - } - else if (baseValue == NFRule::kInfinityRule) { - delete nonNumericalRules[INFINITY_RULE_INDEX]; - nonNumericalRules[INFINITY_RULE_INDEX] = rule; - } - else if (baseValue == NFRule::kNaNRule) { - delete nonNumericalRules[NAN_RULE_INDEX]; - nonNumericalRules[NAN_RULE_INDEX] = rule; + switch (rule->getBaseValue()) { + case NFRule::kNegativeNumberRule: + delete nonNumericalRules[NEGATIVE_RULE_INDEX]; + nonNumericalRules[NEGATIVE_RULE_INDEX] = rule; + return; + case NFRule::kImproperFractionRule: + setBestFractionRule(IMPROPER_FRACTION_RULE_INDEX, rule, true); + return; + case NFRule::kProperFractionRule: + setBestFractionRule(PROPER_FRACTION_RULE_INDEX, rule, true); + return; + case NFRule::kDefaultRule: + setBestFractionRule(DEFAULT_RULE_INDEX, rule, true); + return; + case NFRule::kInfinityRule: + delete nonNumericalRules[INFINITY_RULE_INDEX]; + nonNumericalRules[INFINITY_RULE_INDEX] = rule; + return; + case NFRule::kNaNRule: + delete nonNumericalRules[NAN_RULE_INDEX]; + nonNumericalRules[NAN_RULE_INDEX] = rule; + return; + case NFRule::kNoBase: + case NFRule::kOtherRule: + default: + // If we do not remember the rule inside the object. + // delete it here to prevent memory leak. + delete rule; + return; } } diff --git a/icu4c/source/i18n/nfrule.cpp b/icu4c/source/i18n/nfrule.cpp index 247c0a31be4b..e7908ecab912 100644 --- a/icu4c/source/i18n/nfrule.cpp +++ b/icu4c/source/i18n/nfrule.cpp @@ -19,6 +19,7 @@ #if U_HAVE_RBNF +#include #include "unicode/localpointer.h" #include "unicode/rbnf.h" #include "unicode/tblcoll.h" @@ -116,9 +117,9 @@ NFRule::makeRules(UnicodeString& description, // new it up and initialize its basevalue and divisor // (this also strips the rule descriptor, if any, off the // description string) - NFRule* rule1 = new NFRule(rbnf, description, status); + LocalPointer rule1(new NFRule(rbnf, description, status)); /* test for nullptr */ - if (rule1 == nullptr) { + if (rule1.isNull()) { status = U_MEMORY_ALLOCATION_ERROR; return; } @@ -144,7 +145,7 @@ NFRule::makeRules(UnicodeString& description, else { // if the description does contain a matched pair of brackets, // then it's really shorthand for two rules (with one exception) - NFRule* rule2 = nullptr; + LocalPointer rule2; UnicodeString sbuf; // we'll actually only split the rule into two rules if its @@ -160,9 +161,9 @@ NFRule::makeRules(UnicodeString& description, // set, they both have the same base value; otherwise, // increment the original rule's base value ("rule1" actually // goes SECOND in the rule set's rule list) - rule2 = new NFRule(rbnf, UnicodeString(), status); + rule2.adoptInstead(new NFRule(rbnf, UnicodeString(), status)); /* test for nullptr */ - if (rule2 == nullptr) { + if (rule2.isNull()) { status = U_MEMORY_ALLOCATION_ERROR; return; } @@ -217,20 +218,20 @@ NFRule::makeRules(UnicodeString& description, // BEFORE rule1 in the list: in all cases, rule2 OMITS the // material in the brackets and rule1 INCLUDES the material // in the brackets) - if (rule2 != nullptr) { + if (!rule2.isNull()) { if (rule2->baseValue >= kNoBase) { - rules.add(rule2); + rules.add(rule2.orphan()); } else { - owner->setNonNumericalRule(rule2); + owner->setNonNumericalRule(rule2.orphan()); } } } if (rule1->baseValue >= kNoBase) { - rules.add(rule1); + rules.add(rule1.orphan()); } else { - owner->setNonNumericalRule(rule1); + owner->setNonNumericalRule(rule1.orphan()); } } @@ -289,7 +290,14 @@ NFRule::parseRuleDescriptor(UnicodeString& description, UErrorCode& status) while (p < descriptorLength) { c = descriptor.charAt(p); if (c >= gZero && c <= gNine) { - val = val * ll_10 + static_cast(c - gZero); + int32_t single_digit = static_cast(c - gZero); + if ((val > 0 && val > (std::numeric_limits::max() - single_digit) / 10) || + (val < 0 && val < (std::numeric_limits::min() - single_digit) / 10)) { + // out of int64_t range + status = U_PARSE_ERROR; + return; + } + val = val * ll_10 + single_digit; } else if (c == gSlash || c == gGreaterThan) { break; diff --git a/icu4c/source/test/intltest/itrbnf.cpp b/icu4c/source/test/intltest/itrbnf.cpp index fd2451856e56..c0d00ec8c348 100644 --- a/icu4c/source/test/intltest/itrbnf.cpp +++ b/icu4c/source/test/intltest/itrbnf.cpp @@ -80,6 +80,7 @@ void IntlTestRBNF::runIndexedTest(int32_t index, UBool exec, const char* &name, TESTCASE(28, TestNorwegianSpellout); TESTCASE(29, TestNumberingSystem); TESTCASE(30, TestDFRounding); + TESTCASE(31, TestMemoryLeak22899); #else TESTCASE(0, TestRBNFDisabled); #endif @@ -1340,6 +1341,14 @@ void IntlTestRBNF::TestDFRounding() } } +void IntlTestRBNF::TestMemoryLeak22899() +{ + UErrorCode status = U_ZERO_ERROR; + UParseError perror; + icu::UnicodeString str(u"0,31,01,30,01,0,01,01,30,01,30,31,01,30,01,30,30,00,01,0:"); + icu::RuleBasedNumberFormat rbfmt(str, icu::Locale::getEnglish(), perror, status); +} + void IntlTestRBNF::TestSpanishSpellout() { diff --git a/icu4c/source/test/intltest/itrbnf.h b/icu4c/source/test/intltest/itrbnf.h index fd6e4107817c..3c8c4f1e024a 100644 --- a/icu4c/source/test/intltest/itrbnf.h +++ b/icu4c/source/test/intltest/itrbnf.h @@ -161,6 +161,7 @@ class IntlTestRBNF : public IntlTest { void TestParseFailure(); void TestMinMaxIntegerDigitsIgnored(); void TestNumberingSystem(); + void TestMemoryLeak22899(); protected: virtual void doTest(RuleBasedNumberFormat* formatter, const char* const testData[][2], UBool testParsing);