From c48f218600303630ef7761104b1226655227af56 Mon Sep 17 00:00:00 2001 From: Andy Heninger Date: Tue, 9 Nov 2021 12:53:59 -0800 Subject: [PATCH] ICU-21763 UVector cleanup in Locale & Region Code Revise uses of UVector in Locale and Region related code 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 involve switching uses of UVector::addElementX() to the new adoptElement() or addElement() functions, as appropriate, and using LocalPointers for tracking memory ownership. In Region::loadRegionData(), improved the overall error detection and recovery. --- icu4c/source/common/localematcher.cpp | 42 ++----- icu4c/source/common/locid.cpp | 18 +-- icu4c/source/i18n/region.cpp | 152 ++++++++++++++------------ icu4c/source/i18n/region_impl.h | 6 +- 4 files changed, 108 insertions(+), 110 deletions(-) diff --git a/icu4c/source/common/localematcher.cpp b/icu4c/source/common/localematcher.cpp index 3d178dfbaf1..2cad708d99f 100644 --- a/icu4c/source/common/localematcher.cpp +++ b/icu4c/source/common/localematcher.cpp @@ -168,12 +168,9 @@ void LocaleMatcher::Builder::clearSupportedLocales() { bool LocaleMatcher::Builder::ensureSupportedLocaleVector() { if (U_FAILURE(errorCode_)) { return false; } if (supportedLocales_ != nullptr) { return true; } - supportedLocales_ = new UVector(uprv_deleteUObject, nullptr, errorCode_); + LocalPointer lpSupportedLocales(new UVector(uprv_deleteUObject, nullptr, errorCode_), errorCode_); if (U_FAILURE(errorCode_)) { return false; } - if (supportedLocales_ == nullptr) { - errorCode_ = U_MEMORY_ALLOCATION_ERROR; - return false; - } + supportedLocales_ = lpSupportedLocales.orphan(); return true; } @@ -187,9 +184,8 @@ LocaleMatcher::Builder &LocaleMatcher::Builder::setSupportedLocalesFromListStrin for (int32_t i = 0; i < length; ++i) { Locale *locale = list.orphanLocaleAt(i); if (locale == nullptr) { continue; } - supportedLocales_->addElementX(locale, errorCode_); + supportedLocales_->adoptElement(locale, errorCode_); if (U_FAILURE(errorCode_)) { - delete locale; break; } } @@ -197,35 +193,21 @@ LocaleMatcher::Builder &LocaleMatcher::Builder::setSupportedLocalesFromListStrin } LocaleMatcher::Builder &LocaleMatcher::Builder::setSupportedLocales(Locale::Iterator &locales) { - if (U_FAILURE(errorCode_)) { return *this; } - clearSupportedLocales(); - if (!ensureSupportedLocaleVector()) { return *this; } - while (locales.hasNext()) { - const Locale &locale = locales.next(); - Locale *clone = locale.clone(); - if (clone == nullptr) { - errorCode_ = U_MEMORY_ALLOCATION_ERROR; - break; - } - supportedLocales_->addElementX(clone, errorCode_); - if (U_FAILURE(errorCode_)) { - delete clone; - break; + if (ensureSupportedLocaleVector()) { + clearSupportedLocales(); + while (locales.hasNext() && U_SUCCESS(errorCode_)) { + const Locale &locale = locales.next(); + LocalPointer clone (locale.clone(), errorCode_); + supportedLocales_->adoptElement(clone.orphan(), errorCode_); } } return *this; } LocaleMatcher::Builder &LocaleMatcher::Builder::addSupportedLocale(const Locale &locale) { - if (!ensureSupportedLocaleVector()) { return *this; } - Locale *clone = locale.clone(); - if (clone == nullptr) { - errorCode_ = U_MEMORY_ALLOCATION_ERROR; - return *this; - } - supportedLocales_->addElementX(clone, errorCode_); - if (U_FAILURE(errorCode_)) { - delete clone; + if (ensureSupportedLocaleVector()) { + LocalPointer clone(locale.clone(), errorCode_); + supportedLocales_->adoptElement(clone.orphan(), errorCode_); } return *this; } diff --git a/icu4c/source/common/locid.cpp b/icu4c/source/common/locid.cpp index e8859c7048b..73bb8d8aec1 100644 --- a/icu4c/source/common/locid.cpp +++ b/icu4c/source/common/locid.cpp @@ -1204,14 +1204,11 @@ AliasReplacer::parseLanguageReplacement( // We have multiple field so we have to allocate and parse CharString* str = new CharString( replacement, (int32_t)uprv_strlen(replacement), status); + LocalPointer lpStr(str, status); + toBeFreed.adoptElement(lpStr.orphan(), status); if (U_FAILURE(status)) { return; } - if (str == nullptr) { - status = U_MEMORY_ALLOCATION_ERROR; - return; - } - toBeFreed.addElementX(str, status); char* data = str->data(); replacedLanguage = (const char*) data; char* endOfField = uprv_strchr(data, '_'); @@ -1420,12 +1417,9 @@ AliasReplacer::replaceTerritory(UVector& toBeFreed, UErrorCode& status) (int32_t)(firstSpace - replacement), status), status); } if (U_FAILURE(status)) { return false; } - if (item.isNull()) { - status = U_MEMORY_ALLOCATION_ERROR; - return false; - } replacedRegion = item->data(); - toBeFreed.addElementX(item.orphan(), status); + toBeFreed.adoptElement(item.orphan(), status); + if (U_FAILURE(status)) { return false; } } U_ASSERT(!same(region, replacedRegion)); region = replacedRegion; @@ -1659,10 +1653,10 @@ AliasReplacer::replace(const Locale& locale, CharString& out, UErrorCode& status while ((end = uprv_strchr(start, SEP_CHAR)) != nullptr && U_SUCCESS(status)) { *end = NULL_CHAR; // null terminate inside variantsBuff - variants.addElementX(start, status); + variants.addElement(start, status); start = end + 1; } - variants.addElementX(start, status); + variants.addElement(start, status); } if (U_FAILURE(status)) { return false; } diff --git a/icu4c/source/i18n/region.cpp b/icu4c/source/i18n/region.cpp index 2e013708bb8..277a22fd091 100644 --- a/icu4c/source/i18n/region.cpp +++ b/icu4c/source/i18n/region.cpp @@ -39,11 +39,6 @@ U_CDECL_BEGIN -static void U_CALLCONV -deleteRegion(void *obj) { - delete (icu::Region *)obj; -} - /** * Cleanup callback func */ @@ -90,7 +85,8 @@ void U_CALLCONV Region::loadRegionData(UErrorCode &status) { LocalPointer continents(new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status), status); LocalPointer groupings(new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status), status); - allRegions = new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status); + LocalPointer lpAllRegions(new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status), status); + allRegions = lpAllRegions.orphan(); LocalUResourceBundlePointer metadata(ures_openDirect(NULL,"metadata",&status)); LocalUResourceBundlePointer metadataAlias(ures_getByKey(metadata.getAlias(),"alias",NULL,&status)); @@ -109,16 +105,17 @@ void U_CALLCONV Region::loadRegionData(UErrorCode &status) { LocalUResourceBundlePointer worldContainment(ures_getByKey(territoryContainment.getAlias(),"001",NULL,&status)); LocalUResourceBundlePointer groupingContainment(ures_getByKey(territoryContainment.getAlias(),"grouping",NULL,&status)); + ucln_i18n_registerCleanup(UCLN_I18N_REGION, region_cleanup); if (U_FAILURE(status)) { return; } // now, initialize - uhash_setValueDeleter(newRegionIDMap.getAlias(), deleteRegion); // regionIDMap owns objs - uhash_setKeyDeleter(newRegionAliases.getAlias(), uprv_deleteUObject); // regionAliases owns the string keys + uhash_setValueDeleter(newRegionIDMap.getAlias(), uprv_deleteUObject); // regionIDMap owns objs + uhash_setKeyDeleter(newRegionAliases.getAlias(), uprv_deleteUObject); // regionAliases owns the string keys - while ( ures_hasNext(regionRegular.getAlias()) ) { + while (U_SUCCESS(status) && ures_hasNext(regionRegular.getAlias())) { UnicodeString regionName = ures_getNextUnicodeString(regionRegular.getAlias(),NULL,&status); int32_t rangeMarkerLocation = regionName.indexOf(RANGE_MARKER); UChar buf[6]; @@ -126,18 +123,18 @@ void U_CALLCONV Region::loadRegionData(UErrorCode &status) { if ( rangeMarkerLocation > 0 ) { UChar endRange = regionName.charAt(rangeMarkerLocation+1); buf[rangeMarkerLocation] = 0; - while ( buf[rangeMarkerLocation-1] <= endRange ) { + while (U_SUCCESS(status) && buf[rangeMarkerLocation-1] <= endRange) { LocalPointer newRegion(new UnicodeString(buf), status); - allRegions->addElementX(newRegion.orphan(),status); + allRegions->adoptElement(newRegion.orphan(), status); buf[rangeMarkerLocation-1]++; } } else { LocalPointer newRegion(new UnicodeString(regionName), status); - allRegions->addElementX(newRegion.orphan(),status); + allRegions->adoptElement(newRegion.orphan(), status); } } - while ( ures_hasNext(regionMacro.getAlias()) ) { + while (U_SUCCESS(status) && ures_hasNext(regionMacro.getAlias())) { UnicodeString regionName = ures_getNextUnicodeString(regionMacro.getAlias(),NULL,&status); int32_t rangeMarkerLocation = regionName.indexOf(RANGE_MARKER); UChar buf[6]; @@ -145,25 +142,29 @@ void U_CALLCONV Region::loadRegionData(UErrorCode &status) { if ( rangeMarkerLocation > 0 ) { UChar endRange = regionName.charAt(rangeMarkerLocation+1); buf[rangeMarkerLocation] = 0; - while ( buf[rangeMarkerLocation-1] <= endRange ) { + while ( buf[rangeMarkerLocation-1] <= endRange && U_SUCCESS(status)) { LocalPointer newRegion(new UnicodeString(buf), status); - allRegions->addElementX(newRegion.orphan(),status); + allRegions->adoptElement(newRegion.orphan(),status); buf[rangeMarkerLocation-1]++; } } else { LocalPointer newRegion(new UnicodeString(regionName), status); - allRegions->addElementX(newRegion.orphan(),status); + allRegions->adoptElement(newRegion.orphan(),status); } } - while ( ures_hasNext(regionUnknown.getAlias()) ) { - LocalPointer regionName (new UnicodeString(ures_getNextUnicodeString(regionUnknown.getAlias(),NULL,&status),status)); - allRegions->addElementX(regionName.orphan(),status); + while (U_SUCCESS(status) && ures_hasNext(regionUnknown.getAlias())) { + LocalPointer regionName ( + new UnicodeString(ures_getNextUnicodeString(regionUnknown.getAlias(), nullptr, &status), status)); + allRegions->adoptElement(regionName.orphan(),status); } - while ( ures_hasNext(worldContainment.getAlias()) ) { + while (U_SUCCESS(status) && ures_hasNext(worldContainment.getAlias())) { UnicodeString *continentName = new UnicodeString(ures_getNextUnicodeString(worldContainment.getAlias(),NULL,&status)); - continents->addElementX(continentName,status); + continents->adoptElement(continentName,status); + } + if (U_FAILURE(status)) { + return; } for ( int32_t i = 0 ; i < allRegions->size() ; i++ ) { @@ -191,22 +192,32 @@ void U_CALLCONV Region::loadRegionData(UErrorCode &status) { } UResourceBundle *groupingBundle = nullptr; - while ( ures_hasNext(groupingContainment.getAlias()) ) { + while (U_SUCCESS(status) && ures_hasNext(groupingContainment.getAlias())) { groupingBundle = ures_getNextResource(groupingContainment.getAlias(), groupingBundle, &status); if (U_FAILURE(status)) { break; } UnicodeString *groupingName = new UnicodeString(ures_getKey(groupingBundle), -1, US_INV); - groupings->addElementX(groupingName,status); - Region *grouping = (Region *) uhash_get(newRegionIDMap.getAlias(),groupingName); + LocalPointer lpGroupingName(groupingName, status); + groupings->adoptElement(lpGroupingName.orphan(), status); + if (U_FAILURE(status)) { + break; + } + Region *grouping = (Region *) uhash_get(newRegionIDMap.getAlias(), groupingName); if (grouping != NULL) { - for (int32_t i = 0; i < ures_getSize(groupingBundle); i++) { + for (int32_t i = 0; i < ures_getSize(groupingBundle) && U_SUCCESS(status); i++) { UnicodeString child = ures_getUnicodeStringByIndex(groupingBundle, i, &status); if (U_SUCCESS(status)) { if (grouping->containedRegions == NULL) { - grouping->containedRegions = new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status); + LocalPointer lpContainedRegions( + new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status), status); + grouping->containedRegions = lpContainedRegions.orphan(); + if (U_FAILURE(status)) { + break; + } } - grouping->containedRegions->addElementX(new UnicodeString(child), status); + LocalPointer lpChildCopy(new UnicodeString(child), status); + grouping->containedRegions->adoptElement(lpChildCopy.orphan(), status); } } } @@ -214,7 +225,7 @@ void U_CALLCONV Region::loadRegionData(UErrorCode &status) { ures_close(groupingBundle); // Process the territory aliases - while ( ures_hasNext(territoryAlias.getAlias()) ) { + while (U_SUCCESS(status) && ures_hasNext(territoryAlias.getAlias())) { LocalUResourceBundlePointer res(ures_getNextResource(territoryAlias.getAlias(),NULL,&status)); const char *aliasFrom = ures_getKey(res.getAlias()); LocalPointer aliasFromStr(new UnicodeString(aliasFrom, -1, US_INV), status); @@ -259,7 +270,7 @@ void U_CALLCONV Region::loadRegionData(UErrorCode &status) { } UnicodeString currentRegion; //currentRegion.remove(); TODO: was already 0 length? - for (int32_t i = 0 ; i < aliasTo.length() ; i++ ) { + for (int32_t i = 0 ; i < aliasTo.length() && U_SUCCESS(status); i++ ) { if ( aliasTo.charAt(i) != 0x0020 ) { currentRegion.append(aliasTo.charAt(i)); } @@ -267,7 +278,7 @@ void U_CALLCONV Region::loadRegionData(UErrorCode &status) { Region *target = (Region *)uhash_get(newRegionIDMap.getAlias(),(void *)¤tRegion); if (target) { LocalPointer preferredValue(new UnicodeString(target->idStr), status); - aliasFromRegion->preferredValues->addElementX((void *)preferredValue.orphan(),status); // may add null if err + aliasFromRegion->preferredValues->adoptElement(preferredValue.orphan(),status); // may add null if err } currentRegion.remove(); } @@ -276,9 +287,9 @@ void U_CALLCONV Region::loadRegionData(UErrorCode &status) { } // Process the code mappings - This will allow us to assign numeric codes to most of the territories. - while ( ures_hasNext(codeMappings.getAlias()) ) { + while (U_SUCCESS(status) && ures_hasNext(codeMappings.getAlias())) { UResourceBundle *mapping = ures_getNextResource(codeMappings.getAlias(),NULL,&status); - if ( ures_getType(mapping) == URES_ARRAY && ures_getSize(mapping) == 3) { + if (U_SUCCESS(status) && ures_getType(mapping) == URES_ARRAY && ures_getSize(mapping) == 3) { UnicodeString codeMappingID = ures_getUnicodeStringByIndex(mapping,0,&status); UnicodeString codeMappingNumber = ures_getUnicodeStringByIndex(mapping,1,&status); UnicodeString codeMapping3Letter = ures_getUnicodeStringByIndex(mapping,2,&status); @@ -356,15 +367,23 @@ void U_CALLCONV Region::loadRegionData(UErrorCode &status) { // Add the child region to the set of regions contained by the parent if (parentRegion->containedRegions == NULL) { - parentRegion->containedRegions = new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status); + LocalPointer lpContainedRegions( + new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status), status); + parentRegion->containedRegions = lpContainedRegions.orphan(); + if (U_FAILURE(status)) { + return; + } } LocalPointer childStr(new UnicodeString(), status); - if( U_FAILURE(status) ) { + if (U_FAILURE(status)) { return; // error out } childStr->fastCopyFrom(childRegion->idStr); - parentRegion->containedRegions->addElementX((void *)childStr.orphan(),status); + parentRegion->containedRegions->adoptElement(childStr.orphan(),status); + if (U_FAILURE(status)) { + return; + } // Set the parent region to be the containing region of the child. // Regions of type GROUPING can't be set as the parent, since another region @@ -388,10 +407,9 @@ void U_CALLCONV Region::loadRegionData(UErrorCode &status) { if( U_FAILURE(status) ) { return; // error out } - availableRegions[ar->fType]->addElementX((void *)arString.orphan(),status); + availableRegions[ar->fType]->adoptElement(arString.orphan(), status); } - ucln_i18n_registerCleanup(UCLN_I18N_REGION, region_cleanup); // copy hashtables numericCodeMap = newNumericCodeMap.orphan(); regionIDMap = newRegionIDMap.orphan(); @@ -402,6 +420,7 @@ void Region::cleanupRegionData() { for (int32_t i = 0 ; i < URGN_LIMIT ; i++ ) { if ( availableRegions[i] ) { delete availableRegions[i]; + availableRegions[i] = nullptr; } } @@ -417,7 +436,6 @@ void Region::cleanupRegionData() { uhash_close(regionIDMap); } if (allRegions) { - allRegions->removeAllElements(); // Don't need the temporary list anymore. delete allRegions; allRegions = NULL; } @@ -615,33 +633,30 @@ Region::getContainedRegions(UErrorCode &status) const { StringEnumeration* Region::getContainedRegions( URegionType type, UErrorCode &status ) const { umtx_initOnce(gRegionDataInitOnce, &loadRegionData, status); // returns immediately if U_FAILURE(status) + + UVector result(nullptr, uhash_compareChars, status); + LocalPointer cr(getContainedRegions(status), status); if (U_FAILURE(status)) { - return NULL; + return nullptr; } - UVector *result = new UVector(NULL, uhash_compareChars, status); - - StringEnumeration *cr = getContainedRegions(status); - - for ( int32_t i = 0 ; i < cr->count(status) ; i++ ) { - const char *regionId = cr->next(NULL,status); - const Region *r = Region::getInstance(regionId,status); + const char *regionId; + while((regionId = cr->next(nullptr, status)) != nullptr && U_SUCCESS(status)) { + const Region *r = Region::getInstance(regionId, status); if ( r->getType() == type) { - result->addElementX((void *)&r->idStr,status); + result.addElement(const_cast(&r->idStr), status); } else { - StringEnumeration *children = r->getContainedRegions(type, status); - for ( int32_t j = 0 ; j < children->count(status) ; j++ ) { - const char *id2 = children->next(NULL,status); + LocalPointer children(r->getContainedRegions(type, status)); + const char *id2; + while(U_SUCCESS(status) && ((id2 = children->next(nullptr, status)) != nullptr)) { const Region *r2 = Region::getInstance(id2,status); - result->addElementX((void *)&r2->idStr,status); + result.addElement(const_cast(&r2->idStr), status); } - delete children; } } - delete cr; - StringEnumeration* resultEnumeration = new RegionNameEnumeration(result,status); - delete result; - return resultEnumeration; + LocalPointer resultEnumeration( + new RegionNameEnumeration(&result, status), status); + return U_SUCCESS(status) ? resultEnumeration.orphan() : nullptr; } /** @@ -706,18 +721,21 @@ Region::getType() const { return fType; } -RegionNameEnumeration::RegionNameEnumeration(UVector *fNameList, UErrorCode& status) { - pos=0; - if (fNameList && U_SUCCESS(status)) { - fRegionNames = new UVector(uprv_deleteUObject, uhash_compareUnicodeString, fNameList->size(),status); - for ( int32_t i = 0 ; i < fNameList->size() ; i++ ) { - UnicodeString* this_region_name = (UnicodeString *)fNameList->elementAt(i); - UnicodeString* new_region_name = new UnicodeString(*this_region_name); - fRegionNames->addElementX((void *)new_region_name,status); +RegionNameEnumeration::RegionNameEnumeration(UVector *nameList, UErrorCode& status) : + pos(0), fRegionNames(nullptr) { + // TODO: https://unicode-org.atlassian.net/browse/ICU-21829 + // Is all of the copying going on here really necessary? + if (nameList && U_SUCCESS(status)) { + LocalPointer regionNames( + new UVector(uprv_deleteUObject, uhash_compareUnicodeString, nameList->size(), status), status); + for ( int32_t i = 0 ; U_SUCCESS(status) && i < nameList->size() ; i++ ) { + UnicodeString* this_region_name = (UnicodeString *)nameList->elementAt(i); + LocalPointer new_region_name(new UnicodeString(*this_region_name), status); + regionNames->adoptElement(new_region_name.orphan(), status); + } + if (U_SUCCESS(status)) { + fRegionNames = regionNames.orphan(); } - } - else { - fRegionNames = NULL; } } diff --git a/icu4c/source/i18n/region_impl.h b/icu4c/source/i18n/region_impl.h index 62acaa4511b..b6a281393f8 100644 --- a/icu4c/source/i18n/region_impl.h +++ b/icu4c/source/i18n/region_impl.h @@ -26,7 +26,11 @@ U_NAMESPACE_BEGIN class RegionNameEnumeration : public StringEnumeration { public: - RegionNameEnumeration(UVector *fNameList, UErrorCode& status); + /** + * Construct an string enumeration over the supplied name list. + * Makes a copy of the supplied input name list; does not retain a reference to the original. + */ + RegionNameEnumeration(UVector *nameList, UErrorCode& status); virtual ~RegionNameEnumeration(); static UClassID U_EXPORT2 getStaticClassID(void); virtual UClassID getDynamicClassID(void) const override;