From 35dff23fb54a3566d19bc864a3ca4b5f308145eb Mon Sep 17 00:00:00 2001 From: Andy Heninger Date: Wed, 15 Sep 2021 14:51:48 -0700 Subject: [PATCH] ICU-21662 UVector cleanup in rbtz.cpp Revise uses of UVector in rbtz.cpp to better handle memory allocation failures. This is one of an ongoing series of commits to address similar problems with UVector usage throughout ICU. The changes include - Use LocalPointers and UVector deleter functions to simplify OOM checking and recovery. - Fix RuleBasedTimeZone::addTransitionRule(rule) to have standard ICU adopt behavior when errors occur, meaning automatic deletion of the incoming rule. This simplifies both the implementation of the function and the code at the call sites. - Update addTransitionRule() call sites. Includes modifying the Dangi calendar initializtion to not silently ignore errors. - struct Transition is changed to derive from UMemory, which allows the use of LocalPointers. --- icu4c/source/i18n/dangical.cpp | 45 ++++++---- icu4c/source/i18n/dangical.h | 2 +- icu4c/source/i18n/rbtz.cpp | 109 ++++++++++-------------- icu4c/source/i18n/unicode/rbtz.h | 19 +++-- icu4c/source/i18n/vtzone.cpp | 38 +++------ icu4c/source/test/intltest/tzrulets.cpp | 4 - 6 files changed, 94 insertions(+), 123 deletions(-) diff --git a/icu4c/source/i18n/dangical.cpp b/icu4c/source/i18n/dangical.cpp index 02db40368ec..57fe80220b9 100644 --- a/icu4c/source/i18n/dangical.cpp +++ b/icu4c/source/i18n/dangical.cpp @@ -52,7 +52,7 @@ U_NAMESPACE_BEGIN //------------------------------------------------------------------------- DangiCalendar::DangiCalendar(const Locale& aLocale, UErrorCode& success) -: ChineseCalendar(aLocale, DANGI_EPOCH_YEAR, getDangiCalZoneAstroCalc(), success) +: ChineseCalendar(aLocale, DANGI_EPOCH_YEAR, getDangiCalZoneAstroCalc(success), success) { } @@ -103,32 +103,41 @@ const char *DangiCalendar::getType() const { * 1898-1911: GMT+8 * 1912- : GMT+9 */ -static void U_CALLCONV initDangiCalZoneAstroCalc(void) { - U_ASSERT(gDangiCalendarZoneAstroCalc == NULL); +static void U_CALLCONV initDangiCalZoneAstroCalc(UErrorCode &status) { + U_ASSERT(gDangiCalendarZoneAstroCalc == nullptr); const UDate millis1897[] = { (UDate)((1897 - 1970) * 365 * kOneDay) }; // some days of error is not a problem here const UDate millis1898[] = { (UDate)((1898 - 1970) * 365 * kOneDay) }; // some days of error is not a problem here const UDate millis1912[] = { (UDate)((1912 - 1970) * 365 * kOneDay) }; // this doesn't create an issue for 1911/12/20 - InitialTimeZoneRule* initialTimeZone = new InitialTimeZoneRule(UNICODE_STRING_SIMPLE("GMT+8"), 8*kOneHour, 0); - TimeZoneRule* rule1897 = new TimeArrayTimeZoneRule(UNICODE_STRING_SIMPLE("Korean 1897"), 7*kOneHour, 0, millis1897, 1, DateTimeRule::STANDARD_TIME); - TimeZoneRule* rule1898to1911 = new TimeArrayTimeZoneRule(UNICODE_STRING_SIMPLE("Korean 1898-1911"), 8*kOneHour, 0, millis1898, 1, DateTimeRule::STANDARD_TIME); - TimeZoneRule* ruleFrom1912 = new TimeArrayTimeZoneRule(UNICODE_STRING_SIMPLE("Korean 1912-"), 9*kOneHour, 0, millis1912, 1, DateTimeRule::STANDARD_TIME); - UErrorCode status = U_ZERO_ERROR; - RuleBasedTimeZone* dangiCalZoneAstroCalc = new RuleBasedTimeZone(UNICODE_STRING_SIMPLE("KOREA_ZONE"), initialTimeZone); // adopts initialTimeZone - dangiCalZoneAstroCalc->addTransitionRule(rule1897, status); // adopts rule1897 - dangiCalZoneAstroCalc->addTransitionRule(rule1898to1911, status); - dangiCalZoneAstroCalc->addTransitionRule(ruleFrom1912, status); + LocalPointer initialTimeZone(new InitialTimeZoneRule( + UnicodeString(u"GMT+8"), 8*kOneHour, 0), status); + + LocalPointer rule1897(new TimeArrayTimeZoneRule( + UnicodeString(u"Korean 1897"), 7*kOneHour, 0, millis1897, 1, DateTimeRule::STANDARD_TIME), status); + + LocalPointer rule1898to1911(new TimeArrayTimeZoneRule( + UnicodeString(u"Korean 1898-1911"), 8*kOneHour, 0, millis1898, 1, DateTimeRule::STANDARD_TIME), status); + + LocalPointer ruleFrom1912(new TimeArrayTimeZoneRule( + UnicodeString(u"Korean 1912-"), 9*kOneHour, 0, millis1912, 1, DateTimeRule::STANDARD_TIME), status); + + LocalPointer dangiCalZoneAstroCalc(new RuleBasedTimeZone( + UnicodeString(u"KOREA_ZONE"), initialTimeZone.orphan()), status); // adopts initialTimeZone + + if (U_FAILURE(status)) { + return; + } + dangiCalZoneAstroCalc->addTransitionRule(rule1897.orphan(), status); // adopts rule1897 + dangiCalZoneAstroCalc->addTransitionRule(rule1898to1911.orphan(), status); + dangiCalZoneAstroCalc->addTransitionRule(ruleFrom1912.orphan(), status); dangiCalZoneAstroCalc->complete(status); if (U_SUCCESS(status)) { - gDangiCalendarZoneAstroCalc = dangiCalZoneAstroCalc; - } else { - delete dangiCalZoneAstroCalc; - gDangiCalendarZoneAstroCalc = NULL; + gDangiCalendarZoneAstroCalc = dangiCalZoneAstroCalc.orphan(); } ucln_i18n_registerCleanup(UCLN_I18N_DANGI_CALENDAR, calendar_dangi_cleanup); } -const TimeZone* DangiCalendar::getDangiCalZoneAstroCalc(void) const { - umtx_initOnce(gDangiCalendarInitOnce, &initDangiCalZoneAstroCalc); +const TimeZone* DangiCalendar::getDangiCalZoneAstroCalc(UErrorCode &status) const { + umtx_initOnce(gDangiCalendarInitOnce, &initDangiCalZoneAstroCalc, status); return gDangiCalendarZoneAstroCalc; } diff --git a/icu4c/source/i18n/dangical.h b/icu4c/source/i18n/dangical.h index 4d55a5b59ee..9d0437264ef 100644 --- a/icu4c/source/i18n/dangical.h +++ b/icu4c/source/i18n/dangical.h @@ -74,7 +74,7 @@ class DangiCalendar : public ChineseCalendar { private: - const TimeZone* getDangiCalZoneAstroCalc(void) const; + const TimeZone* getDangiCalZoneAstroCalc(UErrorCode &status) const; // UObject stuff public: diff --git a/icu4c/source/i18n/rbtz.cpp b/icu4c/source/i18n/rbtz.cpp index 0a9c7db6581..495d8310d00 100644 --- a/icu4c/source/i18n/rbtz.cpp +++ b/icu4c/source/i18n/rbtz.cpp @@ -25,12 +25,19 @@ U_NAMESPACE_BEGIN /** * A struct representing a time zone transition */ -struct Transition { +struct Transition : public UMemory { UDate time; TimeZoneRule* from; TimeZoneRule* to; }; +U_CDECL_BEGIN +static void U_CALLCONV +deleteTransition(void* obj) { + delete static_cast(obj); +} +U_CDECL_END + static UBool compareRules(UVector* rules1, UVector* rules2) { if (rules1 == NULL && rules2 == NULL) { return TRUE; @@ -114,32 +121,35 @@ RuleBasedTimeZone::operator!=(const TimeZone& that) const { void RuleBasedTimeZone::addTransitionRule(TimeZoneRule* rule, UErrorCode& status) { + LocalPointerlpRule(rule); if (U_FAILURE(status)) { return; } AnnualTimeZoneRule* atzrule = dynamic_cast(rule); - if (atzrule != NULL && atzrule->getEndYear() == AnnualTimeZoneRule::MAX_YEAR) { + if (atzrule != nullptr && atzrule->getEndYear() == AnnualTimeZoneRule::MAX_YEAR) { // A final rule - if (fFinalRules == NULL) { - fFinalRules = new UVector(status); + if (fFinalRules == nullptr) { + LocalPointer lpFinalRules(new UVector(uprv_deleteUObject, nullptr, status), status); if (U_FAILURE(status)) { return; } + fFinalRules = lpFinalRules.orphan(); } else if (fFinalRules->size() >= 2) { // Cannot handle more than two final rules status = U_INVALID_STATE_ERROR; return; } - fFinalRules->addElementX((void*)rule, status); + fFinalRules->adoptElement(lpRule.orphan(), status); } else { // Non-final rule - if (fHistoricRules == NULL) { - fHistoricRules = new UVector(status); + if (fHistoricRules == nullptr) { + LocalPointer lpHistoricRules(new UVector(uprv_deleteUObject, nullptr, status), status); if (U_FAILURE(status)) { return; } + fHistoricRules = lpHistoricRules.orphan(); } - fHistoricRules->addElementX((void*)rule, status); + fHistoricRules->adoptElement(lpRule.orphan(), status); } // Mark dirty, so transitions are recalculated at next complete() call fUpToDate = FALSE; @@ -175,7 +185,6 @@ RuleBasedTimeZone::complete(UErrorCode& status) { return; } - UBool *done = NULL; // Create a TimezoneTransition and add to the list if (fHistoricRules != NULL || fFinalRules != NULL) { TimeZoneRule *curRule = fInitialRule; @@ -186,13 +195,13 @@ RuleBasedTimeZone::complete(UErrorCode& status) { if (fHistoricRules != NULL && fHistoricRules->size() > 0) { int32_t i; int32_t historicCount = fHistoricRules->size(); - done = (UBool*)uprv_malloc(sizeof(UBool) * historicCount); + LocalMemory done((bool *)uprv_malloc(sizeof(bool) * historicCount)); if (done == NULL) { status = U_MEMORY_ALLOCATION_ERROR; goto cleanup; } for (i = 0; i < historicCount; i++) { - done[i] = FALSE; + done[i] = false; } while (TRUE) { int32_t curStdOffset = curRule->getRawOffset(); @@ -213,7 +222,7 @@ RuleBasedTimeZone::complete(UErrorCode& status) { avail = r->getNextStart(lastTransitionTime, curStdOffset, curDstSavings, false, tt); if (!avail) { // No more transitions from this rule - skip this rule next time - done[i] = TRUE; + done[i] = true; } else { r->getName(name); if (*r == *curRule || @@ -266,20 +275,21 @@ RuleBasedTimeZone::complete(UErrorCode& status) { } if (fHistoricTransitions == NULL) { - fHistoricTransitions = new UVector(status); + LocalPointer lpHistoricTransitions( + new UVector(deleteTransition, nullptr, status), status); if (U_FAILURE(status)) { goto cleanup; } + fHistoricTransitions = lpHistoricTransitions.orphan(); } - Transition *trst = (Transition*)uprv_malloc(sizeof(Transition)); - if (trst == NULL) { - status = U_MEMORY_ALLOCATION_ERROR; + LocalPointer trst(new Transition, status); + if (U_FAILURE(status)) { goto cleanup; } trst->time = nextTransitionTime; trst->from = curRule; trst->to = nextRule; - fHistoricTransitions->addElementX(trst, status); + fHistoricTransitions->adoptElement(trst.orphan(), status); if (U_FAILURE(status)) { goto cleanup; } @@ -289,10 +299,12 @@ RuleBasedTimeZone::complete(UErrorCode& status) { } if (fFinalRules != NULL) { if (fHistoricTransitions == NULL) { - fHistoricTransitions = new UVector(status); + LocalPointer lpHistoricTransitions( + new UVector(deleteTransition, nullptr, status), status); if (U_FAILURE(status)) { goto cleanup; } + fHistoricTransitions = lpHistoricTransitions.orphan(); } // Append the first transition for each TimeZoneRule *rule0 = (TimeZoneRule*)fFinalRules->elementAt(0); @@ -305,16 +317,10 @@ RuleBasedTimeZone::complete(UErrorCode& status) { status = U_INVALID_STATE_ERROR; goto cleanup; } - Transition *final0 = (Transition*)uprv_malloc(sizeof(Transition)); - if (final0 == NULL) { - status = U_MEMORY_ALLOCATION_ERROR; - goto cleanup; - } - Transition *final1 = (Transition*)uprv_malloc(sizeof(Transition)); - if (final1 == NULL) { - uprv_free(final0); - status = U_MEMORY_ALLOCATION_ERROR; - goto cleanup; + LocalPointer final0(new Transition, status); + LocalPointer final1(new Transition, status); + if (U_FAILURE(status)) { + goto cleanup; } if (tt0 < tt1) { final0->time = tt0; @@ -331,27 +337,18 @@ RuleBasedTimeZone::complete(UErrorCode& status) { final1->from = rule1; final1->to = rule0; } - fHistoricTransitions->addElementX(final0, status); - if (U_FAILURE(status)) { - goto cleanup; - } - fHistoricTransitions->addElementX(final1, status); + fHistoricTransitions->adoptElement(final0.orphan(), status); + fHistoricTransitions->adoptElement(final1.orphan(), status); if (U_FAILURE(status)) { goto cleanup; } } } fUpToDate = TRUE; - if (done != NULL) { - uprv_free(done); - } return; cleanup: deleteTransitions(); - if (done != NULL) { - uprv_free(done); - } fUpToDate = FALSE; } @@ -628,16 +625,10 @@ RuleBasedTimeZone::deleteRules(void) { delete fInitialRule; fInitialRule = NULL; if (fHistoricRules != NULL) { - while (!fHistoricRules->isEmpty()) { - delete (TimeZoneRule*)(fHistoricRules->orphanElementAt(0)); - } delete fHistoricRules; fHistoricRules = NULL; } if (fFinalRules != NULL) { - while (!fFinalRules->isEmpty()) { - delete (AnnualTimeZoneRule*)(fFinalRules->orphanElementAt(0)); - } delete fFinalRules; fFinalRules = NULL; } @@ -646,10 +637,6 @@ RuleBasedTimeZone::deleteRules(void) { void RuleBasedTimeZone::deleteTransitions(void) { if (fHistoricTransitions != NULL) { - while (!fHistoricTransitions->isEmpty()) { - Transition *trs = (Transition*)fHistoricTransitions->orphanElementAt(0); - uprv_free(trs); - } delete fHistoricTransitions; } fHistoricTransitions = NULL; @@ -657,32 +644,24 @@ RuleBasedTimeZone::deleteTransitions(void) { UVector* RuleBasedTimeZone::copyRules(UVector* source) { - if (source == NULL) { - return NULL; + if (source == nullptr) { + return nullptr; } UErrorCode ec = U_ZERO_ERROR; int32_t size = source->size(); - UVector *rules = new UVector(size, ec); + LocalPointer rules(new UVector(uprv_deleteUObject, nullptr, size, ec), ec); if (U_FAILURE(ec)) { - return NULL; + return nullptr; } int32_t i; for (i = 0; i < size; i++) { - rules->addElementX(((TimeZoneRule*)source->elementAt(i))->clone(), ec); + LocalPointer rule(((TimeZoneRule*)source->elementAt(i))->clone(), ec); + rules->adoptElement(rule.orphan(), ec); if (U_FAILURE(ec)) { - break; + return nullptr; } } - if (U_FAILURE(ec)) { - // In case of error, clean up - for (i = 0; i < rules->size(); i++) { - TimeZoneRule *rule = (TimeZoneRule*)rules->orphanElementAt(i); - delete rule; - } - delete rules; - return NULL; - } - return rules; + return rules.orphan(); } TimeZoneRule* diff --git a/icu4c/source/i18n/unicode/rbtz.h b/icu4c/source/i18n/unicode/rbtz.h index 17f72ed67c1..1eca70c338b 100644 --- a/icu4c/source/i18n/unicode/rbtz.h +++ b/icu4c/source/i18n/unicode/rbtz.h @@ -89,17 +89,18 @@ public: virtual bool operator!=(const TimeZone& that) const; /** - * Adds the TimeZoneRule which represents time transitions. - * The TimeZoneRule must have start times, that is, the result - * of isTransitionRule() must be true. Otherwise, U_ILLEGAL_ARGUMENT_ERROR + * Adds the `TimeZoneRule` which represents time transitions. + * The `TimeZoneRule` must have start times, that is, the result + * of `isTransitionRule()` must be true. Otherwise, U_ILLEGAL_ARGUMENT_ERROR * is set to the error code. - * The input TimeZoneRule is adopted by this - * RuleBasedTimeZone on successful completion of this method, - * thus, the caller must not delete it when no error is returned. - * After all rules are added, the caller must call complete() method to - * make this RuleBasedTimeZone ready to handle common time + * The input `TimeZoneRule` is adopted by this `RuleBasedTimeZone`; + * the caller must not delete it. Should an error condition prevent + * the successful adoption of the rule, this function will delete it. + * + * After all rules are added, the caller must call `complete()` method to + * make this `RuleBasedTimeZone` ready to handle common time * zone functions. - * @param rule The TimeZoneRule. + * @param rule The `TimeZoneRule`. * @param status Output param to filled in with a success or an error. * @stable ICU 3.8 */ diff --git a/icu4c/source/i18n/vtzone.cpp b/icu4c/source/i18n/vtzone.cpp index cade6b7d316..9111e08848f 100644 --- a/icu4c/source/i18n/vtzone.cpp +++ b/icu4c/source/i18n/vtzone.cpp @@ -1894,23 +1894,25 @@ VTimeZone::writeSimple(UDate time, VTZWriter& writer, UErrorCode& status) const InitialTimeZoneRule *initial = nullptr; AnnualTimeZoneRule *std = nullptr, *dst = nullptr; getSimpleRulesNear(time, initial, std, dst, status); + LocalPointer lpInitial(initial); + LocalPointer lpStd(std); + LocalPointer lpDst(dst); if (U_SUCCESS(status)) { // Create a RuleBasedTimeZone with the subset rule getID(tzid); - RuleBasedTimeZone rbtz(tzid, initial); - if (std != nullptr && dst != nullptr) { - rbtz.addTransitionRule(std, status); - rbtz.addTransitionRule(dst, status); + RuleBasedTimeZone rbtz(tzid, lpInitial.orphan()); + if (lpStd.isValid() && lpDst.isValid()) { + rbtz.addTransitionRule(lpStd.orphan(), status); + rbtz.addTransitionRule(lpDst.orphan(), status); } if (U_FAILURE(status)) { - goto cleanupWriteSimple; + return; } if (olsonzid.length() > 0 && icutzver.length() > 0) { - UnicodeString *icutzprop = new UnicodeString(ICU_TZINFO_PROP); - if (icutzprop == nullptr) { - status = U_MEMORY_ALLOCATION_ERROR; - goto cleanupWriteSimple; + LocalPointer icutzprop(new UnicodeString(ICU_TZINFO_PROP), status); + if (U_FAILURE(status)) { + return; } icutzprop->append(olsonzid); icutzprop->append((UChar)0x005B/*'['*/); @@ -1918,26 +1920,10 @@ VTimeZone::writeSimple(UDate time, VTZWriter& writer, UErrorCode& status) const icutzprop->append(ICU_TZINFO_SIMPLE, -1); appendMillis(time, *icutzprop); icutzprop->append((UChar)0x005D/*']'*/); - customProps.addElementX(icutzprop, status); - if (U_FAILURE(status)) { - delete icutzprop; - goto cleanupWriteSimple; - } + customProps.adoptElement(icutzprop.orphan(), status); } writeZone(writer, rbtz, &customProps, status); } - return; - -cleanupWriteSimple: - if (initial != nullptr) { - delete initial; - } - if (std != nullptr) { - delete std; - } - if (dst != nullptr) { - delete dst; - } } void diff --git a/icu4c/source/test/intltest/tzrulets.cpp b/icu4c/source/test/intltest/tzrulets.cpp index d5ffc07d937..b55dd2ef7d7 100644 --- a/icu4c/source/test/intltest/tzrulets.cpp +++ b/icu4c/source/test/intltest/tzrulets.cpp @@ -402,8 +402,6 @@ TimeZoneRuleTest::TestSimpleRuleBasedTimeZone(void) { rbtz1->addTransitionRule(atzr, status); if (U_SUCCESS(status)) { errln("FAIL: 3rd final rule must be rejected"); - } else { - delete atzr; } // Try to add an initial rule @@ -411,8 +409,6 @@ TimeZoneRuleTest::TestSimpleRuleBasedTimeZone(void) { rbtz1->addTransitionRule(ir1, status); if (U_SUCCESS(status)) { errln("FAIL: InitialTimeZoneRule must be rejected"); - } else { - delete ir1; } delete ir;