]> granicus.if.org Git - icu/commitdiff
ICU-21763 UVector cleanup in Locale & Region Code
authorAndy Heninger <andy.heninger@gmail.com>
Tue, 9 Nov 2021 20:53:59 +0000 (12:53 -0800)
committerAndy Heninger <andy.heninger@gmail.com>
Fri, 12 Nov 2021 00:06:36 +0000 (16:06 -0800)
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
icu4c/source/common/locid.cpp
icu4c/source/i18n/region.cpp
icu4c/source/i18n/region_impl.h

index 3d178dfbaf17328aecbbd907126567c46fabd0b6..2cad708d99f0d2a9854ce318380093839196064f 100644 (file)
@@ -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<UVector> 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<Locale> 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<Locale> clone(locale.clone(), errorCode_);
+        supportedLocales_->adoptElement(clone.orphan(), errorCode_);
     }
     return *this;
 }
index e8859c7048b11008a3f9dbf9a5f2a5d9c717fa6b..73bb8d8aec1c703852632e9a7c580f21eb1d9ca1 100644 (file)
@@ -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<CharString> 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; }
 
index 2e013708bb88e3f3d2f6c136e88145c42194eae1..277a22fd091cfb094597bce7dbdf50b63c0f3cbc 100644 (file)
 
 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<UVector> continents(new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status), status);
     LocalPointer<UVector> groupings(new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status), status);
-    allRegions = new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status);
+    LocalPointer<UVector> 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<UnicodeString> newRegion(new UnicodeString(buf), status);
-                allRegions->addElementX(newRegion.orphan(),status);
+                allRegions->adoptElement(newRegion.orphan(), status);
                 buf[rangeMarkerLocation-1]++;
             }
         } else {
             LocalPointer<UnicodeString> 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<UnicodeString> newRegion(new UnicodeString(buf), status);
-                allRegions->addElementX(newRegion.orphan(),status);
+                allRegions->adoptElement(newRegion.orphan(),status);
                 buf[rangeMarkerLocation-1]++;
             }
         } else {
             LocalPointer<UnicodeString> newRegion(new UnicodeString(regionName), status);
-            allRegions->addElementX(newRegion.orphan(),status);
+            allRegions->adoptElement(newRegion.orphan(),status);
         }
     }
 
-    while ( ures_hasNext(regionUnknown.getAlias()) ) {
-        LocalPointer<UnicodeString> regionName (new UnicodeString(ures_getNextUnicodeString(regionUnknown.getAlias(),NULL,&status),status));
-        allRegions->addElementX(regionName.orphan(),status);
+    while (U_SUCCESS(status) && ures_hasNext(regionUnknown.getAlias())) {
+        LocalPointer<UnicodeString> 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<UnicodeString> 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<UVector> 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<UnicodeString> 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<UnicodeString> 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 *)&currentRegion);
                     if (target) {
                         LocalPointer<UnicodeString> 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<UVector> lpContainedRegions(
+                        new UVector(uprv_deleteUObject, uhash_compareUnicodeString, status), status);
+                    parentRegion->containedRegions = lpContainedRegions.orphan();
+                    if (U_FAILURE(status)) {
+                        return;
+                    }
                 }
 
                 LocalPointer<UnicodeString> 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<StringEnumeration> 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<UnicodeString *>(&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<StringEnumeration> 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<UnicodeString *>(&r2->idStr), status);
             }
-            delete children;
         }
     }
-    delete cr;
-    StringEnumeration* resultEnumeration = new RegionNameEnumeration(result,status);
-    delete result;
-    return resultEnumeration;
+    LocalPointer<StringEnumeration> 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<UVector> 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<UnicodeString> 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;
     }
 }
 
index 62acaa4511b49f8f3e61c978ed905f5693048943..b6a281393f891196caa5a40ff830fd31b763d66b 100644 (file)
@@ -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;