From 1817a9e13d4688e9f0cad344c83f340bfbf87147 Mon Sep 17 00:00:00 2001 From: Mike FABIAN Date: Mon, 23 Aug 2021 18:03:31 +0200 Subject: [PATCH] ICU-21667 Fix coverity warnings See: https://unicode-org.atlassian.net/browse/ICU-21667 (Issues found by coverity in icu-69.1) --- icu4c/source/common/brkiter.cpp | 2 + icu4c/source/common/serv.cpp | 76 +++++++++++++---------- icu4c/source/common/serv.h | 5 ++ icu4c/source/common/uloc_keytype.cpp | 12 ++-- icu4c/source/common/umutablecptrie.cpp | 2 +- icu4c/source/common/uresbund.cpp | 1 + icu4c/source/i18n/decNumber.h | 2 +- icu4c/source/i18n/rbt_pars.cpp | 7 +-- icu4c/source/i18n/tridpars.cpp | 1 + icu4c/source/i18n/usearch.cpp | 2 + icu4c/source/i18n/uspoof_impl.cpp | 6 +- icu4c/source/tools/gensprep/store.c | 1 - icu4c/source/tools/makeconv/genmbcs.cpp | 4 ++ icu4c/source/tools/pkgdata/pkgtypes.c | 12 ++-- icu4c/source/tools/toolutil/filetools.cpp | 1 + 15 files changed, 81 insertions(+), 53 deletions(-) diff --git a/icu4c/source/common/brkiter.cpp b/icu4c/source/common/brkiter.cpp index 4d945cc17e2..c3be2202edc 100644 --- a/icu4c/source/common/brkiter.cpp +++ b/icu4c/source/common/brkiter.cpp @@ -107,7 +107,9 @@ BreakIterator::buildInstance(const Locale& loc, const char *type, UErrorCode &st } } + /* coverity[incorrect_free] */ ures_close(brkRules); + /* coverity[incorrect_free] */ ures_close(brkName); UDataMemory* file = udata_open(U_ICUDATA_BRKITR, ext, fnbuff, &status); diff --git a/icu4c/source/common/serv.cpp b/icu4c/source/common/serv.cpp index 3a4aee5797f..3fab52592e7 100644 --- a/icu4c/source/common/serv.cpp +++ b/icu4c/source/common/serv.cpp @@ -722,49 +722,57 @@ UVector& ICUService::getDisplayNames(UVector& result, const Locale& locale, const UnicodeString* matchID, - UErrorCode& status) const + UErrorCode& status) const { + // cast away semantic const + return const_cast(this)->getDisplayNamesImpl(result, locale, matchID, status); +} + +UVector& +ICUService::getDisplayNamesImpl(UVector& result, + const Locale& locale, + const UnicodeString* matchID, + UErrorCode& status) +{ + if (U_FAILURE(status)) { return result; } result.removeAllElements(); result.setDeleter(userv_deleteStringPair); - if (U_SUCCESS(status)) { - ICUService* ncthis = const_cast(this); // cast away semantic const - Mutex mutex(&lock); + Mutex mutex(&lock); - if (dnCache != nullptr && dnCache->locale != locale) { - delete dnCache; - ncthis->dnCache = nullptr; + if (dnCache != nullptr && dnCache->locale != locale) { + delete dnCache; + dnCache = nullptr; + } + + if (dnCache == nullptr) { + const Hashtable* m = getVisibleIDMap(status); + if (U_FAILURE(status)) { + return result; + } + dnCache = new DNCache(locale); + if (dnCache == nullptr) { + status = U_MEMORY_ALLOCATION_ERROR; + return result; } - if (dnCache == nullptr) { - const Hashtable* m = getVisibleIDMap(status); - if (U_FAILURE(status)) { - return result; - } - ncthis->dnCache = new DNCache(locale); - if (dnCache == nullptr) { + int32_t pos = UHASH_FIRST; + const UHashElement* entry = nullptr; + while ((entry = m->nextElement(pos)) != nullptr) { + const UnicodeString* id = static_cast(entry->key.pointer); + ICUServiceFactory* f = static_cast(entry->value.pointer); + UnicodeString dname; + f->getDisplayName(*id, locale, dname); + if (dname.isBogus()) { status = U_MEMORY_ALLOCATION_ERROR; - return result; - } - - int32_t pos = UHASH_FIRST; - const UHashElement* entry = nullptr; - while ((entry = m->nextElement(pos)) != nullptr) { - const UnicodeString* id = static_cast(entry->key.pointer); - ICUServiceFactory* f = static_cast(entry->value.pointer); - UnicodeString dname; - f->getDisplayName(*id, locale, dname); - if (dname.isBogus()) { - status = U_MEMORY_ALLOCATION_ERROR; - } else { - dnCache->cache.put(dname, (void*)id, status); // share pointer with visibleIDMap - if (U_SUCCESS(status)) { - continue; - } + } else { + dnCache->cache.put(dname, (void*)id, status); // share pointer with visibleIDMap + if (U_SUCCESS(status)) { + continue; } - delete dnCache; - ncthis->dnCache = nullptr; - return result; } + delete dnCache; + dnCache = nullptr; + return result; } } diff --git a/icu4c/source/common/serv.h b/icu4c/source/common/serv.h index 9aea548fc3a..3c53f228738 100644 --- a/icu4c/source/common/serv.h +++ b/icu4c/source/common/serv.h @@ -759,6 +759,11 @@ class U_COMMON_API ICUService : public ICUNotifier { const UnicodeString* matchID, UErrorCode& status) const; + UVector& getDisplayNamesImpl(UVector& result, + const Locale& locale, + const UnicodeString* matchID, + UErrorCode& status); + /** *

A convenience override of registerInstance(UObject*, const UnicodeString&, UBool) * that defaults visible to true.

diff --git a/icu4c/source/common/uloc_keytype.cpp b/icu4c/source/common/uloc_keytype.cpp index 9dc392126ef..d293273a68c 100644 --- a/icu4c/source/common/uloc_keytype.cpp +++ b/icu4c/source/common/uloc_keytype.cpp @@ -160,7 +160,7 @@ initFromResourceBundle(UErrorCode& sts) { bool isTZ = uprv_strcmp(legacyKeyId, "timezone") == 0; - UHashtable* typeDataMap = uhash_open(uhash_hashIStringView, uhash_compareIStringView, nullptr, &sts); + LocalUHashtablePointer typeDataMap(uhash_open(uhash_hashIStringView, uhash_compareIStringView, nullptr, &sts)); if (U_FAILURE(sts)) { break; } @@ -269,10 +269,10 @@ initFromResourceBundle(UErrorCode& sts) { t->bcpId = bcpTypeId; t->legacyId = legacyTypeId; - uhash_put(typeDataMap, &t->legacyId, t, &sts); + uhash_put(typeDataMap.getAlias(), &t->legacyId, t, &sts); if (bcpTypeId != legacyTypeId) { // different type value - uhash_put(typeDataMap, &t->bcpId, t, &sts); + uhash_put(typeDataMap.getAlias(), &t->bcpId, t, &sts); } if (U_FAILURE(sts)) { break; @@ -318,7 +318,7 @@ initFromResourceBundle(UErrorCode& sts) { alias->from = fromBuf->toStringPiece(); } } - uhash_put(typeDataMap, &alias->from, t, &sts); + uhash_put(typeDataMap.getAlias(), &alias->from, t, &sts); } } if (U_FAILURE(sts)) { @@ -346,7 +346,7 @@ initFromResourceBundle(UErrorCode& sts) { toLen) == 0) { const char* from = ures_getKey(bcpTypeAliasDataEntry.getAlias()); TypeAlias* alias = gTypeAliasEntries->create(TypeAlias{{}, from}); - uhash_put(typeDataMap, &alias->from, t, &sts); + uhash_put(typeDataMap.getAlias(), &alias->from, t, &sts); } } if (U_FAILURE(sts)) { @@ -367,7 +367,7 @@ initFromResourceBundle(UErrorCode& sts) { keyData->bcpId = bcpKeyId; keyData->legacyId = legacyKeyId; keyData->specialTypes = specialTypes; - keyData->typeMap.adoptInstead(typeDataMap); + keyData->typeMap = std::move(typeDataMap); uhash_put(gLocExtKeyMap, &keyData->legacyId, keyData, &sts); if (legacyKeyId != bcpKeyId) { diff --git a/icu4c/source/common/umutablecptrie.cpp b/icu4c/source/common/umutablecptrie.cpp index 40ea57f1491..49a4dd3eb56 100644 --- a/icu4c/source/common/umutablecptrie.cpp +++ b/icu4c/source/common/umutablecptrie.cpp @@ -1543,7 +1543,7 @@ int32_t MutableCodePointTrie::compactTrie(int32_t fastILimit, UErrorCode &errorC MixedBlocks mixedBlocks; int32_t newDataLength = compactData(fastILimit, newData, newDataCapacity, dataNullIndex, mixedBlocks, errorCode); - if (U_FAILURE(errorCode)) { return 0; } + if (U_FAILURE(errorCode)) { uprv_free(newData); return 0; } U_ASSERT(newDataLength <= newDataCapacity); uprv_free(data); data = newData; diff --git a/icu4c/source/common/uresbund.cpp b/icu4c/source/common/uresbund.cpp index cfe2f4a4d20..c938edd6fb1 100644 --- a/icu4c/source/common/uresbund.cpp +++ b/icu4c/source/common/uresbund.cpp @@ -2924,6 +2924,7 @@ typedef struct ULocalesContext { static void U_CALLCONV ures_loc_closeLocales(UEnumeration *enumerator) { + if (enumerator == nullptr) { return; } ULocalesContext* ctx = static_cast(enumerator->context); ures_close(&ctx->curr); ures_close(&ctx->installed); diff --git a/icu4c/source/i18n/decNumber.h b/icu4c/source/i18n/decNumber.h index 4a1eb364e19..6ad94386c1f 100644 --- a/icu4c/source/i18n/decNumber.h +++ b/icu4c/source/i18n/decNumber.h @@ -86,7 +86,7 @@ /* range: -1999999997 through 999999999 */ uint8_t bits; /* Indicator bits (see above) */ /* Coefficient, from least significant unit */ - decNumberUnit lsu[DECNUMUNITS]; + decNumberUnit lsu[DECNUMUNITS+2]; } decNumber; /* Notes: */ diff --git a/icu4c/source/i18n/rbt_pars.cpp b/icu4c/source/i18n/rbt_pars.cpp index 6d0d2272432..b2ced2f1926 100644 --- a/icu4c/source/i18n/rbt_pars.cpp +++ b/icu4c/source/i18n/rbt_pars.cpp @@ -552,16 +552,15 @@ int32_t RuleHalf::parseSection(const UnicodeString& rule, int32_t pos, int32_t l case ALT_FUNCTION: { int32_t iref = pos; - TransliteratorIDParser::SingleID* single = - TransliteratorIDParser::parseFilterID(rule, iref); + LocalPointer single( + TransliteratorIDParser::parseFilterID(rule, iref)); // The next character MUST be a segment open - if (single == nullptr || + if (single.isNull() || !ICU_Utility::parseChar(rule, iref, SEGMENT_OPEN)) { return syntaxError(U_INVALID_FUNCTION, rule, start, status); } Transliterator *t = single->createInstance(); - delete single; if (t == nullptr) { return syntaxError(U_INVALID_FUNCTION, rule, start, status); } diff --git a/icu4c/source/i18n/tridpars.cpp b/icu4c/source/i18n/tridpars.cpp index 5c769dc39fb..c2ab4b04ce3 100644 --- a/icu4c/source/i18n/tridpars.cpp +++ b/icu4c/source/i18n/tridpars.cpp @@ -136,6 +136,7 @@ TransliteratorIDParser::parseSingleID(const UnicodeString& id, int32_t& pos, specsB = parseFilterID(id, pos, true); // Must close with a ')' if (specsB == nullptr || !ICU_Utility::parseChar(id, pos, CLOSE_REV)) { + delete specsB; delete specsA; pos = start; return nullptr; diff --git a/icu4c/source/i18n/usearch.cpp b/icu4c/source/i18n/usearch.cpp index d296c287402..b3273e6fbab 100644 --- a/icu4c/source/i18n/usearch.cpp +++ b/icu4c/source/i18n/usearch.cpp @@ -199,6 +199,7 @@ inline int32_t * addTouint32_tArray(int32_t *destination, int32_t* temp = static_cast(allocateMemory( sizeof(int32_t) * newlength, status)); if (U_FAILURE(*status)) { + uprv_free(temp); return nullptr; } uprv_memcpy(temp, destination, sizeof(int32_t) * (size_t)offset); @@ -240,6 +241,7 @@ inline int64_t * addTouint64_tArray(int64_t *destination, sizeof(int64_t) * newlength, status)); if (U_FAILURE(*status)) { + uprv_free(temp); return nullptr; } diff --git a/icu4c/source/i18n/uspoof_impl.cpp b/icu4c/source/i18n/uspoof_impl.cpp index c727fafb37b..dad3e36b7ce 100644 --- a/icu4c/source/i18n/uspoof_impl.cpp +++ b/icu4c/source/i18n/uspoof_impl.cpp @@ -194,8 +194,12 @@ void SpoofImpl::setAllowedLocales(const char *localesList, UErrorCode &status) { // Store the updated spoof checker state. tmpSet = allowedChars.clone(); + if (tmpSet == nullptr) { + status = U_MEMORY_ALLOCATION_ERROR; + return; + } const char *tmpLocalesList = uprv_strdup(localesList); - if (tmpSet == nullptr || tmpLocalesList == nullptr) { + if (tmpLocalesList == nullptr) { status = U_MEMORY_ALLOCATION_ERROR; return; } diff --git a/icu4c/source/tools/gensprep/store.c b/icu4c/source/tools/gensprep/store.c index f0f4d7844be..8a868654be8 100644 --- a/icu4c/source/tools/gensprep/store.c +++ b/icu4c/source/tools/gensprep/store.c @@ -638,7 +638,6 @@ extern void cleanUpData(void) { uprv_free(mappingData); utrie_close(sprepTrie); - uprv_free(sprepTrie); } #endif /* #if !UCONFIG_NO_IDNA */ diff --git a/icu4c/source/tools/makeconv/genmbcs.cpp b/icu4c/source/tools/makeconv/genmbcs.cpp index 774cdc6045d..a996ac5aa8d 100644 --- a/icu4c/source/tools/makeconv/genmbcs.cpp +++ b/icu4c/source/tools/makeconv/genmbcs.cpp @@ -173,6 +173,10 @@ MBCSOpen(UCMFile *ucm) { } MBCSInit(mbcsData, ucm); + /* The memory in the MBSData structure following + * newConverter will be properly freed in MBCSClose. + */ + /* coverity[leaked_storage] */ return &mbcsData->newConverter; } diff --git a/icu4c/source/tools/pkgdata/pkgtypes.c b/icu4c/source/tools/pkgdata/pkgtypes.c index 26bd945df73..ba516861a01 100644 --- a/icu4c/source/tools/pkgdata/pkgtypes.c +++ b/icu4c/source/tools/pkgdata/pkgtypes.c @@ -31,6 +31,7 @@ const char *pkg_writeCharListWrap(FileStream *s, CharList *l, const char *delim, { int32_t ln = 0; char buffer[1024]; + char *bufferp = buffer; while(l != NULL) { if(l->str) @@ -43,7 +44,7 @@ const char *pkg_writeCharListWrap(FileStream *s, CharList *l, const char *delim, buffer[uprv_strlen(buffer)-1] = '\0'; } if(buffer[0] == '"') { - uprv_strcpy(buffer, buffer+1); + bufferp = buffer+1; } } else if(quote > 0) { /* add quotes */ if(l->str[0] != '"') { @@ -54,7 +55,7 @@ const char *pkg_writeCharListWrap(FileStream *s, CharList *l, const char *delim, uprv_strcat(buffer, "\""); } } - T_FileStream_write(s, buffer, (int32_t)uprv_strlen(buffer)); + T_FileStream_write(s, bufferp, (int32_t)uprv_strlen(bufferp)); ln += (int32_t)uprv_strlen(l->str); } @@ -75,7 +76,8 @@ const char *pkg_writeCharListWrap(FileStream *s, CharList *l, const char *delim, const char *pkg_writeCharList(FileStream *s, CharList *l, const char *delim, int32_t quote) { - char buffer[1024]; + char buffer[1026]; /* 1026 instead of 1024 because quotes may be added */ + char *bufferp = buffer; while(l != NULL) { if(l->str) @@ -93,7 +95,7 @@ const char *pkg_writeCharList(FileStream *s, CharList *l, const char *delim, int buffer[uprv_strlen(buffer)-1] = '\0'; } if(buffer[0] == '"') { - uprv_strcpy(buffer, buffer+1); + bufferp = buffer+1; } } else if(quote > 0) { /* add quotes */ if(l->str[0] != '"') { @@ -104,7 +106,7 @@ const char *pkg_writeCharList(FileStream *s, CharList *l, const char *delim, int uprv_strcat(buffer, "\""); } } - T_FileStream_write(s, buffer, (int32_t)uprv_strlen(buffer)); + T_FileStream_write(s, bufferp, (int32_t)uprv_strlen(bufferp)); } if(l->next && delim) diff --git a/icu4c/source/tools/toolutil/filetools.cpp b/icu4c/source/tools/toolutil/filetools.cpp index 994d8e31f00..8dcbb6a480a 100644 --- a/icu4c/source/tools/toolutil/filetools.cpp +++ b/icu4c/source/tools/toolutil/filetools.cpp @@ -64,6 +64,7 @@ isFileModTimeLater(const char *filePath, const char *checkAgainst, UBool isDir) newpath.append(dirEntry->d_name, -1, status); if (U_FAILURE(status)) { fprintf(stderr, "%s:%d: %s\n", __FILE__, __LINE__, u_errorName(status)); + closedir(pDir); return false; }