From 081cf773302a4ba23faa9bfdbbf3e5707c7f8b0e Mon Sep 17 00:00:00 2001 From: Andy Heninger Date: Wed, 28 Jul 2021 11:20:53 -0700 Subject: [PATCH] ICU-21662 Improve UVector error handling. - Add updated versions of UVector::addElement() and ensureCapacity() that respect incoming errors. Follow on to c26aebe, which renamed the original versions. - Add UVector::adoptElement() as a replacement for addElement() when the vector has a deleter function set, meaning that it adopts ownership of its elements. The intent is to make the behavior clearer at the call sites when looking at unfamiliar code. - Make all functions with an incoming failure, as indicated by a UErrorCode parameter, leave the vector unchanged. - Change all functions that store object pointers into the vector such that, when the store cannot be completed for any reason _and_ the vector has a deleter function, then the incoming object is deleted. This change can simplify the error handling code around calls to the affected functions (addElement() and insertElementAt(), in particular) - Add index bounds checking on functions where it was possible - that is, on functions with both U_ErrorCode and index parameters. - Changed to more modern C++ idioms in some parts of the UVector implementation. - Review & update as required all uses of the UVector functions setElementAt(), insertElementAt(), setSize(), sortedInsert() these being the functions with changed behavior on error conditions (aside from addElement()). This PR will be followed by more, switching call sites in various ICU services from UVector::addElementX() (old behavior on errors) to UVector::addElement() (new behavior on errors) --- icu4c/source/common/filteredbrk.cpp | 1 - icu4c/source/common/rbbi.cpp | 1 + icu4c/source/common/rbbitblb.cpp | 5 + icu4c/source/common/serv.cpp | 60 +++---- icu4c/source/common/uniset.cpp | 1 - icu4c/source/common/uvector.cpp | 220 ++++++++++++++---------- icu4c/source/common/uvector.h | 205 +++++++++------------- icu4c/source/i18n/alphaindex.cpp | 3 +- icu4c/source/i18n/rbt_pars.cpp | 44 ++++- icu4c/source/i18n/translit.cpp | 22 ++- icu4c/source/i18n/transreg.cpp | 29 ++-- icu4c/source/i18n/tridpars.cpp | 17 +- icu4c/source/test/intltest/transapi.cpp | 3 + icu4c/source/test/intltest/uvectest.cpp | 15 ++ 14 files changed, 343 insertions(+), 283 deletions(-) diff --git a/icu4c/source/common/filteredbrk.cpp b/icu4c/source/common/filteredbrk.cpp index 950dab8e702..9bced688702 100644 --- a/icu4c/source/common/filteredbrk.cpp +++ b/icu4c/source/common/filteredbrk.cpp @@ -90,7 +90,6 @@ class U_COMMON_API UStringSet : public UVector { } else { sortedInsert(str, compareUnicodeString, status); if(U_FAILURE(status)) { - delete str; return false; } return true; diff --git a/icu4c/source/common/rbbi.cpp b/icu4c/source/common/rbbi.cpp index 905d2aa09d4..f65177f2323 100644 --- a/icu4c/source/common/rbbi.cpp +++ b/icu4c/source/common/rbbi.cpp @@ -1260,6 +1260,7 @@ RuleBasedBreakIterator::getLanguageBreakEngine(UChar32 c) { // first. fLanguageBreakEngines->insertElementAt(fUnhandledBreakEngine, 0, status); // If we can't insert it, or creation failed, get rid of it + U_ASSERT(!fLanguageBreakEngines->hasDeleter()); if (U_FAILURE(status)) { delete fUnhandledBreakEngine; fUnhandledBreakEngine = 0; diff --git a/icu4c/source/common/rbbitblb.cpp b/icu4c/source/common/rbbitblb.cpp index e291d886916..ab24bf6ddf6 100644 --- a/icu4c/source/common/rbbitblb.cpp +++ b/icu4c/source/common/rbbitblb.cpp @@ -1042,6 +1042,8 @@ void RBBITableBuilder::sortedAdd(UVector **vector, int32_t val) { // //----------------------------------------------------------------------------- void RBBITableBuilder::setAdd(UVector *dest, UVector *source) { + U_ASSERT(!dest->hasDeleter()); + U_ASSERT(!source->hasDeleter()); int32_t destOriginalSize = dest->size(); int32_t sourceSize = source->size(); int32_t di = 0; @@ -1070,6 +1072,9 @@ void RBBITableBuilder::setAdd(UVector *dest, UVector *source) { (void) source->toArray(sourcePtr); dest->setSize(sourceSize+destOriginalSize, *fStatus); + if (U_FAILURE(*fStatus)) { + return; + } while (sourcePtr < sourceLim && destPtr < destLim) { if (*destPtr == *sourcePtr) { diff --git a/icu4c/source/common/serv.cpp b/icu4c/source/common/serv.cpp index e51d41c000c..0c54a4dce99 100644 --- a/icu4c/source/common/serv.cpp +++ b/icu4c/source/common/serv.cpp @@ -257,20 +257,13 @@ public: } }; -// UObjectDeleter for serviceCache +// Deleter for serviceCache U_CDECL_BEGIN static void U_CALLCONV cacheDeleter(void* obj) { U_NAMESPACE_USE ((CacheEntry*)obj)->unref(); } -/** -* Deleter for UObjects -*/ -static void U_CALLCONV -deleteUObject(void *obj) { - U_NAMESPACE_USE delete (UObject*) obj; -} U_CDECL_END /* @@ -534,11 +527,10 @@ ICUService::getKey(ICUServiceKey& key, UnicodeString* actualReturn, const ICUSer status = U_MEMORY_ALLOCATION_ERROR; return NULL; } - cacheDescriptorList->addElementX(idToCache.getAlias(), status); + cacheDescriptorList->adoptElement(idToCache.orphan(), status); if (U_FAILURE(status)) { return NULL; } - idToCache.orphan(); // cacheDescriptorList now owns the string. } while (key.fallback()); outerEnd: @@ -612,6 +604,7 @@ ICUService::getVisibleIDs(UVector& result, const UnicodeString* matchID, UErrorC if (U_FAILURE(status)) { return result; } + UObjectDeleter *savedDeleter = result.setDeleter(uprv_deleteUObject); { Mutex mutex(&lock); @@ -619,7 +612,7 @@ ICUService::getVisibleIDs(UVector& result, const UnicodeString* matchID, UErrorC if (map != NULL) { ICUServiceKey* fallbackKey = createKey(matchID, status); - for (int32_t pos = UHASH_FIRST;;) { + for (int32_t pos = UHASH_FIRST; U_SUCCESS(status); ) { const UHashElement* e = map->nextElement(pos); if (e == NULL) { break; @@ -632,17 +625,11 @@ ICUService::getVisibleIDs(UVector& result, const UnicodeString* matchID, UErrorC } } - UnicodeString* idClone = new UnicodeString(*id); - if (idClone == NULL || idClone->isBogus()) { - delete idClone; + LocalPointer idClone(new UnicodeString(*id), status); + if (U_SUCCESS(status) && idClone->isBogus()) { status = U_MEMORY_ALLOCATION_ERROR; - break; - } - result.addElementX(idClone, status); - if (U_FAILURE(status)) { - delete idClone; - break; } + result.adoptElement(idClone.orphan(), status); } delete fallbackKey; } @@ -650,6 +637,7 @@ ICUService::getVisibleIDs(UVector& result, const UnicodeString* matchID, UErrorC if (U_FAILURE(status)) { result.removeAllElements(); } + result.setDeleter(savedDeleter); return result; } @@ -797,7 +785,7 @@ ICUService::getDisplayNames(UVector& result, } const UnicodeString* dn = (const UnicodeString*)entry->key.pointer; StringPair* sp = StringPair::create(*id, *dn, status); - result.addElementX(sp, status); + result.adoptElement(sp, status); if (U_FAILURE(status)) { result.removeAllElements(); break; @@ -845,32 +833,34 @@ ICUService::createSimpleFactory(UObject* objToAdopt, const UnicodeString& id, UB } URegistryKey -ICUService::registerFactory(ICUServiceFactory* factoryToAdopt, UErrorCode& status) +ICUService::registerFactory(ICUServiceFactory* factoryToAdopt, UErrorCode& status) { - if (U_SUCCESS(status) && factoryToAdopt != NULL) { + LocalPointerlpFactoryToAdopt(factoryToAdopt); + if (U_FAILURE(status) || factoryToAdopt == nullptr) { + return nullptr; + } + { Mutex mutex(&lock); - if (factories == NULL) { - factories = new UVector(deleteUObject, NULL, status); + if (factories == nullptr) { + LocalPointer lpFactories(new UVector(uprv_deleteUObject, nullptr, status), status); if (U_FAILURE(status)) { - delete factories; - return NULL; + return nullptr; } + factories = lpFactories.orphan(); } - factories->insertElementAt(factoryToAdopt, 0, status); + factories->insertElementAt(lpFactoryToAdopt.orphan(), 0, status); if (U_SUCCESS(status)) { clearCaches(); - } else { - delete factoryToAdopt; - factoryToAdopt = NULL; } - } + } // Close of mutex lock block. - if (factoryToAdopt != NULL) { + if (U_SUCCESS(status)) { notifyChanged(); + return (URegistryKey)factoryToAdopt; + } else { + return nullptr; } - - return (URegistryKey)factoryToAdopt; } UBool diff --git a/icu4c/source/common/uniset.cpp b/icu4c/source/common/uniset.cpp index 12764c28c5c..47993361f2f 100644 --- a/icu4c/source/common/uniset.cpp +++ b/icu4c/source/common/uniset.cpp @@ -984,7 +984,6 @@ void UnicodeSet::_add(const UnicodeString& s) { strings->sortedInsert(t, compareUnicodeString, ec); if (U_FAILURE(ec)) { setToBogus(); - delete t; } } diff --git a/icu4c/source/common/uvector.cpp b/icu4c/source/common/uvector.cpp index 5a7f33b64fe..4da8b864e1b 100644 --- a/icu4c/source/common/uvector.cpp +++ b/icu4c/source/common/uvector.cpp @@ -17,59 +17,34 @@ U_NAMESPACE_BEGIN -#define DEFAULT_CAPACITY 8 +constexpr int32_t DEFAULT_CAPACITY = 8; /* * Constants for hinting whether a key is an integer * or a pointer. If a hint bit is zero, then the associated * token is assumed to be an integer. This is needed for iSeries */ -#define HINT_KEY_POINTER (1) -#define HINT_KEY_INTEGER (0) +constexpr int8_t HINT_KEY_POINTER = 1; +constexpr int8_t HINT_KEY_INTEGER = 0; UOBJECT_DEFINE_RTTI_IMPLEMENTATION(UVector) UVector::UVector(UErrorCode &status) : - count(0), - capacity(0), - elements(0), - deleter(0), - comparer(0) -{ - _init(DEFAULT_CAPACITY, status); + UVector(nullptr, nullptr, DEFAULT_CAPACITY, status) { } UVector::UVector(int32_t initialCapacity, UErrorCode &status) : - count(0), - capacity(0), - elements(0), - deleter(0), - comparer(0) -{ - _init(initialCapacity, status); + UVector(nullptr, nullptr, initialCapacity, status) { } UVector::UVector(UObjectDeleter *d, UElementsAreEqual *c, UErrorCode &status) : - count(0), - capacity(0), - elements(0), - deleter(d), - comparer(c) -{ - _init(DEFAULT_CAPACITY, status); + UVector(d, c, DEFAULT_CAPACITY, status) { } UVector::UVector(UObjectDeleter *d, UElementsAreEqual *c, int32_t initialCapacity, UErrorCode &status) : - count(0), - capacity(0), - elements(0), deleter(d), comparer(c) { - _init(initialCapacity, status); -} - -void UVector::_init(int32_t initialCapacity, UErrorCode &status) { if (U_FAILURE(status)) { return; } @@ -78,7 +53,7 @@ void UVector::_init(int32_t initialCapacity, UErrorCode &status) { initialCapacity = DEFAULT_CAPACITY; } elements = (UElement *)uprv_malloc(sizeof(UElement)*initialCapacity); - if (elements == 0) { + if (elements == nullptr) { status = U_MEMORY_ALLOCATION_ERROR; } else { capacity = initialCapacity; @@ -88,7 +63,7 @@ void UVector::_init(int32_t initialCapacity, UErrorCode &status) { UVector::~UVector() { removeAllElements(); uprv_free(elements); - elements = 0; + elements = nullptr; } /** @@ -96,11 +71,11 @@ UVector::~UVector() { * Use the 'assign' function to assign each element. */ void UVector::assign(const UVector& other, UElementAssigner *assign, UErrorCode &ec) { - if (ensureCapacityX(other.count, ec)) { + if (ensureCapacity(other.count, ec)) { setSize(other.count, ec); if (U_SUCCESS(ec)) { for (int32_t i=0; iindex; --i) { - elements[i] = elements[i-1]; + if (ensureCapacity(count + 1, status)) { + if (0 <= index && index <= count) { + for (int32_t i=count; i>index; --i) { + elements[i] = elements[i-1]; + } + elements[index].pointer = obj; + ++count; + } else { + /* index out of range */ + status = U_ILLEGAL_ARGUMENT_ERROR; } - elements[index].pointer = obj; - ++count; } - /* else index out of range */ + if (U_FAILURE(status) && deleter != nullptr) { + (*deleter)(obj); + } } void UVector::insertElementAt(int32_t elem, int32_t index, UErrorCode &status) { + U_ASSERT(deleter == nullptr); // Usage error. Mixing up ints and pointers. // must have 0 <= index <= count - if (0 <= index && index <= count && ensureCapacityX(count + 1, status)) { - for (int32_t i=count; i>index; --i) { - elements[i] = elements[i-1]; + if (ensureCapacity(count + 1, status)) { + if (0 <= index && index <= count) { + for (int32_t i=count; i>index; --i) { + elements[i] = elements[i-1]; + } + elements[index].pointer = nullptr; + elements[index].integer = elem; + ++count; + } else { + /* index out of range */ + status = U_ILLEGAL_ARGUMENT_ERROR; } - elements[index].pointer = NULL; - elements[index].integer = elem; - ++count; } - /* else index out of range */ } void* UVector::elementAt(int32_t index) const { @@ -237,7 +242,7 @@ UBool UVector::retainAll(const UVector& other) { void UVector::removeElementAt(int32_t index) { void* e = orphanElementAt(index); - if (e != 0 && deleter != 0) { + if (e != nullptr && deleter != nullptr) { (*deleter)(e); } } @@ -252,9 +257,9 @@ UBool UVector::removeElement(void* obj) { } void UVector::removeAllElements(void) { - if (deleter != 0) { + if (deleter != nullptr) { for (int32_t i=0; icount != other.count) { return FALSE; } - if (comparer == 0) { + if (comparer == nullptr) { for (i=0; i (INT32_MAX - 1) / 2) { // integer overflow check + status = U_ILLEGAL_ARGUMENT_ERROR; + return false; + } + int32_t newCap = capacity * 2; + if (newCap < minimumCapacity) { + newCap = minimumCapacity; + } + if (newCap > (int32_t)(INT32_MAX / sizeof(UElement))) { // integer overflow check + // We keep the original memory contents on bad minimumCapacity. + status = U_ILLEGAL_ARGUMENT_ERROR; + return false; + } + UElement* newElems = (UElement *)uprv_realloc(elements, sizeof(UElement)*newCap); + if (newElems == nullptr) { + // We keep the original contents on the memory failure on realloc or bad minimumCapacity. + status = U_MEMORY_ALLOCATION_ERROR; + return false; + } + elements = newElems; + capacity = newCap; + } + return true; +} /** * Change the size of this vector as follows: If newSize is smaller, * then truncate the array, possibly deleting held elements for i >= * newSize. If newSize is larger, grow the array, filling in new - * slots with NULL. + * slots with nullptr. */ void UVector::setSize(int32_t newSize, UErrorCode &status) { - int32_t i; - if (newSize < 0) { + if (!ensureCapacity(newSize, status)) { return; } if (newSize > count) { - if (!ensureCapacityX(newSize, status)) { - return; - } UElement empty; - empty.pointer = NULL; + empty.pointer = nullptr; empty.integer = 0; - for (i=count; i=newSize; --i) { + for (int32_t i=count-1; i>=newSize; --i) { removeElementAt(i); } } @@ -422,7 +455,7 @@ UElementsAreEqual *UVector::setComparer(UElementsAreEqual *d) { * then 0 is returned and the vector is unchanged. */ void* UVector::orphanElementAt(int32_t index) { - void* e = 0; + void* e = nullptr; if (0 <= index && index < count) { e = elements[index].pointer; for (int32_t i=index; imin; --i) { - elements[i] = elements[i-1]; - } - elements[min] = e; - ++count; + for (int32_t i=count; i>min; --i) { + elements[i] = elements[i-1]; } + elements[min] = e; + ++count; } /** @@ -526,7 +564,7 @@ sortiComparator(const void * /*context */, const void *left, const void *right) void UVector::sorti(UErrorCode &ec) { if (U_SUCCESS(ec)) { uprv_sortArray(elements, count, sizeof(UElement), - sortiComparator, NULL, FALSE, &ec); + sortiComparator, nullptr, FALSE, &ec); } } diff --git a/icu4c/source/common/uvector.h b/icu4c/source/common/uvector.h index 02110f5bbb2..ff086308594 100644 --- a/icu4c/source/common/uvector.h +++ b/icu4c/source/common/uvector.h @@ -23,43 +23,44 @@ U_NAMESPACE_BEGIN /** - *

Ultralightweight C++ implementation of a void* vector + * Ultralightweight C++ implementation of a `void*` vector * that is (mostly) compatible with java.util.Vector. * - *

This is a very simple implementation, written to satisfy an + * This is a very simple implementation, written to satisfy an * immediate porting need. As such, it is not completely fleshed out, * and it aims for simplicity and conformity. Nonetheless, it serves * its purpose (porting code from java that uses java.util.Vector) * well, and it could be easily made into a more robust vector class. * - *

Design notes + * *Design notes* * - *

There is index bounds checking, but little is done about it. If + * There is index bounds checking, but little is done about it. If * indices are out of bounds, either nothing happens, or zero is - * returned. We do avoid indexing off into the weeds. + * returned. We *do* avoid indexing off into the weeds. * - *

There is detection of out of memory, but the handling is very - * coarse-grained -- similar to UnicodeString's protocol, but even - * coarser. The class contains one static flag that is set - * when any call to new returns zero. This allows the caller - * to use several vectors and make just one check at the end to see if - * a memory failure occurred. This is more efficient than making a - * check after each call on each vector when doing many operations on - * multiple vectors. The single static flag works best when memory - * failures are infrequent, and when recovery options are limited or - * nonexistent. - * - *

Since we don't have garbage collection, UVector was given the - * option to ownits contents. To employ this, set a deleter - * function. The deleter is called on a void* pointer when that + * Since we don't have garbage collection, UVector was given the + * option to *own* its contents. To employ this, set a deleter + * function. The deleter is called on a `void *` pointer when that * pointer is released by the vector, either when the vector itself is - * destructed, or when a call to setElementAt() overwrites an element, - * or when a call to remove() or one of its variants explicitly + * destructed, or when a call to `setElementAt()` overwrites an element, + * or when a call to remove()` or one of its variants explicitly * removes an element. If no deleter is set, or the deleter is set to * zero, then it is assumed that the caller will delete elements as * needed. * - *

In order to implement methods such as contains() and indexOf(), + * *Error Handling* Functions that can fail, from out of memory conditions + * for example, include a UErrorCode parameter. Any function called + * with an error code already indicating a failure will not modify the + * vector in any way. + * + * For vectors that have a deleter function, any failure in inserting + * an element into the vector will instead delete the element that + * could not be adopted. This simplifies object ownership + * management around calls to `addElement()` and `insertElementAt()`; + * error or no, the function always takes ownership of an incoming object + * from the caller. + * + * In order to implement methods such as `contains()` and `indexOf()`, * UVector needs a way to compare objects for equality. To do so, it * uses a comparison function, or "comparer." If the comparer is not * set, or is set to zero, then all such methods will act as if the @@ -73,23 +74,21 @@ U_NAMESPACE_BEGIN * @author Alan Liu */ class U_COMMON_API UVector : public UObject { - // NOTE: UVector uses the UHashKey (union of void* and int32_t) as + // NOTE: UVector uses the UElement (union of void* and int32_t) as // its basic storage type. It uses UElementsAreEqual as its // comparison function. It uses UObjectDeleter as its deleter - // function. These are named for hashtables, but used here as-is - // rather than duplicating the type. This allows sharing of - // support functions. + // function. This allows sharing of support functions with UHashtable. private: - int32_t count; + int32_t count = 0; - int32_t capacity; + int32_t capacity = 0; - UElement* elements; + UElement* elements = nullptr; - UObjectDeleter *deleter; + UObjectDeleter *deleter = nullptr; - UElementsAreEqual *comparer; + UElementsAreEqual *comparer = nullptr; public: UVector(UErrorCode &status); @@ -113,12 +112,12 @@ public: * equal if they are of the same size and all elements are equal, * as compared using this object's comparer. */ - bool operator==(const UVector& other); + bool operator==(const UVector& other) const; /** * Equivalent to !operator==() */ - inline bool operator!=(const UVector& other); + inline bool operator!=(const UVector& other) const {return !operator==(other);} //------------------------------------------------------------ // java.util.Vector API @@ -130,6 +129,28 @@ public: */ void addElementX(void* obj, UErrorCode &status); + /** + * Add an element at the end of the vector. + * For use only with vectors that do not adopt their elements, which is to say, + * have not set an element deleter function. See `adoptElement()`. + */ + void addElement(void *obj, UErrorCode &status); + + /** + * Add an element at the end of the vector. + * For use only with vectors that adopt their elements, which is to say, + * have set an element deleter function. See `addElement()`. + * + * If the element cannot be successfully added, it will be deleted. This is + * normal ICU _adopt_ behavior - one way or another ownership of the incoming + * object is transferred from the caller. + * + * `addElement()` and `adoptElement()` are separate functions to make it easier + * to see what the function is doing at call sites. Having a single combined function, + * as in earlier versions of UVector, had proved to be error-prone. + */ + void adoptElement(void *obj, UErrorCode &status); + void addElement(int32_t elem, UErrorCode &status); void setElementAt(void* obj, int32_t index); @@ -146,19 +167,19 @@ public: UBool equals(const UVector &other) const; - inline void* firstElement(void) const; + inline void* firstElement(void) const {return elementAt(0);} - inline void* lastElement(void) const; + inline void* lastElement(void) const {return elementAt(count-1);} - inline int32_t lastElementi(void) const; + inline int32_t lastElementi(void) const {return elementAti(count-1);} int32_t indexOf(void* obj, int32_t startIndex = 0) const; int32_t indexOf(int32_t obj, int32_t startIndex = 0) const; - inline UBool contains(void* obj) const; + inline UBool contains(void* obj) const {return indexOf(obj) >= 0;} - inline UBool contains(int32_t obj) const; + inline UBool contains(int32_t obj) const {return indexOf(obj) >= 0;} UBool containsAll(const UVector& other) const; @@ -172,9 +193,9 @@ public: void removeAllElements(); - inline int32_t size(void) const; + inline int32_t size(void) const {return count;} - inline UBool isEmpty(void) const; + inline UBool isEmpty(void) const {return count == 0;} /* * Old version of ensureCapacity, with non-standard error handling. @@ -182,6 +203,8 @@ public: */ UBool ensureCapacityX(int32_t minimumCapacity, UErrorCode &status); + UBool ensureCapacity(int32_t minimumCapacity, UErrorCode &status); + /** * Change the size of this vector as follows: If newSize is * smaller, then truncate the array, possibly deleting held @@ -200,10 +223,11 @@ public: //------------------------------------------------------------ UObjectDeleter *setDeleter(UObjectDeleter *d); + bool hasDeleter() {return deleter != nullptr;} UElementsAreEqual *setComparer(UElementsAreEqual *c); - inline void* operator[](int32_t index) const; + inline void* operator[](int32_t index) const {return elementAt(index);} /** * Removes the element at the given index from this vector and @@ -271,33 +295,32 @@ public: virtual UClassID getDynamicClassID() const; private: - void _init(int32_t initialCapacity, UErrorCode &status); - int32_t indexOf(UElement key, int32_t startIndex = 0, int8_t hint = 0) const; void sortedInsert(UElement e, UElementComparator *compare, UErrorCode& ec); +public: // Disallow - UVector(const UVector&); + UVector(const UVector&) = delete; // Disallow - UVector& operator=(const UVector&); + UVector& operator=(const UVector&) = delete; }; /** - *

Ultralightweight C++ implementation of a void* stack + * Ultralightweight C++ implementation of a `void*` stack * that is (mostly) compatible with java.util.Stack. As in java, this * is merely a paper thin layer around UVector. See the UVector * documentation for further information. * - *

Design notes + * *Design notes* * - *

The element at index n-1 is (of course) the top of the + * The element at index `n-1` is (of course) the top of the * stack. * - *

The poorly named empty() method doesn't empty the + * The poorly named `empty()` method doesn't empty the * stack; it determines if the stack is empty. * * @author Alan Liu @@ -317,19 +340,25 @@ public: // It's okay not to have a virtual destructor (in UVector) // because UStack has no special cleanup to do. - inline UBool empty(void) const; + inline UBool empty(void) const {return isEmpty();} - inline void* peek(void) const; + inline void* peek(void) const {return lastElement();} - inline int32_t peeki(void) const; + inline int32_t peeki(void) const {return lastElementi();} void* pop(void); int32_t popi(void); - inline void* push(void* obj, UErrorCode &status); + inline void* push(void* obj, UErrorCode &status) { + addElementX(obj, status); + return obj; + } - inline int32_t push(int32_t i, UErrorCode &status); + inline int32_t push(int32_t i, UErrorCode &status) { + addElement(i, status); + return i; + } /* If the object o occurs as an item in this stack, @@ -347,77 +376,13 @@ public: */ virtual UClassID getDynamicClassID() const; -private: // Disallow - UStack(const UStack&); + UStack(const UStack&) = delete; // Disallow - UStack& operator=(const UStack&); + UStack& operator=(const UStack&) = delete; }; - -// UVector inlines - -inline int32_t UVector::size(void) const { - return count; -} - -inline UBool UVector::isEmpty(void) const { - return count == 0; -} - -inline UBool UVector::contains(void* obj) const { - return indexOf(obj) >= 0; -} - -inline UBool UVector::contains(int32_t obj) const { - return indexOf(obj) >= 0; -} - -inline void* UVector::firstElement(void) const { - return elementAt(0); -} - -inline void* UVector::lastElement(void) const { - return elementAt(count-1); -} - -inline int32_t UVector::lastElementi(void) const { - return elementAti(count-1); -} - -inline void* UVector::operator[](int32_t index) const { - return elementAt(index); -} - -inline bool UVector::operator!=(const UVector& other) { - return !operator==(other); -} - -// UStack inlines - -inline UBool UStack::empty(void) const { - return isEmpty(); -} - -inline void* UStack::peek(void) const { - return lastElement(); -} - -inline int32_t UStack::peeki(void) const { - return lastElementi(); -} - -inline void* UStack::push(void* obj, UErrorCode &status) { - addElementX(obj, status); - return obj; -} - -inline int32_t UStack::push(int32_t i, UErrorCode &status) { - addElement(i, status); - return i; -} - U_NAMESPACE_END #endif diff --git a/icu4c/source/i18n/alphaindex.cpp b/icu4c/source/i18n/alphaindex.cpp index 6be2cb6ced3..34407f677a6 100644 --- a/icu4c/source/i18n/alphaindex.cpp +++ b/icu4c/source/i18n/alphaindex.cpp @@ -293,6 +293,7 @@ int32_t AlphabeticIndex::getRecordCount(UErrorCode &status) { } void AlphabeticIndex::initLabels(UVector &indexCharacters, UErrorCode &errorCode) const { + U_ASSERT(indexCharacters.hasDeleter()); const Normalizer2 *nfkdNormalizer = Normalizer2::getNFKDInstance(errorCode); if (U_FAILURE(errorCode)) { return; } @@ -305,7 +306,7 @@ void AlphabeticIndex::initLabels(UVector &indexCharacters, UErrorCode &errorCode // That is, we might have c, ch, d, where "ch" sorts just like "c", "h". // We filter out those cases. UnicodeSetIterator iter(*initialLabels_); - while (iter.next()) { + while (U_SUCCESS(errorCode) && iter.next()) { const UnicodeString *item = &iter.getString(); LocalPointer ownedItem; UBool checkDistinct; diff --git a/icu4c/source/i18n/rbt_pars.cpp b/icu4c/source/i18n/rbt_pars.cpp index 8b62cac801c..44e96c659a0 100644 --- a/icu4c/source/i18n/rbt_pars.cpp +++ b/icu4c/source/i18n/rbt_pars.cpp @@ -975,10 +975,14 @@ void TransliteratorParser::parseRules(const UnicodeString& rule, if (!parsingIDs) { if (curData != NULL) { + U_ASSERT(!dataVector.hasDeleter()); if (direction == UTRANS_FORWARD) - dataVector.addElementX(curData, status); + dataVector.addElement(curData, status); else dataVector.insertElementAt(curData, 0, status); + if (U_FAILURE(status)) { + delete curData; + } curData = NULL; } parsingIDs = TRUE; @@ -1031,10 +1035,14 @@ void TransliteratorParser::parseRules(const UnicodeString& rule, status = U_MEMORY_ALLOCATION_ERROR; return; } + U_ASSERT(idBlockVector.hasDeleter()); if (direction == UTRANS_FORWARD) - idBlockVector.addElementX(tempstr, status); + idBlockVector.adoptElement(tempstr, status); else idBlockVector.insertElementAt(tempstr, 0, status); + if (U_FAILURE(status)) { + return; + } idBlockResult.remove(); parsingIDs = FALSE; curData = new TransliterationRuleData(status); @@ -1069,19 +1077,29 @@ void TransliteratorParser::parseRules(const UnicodeString& rule, tempstr = new UnicodeString(idBlockResult); // NULL pointer check if (tempstr == NULL) { + // TODO: Testing, forcing this path, shows many memory leaks. ICU-21701 + // intltest translit/TransliteratorTest/TestInstantiation status = U_MEMORY_ALLOCATION_ERROR; return; } if (direction == UTRANS_FORWARD) - idBlockVector.addElementX(tempstr, status); + idBlockVector.adoptElement(tempstr, status); else idBlockVector.insertElementAt(tempstr, 0, status); + if (U_FAILURE(status)) { + return; + } } else if (!parsingIDs && curData != NULL) { - if (direction == UTRANS_FORWARD) - dataVector.addElementX(curData, status); - else + if (direction == UTRANS_FORWARD) { + dataVector.addElement(curData, status); + } else { dataVector.insertElementAt(curData, 0, status); + } + if (U_FAILURE(status)) { + delete curData; + curData = nullptr; + } } if (U_SUCCESS(status)) { @@ -1537,7 +1555,11 @@ UChar TransliteratorParser::generateStandInFor(UnicodeFunctor* adopted, UErrorCo status = U_VARIABLE_RANGE_EXHAUSTED; return 0; } - variablesVector.addElementX(adopted, status); + variablesVector.addElement(adopted, status); + if (U_FAILURE(status)) { + delete adopted; + return 0; + } return variableNext++; } @@ -1560,7 +1582,7 @@ UChar TransliteratorParser::getSegmentStandin(int32_t seg, UErrorCode& status) { // Set a placeholder in the primary variables vector that will be // filled in later by setSegmentObject(). We know that we will get // called first because setSegmentObject() will call us. - variablesVector.addElementX((void*) NULL, status); + variablesVector.addElement((void*) NULL, status); segmentStandins.setCharAt(seg-1, c); } return c; @@ -1577,13 +1599,17 @@ void TransliteratorParser::setSegmentObject(int32_t seg, StringMatcher* adopted, if (segmentObjects.size() < seg) { segmentObjects.setSize(seg, status); } + if (U_FAILURE(status)) { + return; + } int32_t index = getSegmentStandin(seg, status) - curData->variablesBase; if (segmentObjects.elementAt(seg-1) != NULL || variablesVector.elementAt(index) != NULL) { // should never happen - status = U_INTERNAL_TRANSLITERATOR_ERROR; + if (U_SUCCESS(status)) {status = U_INTERNAL_TRANSLITERATOR_ERROR;} return; } + // Note: neither segmentObjects or variablesVector has an object deleter function. segmentObjects.setElementAt(adopted, seg-1); variablesVector.setElementAt(adopted, index); } diff --git a/icu4c/source/i18n/translit.cpp b/icu4c/source/i18n/translit.cpp index 64e9fe1e59d..c7d6b510576 100644 --- a/icu4c/source/i18n/translit.cpp +++ b/icu4c/source/i18n/translit.cpp @@ -1093,6 +1093,8 @@ Transliterator::createFromRules(const UnicodeString& ID, } else { UVector transliterators(status); + // TODO ICU-21701 missing U_FAILURE check here. + // Error and nullptr checking through this whole block looks suspect. int32_t passNumber = 1; int32_t limit = parser.idBlockVector.size(); @@ -1108,10 +1110,15 @@ Transliterator::createFromRules(const UnicodeString& ID, delete temp; return nullptr; } - if (temp != NULL && typeid(*temp) != typeid(NullTransliterator)) - transliterators.addElementX(temp, status); - else + if (temp != NULL && typeid(*temp) != typeid(NullTransliterator)) { + transliterators.addElement(temp, status); + if (U_FAILURE(status)) { + delete temp; + return nullptr; + } + } else { delete temp; + } } } if (!parser.dataVector.isEmpty()) { @@ -1126,7 +1133,14 @@ Transliterator::createFromRules(const UnicodeString& ID, } return t; } - transliterators.addElementX(temprbt, status); + transliterators.addElement(temprbt, status); + if (U_FAILURE(status)) { + delete temprbt; + return t; + } + // TODO: ICU-21701 the transliterators vector will leak its contents if anything goes wrong. + // Under normal operation, the CompoundTransliterator constructor adopts the + // the contents of the vector. } } diff --git a/icu4c/source/i18n/transreg.cpp b/icu4c/source/i18n/transreg.cpp index 1632bbcba5b..918e936223e 100644 --- a/icu4c/source/i18n/transreg.cpp +++ b/icu4c/source/i18n/transreg.cpp @@ -154,22 +154,23 @@ Transliterator* TransliteratorAlias::create(UParseError& pe, pos = aliasesOrRules.indexOf(noIDBlock, pos + 1); } - UVector transliterators(ec); + UVector transliterators(uprv_deleteUObject, nullptr, ec); UnicodeString idBlock; int32_t blockSeparatorPos = aliasesOrRules.indexOf((UChar)(0xffff)); while (blockSeparatorPos >= 0) { aliasesOrRules.extract(0, blockSeparatorPos, idBlock); aliasesOrRules.remove(0, blockSeparatorPos + 1); if (!idBlock.isEmpty()) - transliterators.addElementX(Transliterator::createInstance(idBlock, UTRANS_FORWARD, pe, ec), ec); + transliterators.adoptElement(Transliterator::createInstance(idBlock, UTRANS_FORWARD, pe, ec), ec); if (!transes->isEmpty()) - transliterators.addElementX(transes->orphanElementAt(0), ec); + transliterators.adoptElement(transes->orphanElementAt(0), ec); blockSeparatorPos = aliasesOrRules.indexOf((UChar)(0xffff)); } if (!aliasesOrRules.isEmpty()) - transliterators.addElementX(Transliterator::createInstance(aliasesOrRules, UTRANS_FORWARD, pe, ec), ec); + transliterators.adoptElement(Transliterator::createInstance(aliasesOrRules, UTRANS_FORWARD, pe, ec), ec); while (!transes->isEmpty()) - transliterators.addElementX(transes->orphanElementAt(0), ec); + transliterators.adoptElement(transes->orphanElementAt(0), ec); + transliterators.setDeleter(nullptr); if (U_SUCCESS(ec)) { t = new CompoundTransliterator(ID, transliterators, @@ -543,7 +544,7 @@ TransliteratorRegistry::TransliteratorRegistry(UErrorCode& status) : variantList.setComparer(uhash_compareCaselessUnicodeString); UnicodeString *emptyString = new UnicodeString(); if (emptyString != NULL) { - variantList.addElementX(emptyString, status); + variantList.adoptElement(emptyString, status); } availableIDs.setDeleter(uprv_deleteUObject); availableIDs.setComparer(uhash_compareCaselessUnicodeString); @@ -611,6 +612,8 @@ Transliterator* TransliteratorRegistry::reget(const UnicodeString& ID, entry->entryType = TransliteratorEntry::COMPOUND_RBT; entry->compoundFilter = parser.orphanCompoundFilter(); entry->u.dataVector = new UVector(status); + // TODO ICU-21701: missing check for nullptr and failed status. + // Unclear how best to bail out. entry->stringArg.remove(); int32_t limit = parser.idBlockVector.size(); @@ -625,7 +628,10 @@ Transliterator* TransliteratorRegistry::reget(const UnicodeString& ID, } if (!parser.dataVector.isEmpty()) { TransliterationRuleData* data = (TransliterationRuleData*)parser.dataVector.orphanElementAt(0); - entry->u.dataVector->addElementX(data, status); + entry->u.dataVector->addElement(data, status); + if (U_FAILURE(status)) { + delete data; + } entry->stringArg += (UChar)0xffff; // use U+FFFF to mark position of RBTs in ID block } } @@ -951,7 +957,7 @@ void TransliteratorRegistry::registerEntry(const UnicodeString& ID, if (newID != NULL) { // NUL-terminate the ID string newID->getTerminatedBuffer(); - availableIDs.addElementX(newID, status); + availableIDs.adoptElement(newID, status); } } } else { @@ -992,7 +998,7 @@ void TransliteratorRegistry::registerSTV(const UnicodeString& source, } UnicodeString *variantEntry = new UnicodeString(variant); if (variantEntry != NULL) { - variantList.addElementX(variantEntry, status); + variantList.adoptElement(variantEntry, status); if (U_SUCCESS(status)) { variantListIndex = variantList.size() - 1; } @@ -1320,7 +1326,7 @@ Transliterator* TransliteratorRegistry::instantiateEntry(const UnicodeString& ID return t; case TransliteratorEntry::COMPOUND_RBT: { - UVector* rbts = new UVector(entry->u.dataVector->size(), status); + UVector* rbts = new UVector(uprv_deleteUObject, nullptr, entry->u.dataVector->size(), status); // Check for null pointer if (rbts == NULL) { status = U_MEMORY_ALLOCATION_ERROR; @@ -1334,12 +1340,13 @@ Transliterator* TransliteratorRegistry::instantiateEntry(const UnicodeString& ID if (tl == 0) status = U_MEMORY_ALLOCATION_ERROR; else - rbts->addElementX(tl, status); + rbts->adoptElement(tl, status); } if (U_FAILURE(status)) { delete rbts; return 0; } + rbts->setDeleter(nullptr); aliasReturn = new TransliteratorAlias(ID, entry->stringArg, rbts, entry->compoundFilter); } if (aliasReturn == 0) { diff --git a/icu4c/source/i18n/tridpars.cpp b/icu4c/source/i18n/tridpars.cpp index c163d06d29a..0ca168e7a3e 100644 --- a/icu4c/source/i18n/tridpars.cpp +++ b/icu4c/source/i18n/tridpars.cpp @@ -364,6 +364,8 @@ UBool TransliteratorIDParser::parseCompoundID(const UnicodeString& id, int32_t d int32_t pos = 0; int32_t withParens = 1; list.removeAllElements(); + UObjectDeleter *save = list.setDeleter(_deleteSingleID); + UnicodeSet* filter; globalFilter = NULL; canonID.truncate(0); @@ -392,7 +394,7 @@ UBool TransliteratorIDParser::parseCompoundID(const UnicodeString& id, int32_t d break; } if (dir == FORWARD) { - list.addElementX(single, ec); + list.adoptElement(single, ec); } else { list.insertElementAt(single, 0, ec); } @@ -442,10 +444,10 @@ UBool TransliteratorIDParser::parseCompoundID(const UnicodeString& id, int32_t d goto FAIL; } + list.setDeleter(save); return TRUE; FAIL: - UObjectDeleter *save = list.setDeleter(_deleteSingleID); list.removeAllElements(); list.setDeleter(save); delete globalFilter; @@ -494,9 +496,8 @@ void TransliteratorIDParser::instantiateList(UVector& list, ec = U_INVALID_ID; goto RETURN; } - tlist.addElementX(t, ec); + tlist.adoptElement(t, ec); if (U_FAILURE(ec)) { - delete t; goto RETURN; } } @@ -509,10 +510,7 @@ void TransliteratorIDParser::instantiateList(UVector& list, // Should never happen ec = U_INTERNAL_TRANSLITERATOR_ERROR; } - tlist.addElementX(t, ec); - if (U_FAILURE(ec)) { - delete t; - } + tlist.adoptElement(t, ec); } RETURN: @@ -525,9 +523,8 @@ void TransliteratorIDParser::instantiateList(UVector& list, while (tlist.size() > 0) { t = (Transliterator*) tlist.orphanElementAt(0); - list.addElementX(t, ec); + list.adoptElement(t, ec); if (U_FAILURE(ec)) { - delete t; list.removeAllElements(); break; } diff --git a/icu4c/source/test/intltest/transapi.cpp b/icu4c/source/test/intltest/transapi.cpp index 4b59ecfde49..9a3577f3a4f 100644 --- a/icu4c/source/test/intltest/transapi.cpp +++ b/icu4c/source/test/intltest/transapi.cpp @@ -620,6 +620,9 @@ void TransliteratorAPITest::TestNullTransliterator(){ UErrorCode status=U_ZERO_ERROR; UnicodeString s("Transliterate using null transliterator"); Transliterator *nullTrans=Transliterator::createInstance("Any-Null", UTRANS_FORWARD, status); + if (!assertSuccess(WHERE, status)) { + return; + } int32_t transLimit; int32_t start=0; int32_t limit=s.length(); diff --git a/icu4c/source/test/intltest/uvectest.cpp b/icu4c/source/test/intltest/uvectest.cpp index 0a05ea9f59d..0832663d666 100644 --- a/icu4c/source/test/intltest/uvectest.cpp +++ b/icu4c/source/test/intltest/uvectest.cpp @@ -124,6 +124,21 @@ void UVectorTest::UVector_API() { TEST_ASSERT(a->contains((int32_t)15)); TEST_ASSERT(!a->contains((int32_t)5)); delete a; + + status = U_ZERO_ERROR; + UVector vec(status); + vec.setDeleter(uprv_deleteUObject); + vec.adoptElement(new UnicodeString(), status); + vec.adoptElement(new UnicodeString(), status); + assertSuccess(WHERE, status); + assertEquals(WHERE, 2, vec.size()); + + // With an incoming error, adoptElement will not add to the vector, + // and will delete the object. Failure here will show as a memory leak. + status = U_ILLEGAL_ARGUMENT_ERROR; + vec.adoptElement(new UnicodeString(), status); + assertEquals(WHERE, U_ILLEGAL_ARGUMENT_ERROR, status); + assertEquals(WHERE, 2, vec.size()); } void UVectorTest::UStack_API() {