From 9663195189adc6ac7a70f156caba2e500ce6359f Mon Sep 17 00:00:00 2001 From: Frank Tang Date: Thu, 12 Nov 2020 08:33:40 +0000 Subject: [PATCH] ICU-21385 Fix assertion when setKeywordValue w/ long value. See #1461 --- icu4c/source/common/locid.cpp | 5 +++++ icu4c/source/common/uloc.cpp | 21 ++++++++++++++++++++- icu4c/source/test/cintltst/cloctst.c | 7 +------ icu4c/source/test/intltest/loctest.cpp | 20 +++++++++++++++++++- 4 files changed, 45 insertions(+), 8 deletions(-) diff --git a/icu4c/source/common/locid.cpp b/icu4c/source/common/locid.cpp index 1efe0da93c3..feadbcbcccc 100644 --- a/icu4c/source/common/locid.cpp +++ b/icu4c/source/common/locid.cpp @@ -2457,9 +2457,13 @@ Locale::setKeywordValue(const char* keywordName, const char* keywordValue, UErro if (U_FAILURE(status)) { return; } + if (status == U_STRING_NOT_TERMINATED_WARNING) { + status = U_ZERO_ERROR; + } int32_t bufferLength = uprv_max((int32_t)(uprv_strlen(fullName) + 1), ULOC_FULLNAME_CAPACITY); int32_t newLength = uloc_setKeywordValue(keywordName, keywordValue, fullName, bufferLength, &status) + 1; + U_ASSERT(status != U_STRING_NOT_TERMINATED_WARNING); /* Handle the case the current buffer is not enough to hold the new id */ if (status == U_BUFFER_OVERFLOW_ERROR) { U_ASSERT(newLength > bufferLength); @@ -2476,6 +2480,7 @@ Locale::setKeywordValue(const char* keywordName, const char* keywordValue, UErro fullName = newFullName; status = U_ZERO_ERROR; uloc_setKeywordValue(keywordName, keywordValue, fullName, newLength, &status); + U_ASSERT(status != U_STRING_NOT_TERMINATED_WARNING); } else { U_ASSERT(newLength <= bufferLength); } diff --git a/icu4c/source/common/uloc.cpp b/icu4c/source/common/uloc.cpp index 522f33dbe24..ebfbb506508 100644 --- a/icu4c/source/common/uloc.cpp +++ b/icu4c/source/common/uloc.cpp @@ -877,6 +877,9 @@ uloc_setKeywordValue(const char* keywordName, if(U_FAILURE(*status)) { return -1; } + if (*status == U_STRING_NOT_TERMINATED_WARNING) { + *status = U_ZERO_ERROR; + } if (keywordName == NULL || keywordName[0] == 0 || bufferCapacity <= 1) { *status = U_ILLEGAL_ARGUMENT_ERROR; return 0; @@ -914,6 +917,7 @@ uloc_setKeywordValue(const char* keywordName, startSearchHere = (char*)locale_getKeywordsStart(buffer); if(startSearchHere == NULL || (startSearchHere[1]==0)) { if(keywordValueLen == 0) { /* no keywords = nothing to remove */ + U_ASSERT(*status != U_STRING_NOT_TERMINATED_WARNING); return bufLen; } @@ -933,6 +937,7 @@ uloc_setKeywordValue(const char* keywordName, startSearchHere += keywordNameLen; *startSearchHere++ = '='; uprv_strcpy(startSearchHere, keywordValueBuffer); + U_ASSERT(*status != U_STRING_NOT_TERMINATED_WARNING); return needLen; } /* end shortcut - no @ */ @@ -1047,13 +1052,27 @@ uloc_setKeywordValue(const char* keywordName, if (!handledInputKeyAndValue || U_FAILURE(*status)) { /* if input key/value specified removal of a keyword not present in locale, or * there was an error in CharString.append, leave original locale alone. */ + U_ASSERT(*status != U_STRING_NOT_TERMINATED_WARNING); return bufLen; } // needLen = length of the part before '@' needLen = (int32_t)(startSearchHere - buffer); - return needLen + updatedKeysAndValues.extract( + // Check to see can we fit the startSearchHere, if not, return + // U_BUFFER_OVERFLOW_ERROR without copy updatedKeysAndValues into it. + // We do this because this API function does not behave like most others: + // It promises never to set a U_STRING_NOT_TERMINATED_WARNING. + // When the contents fits but without the terminating NUL, in this case we need to not change + // the buffer contents and return with a buffer overflow error. + int32_t appendLength = updatedKeysAndValues.length(); + if (appendLength >= bufferCapacity - needLen) { + *status = U_BUFFER_OVERFLOW_ERROR; + return needLen + appendLength; + } + needLen += updatedKeysAndValues.extract( startSearchHere, bufferCapacity - needLen, *status); + U_ASSERT(*status != U_STRING_NOT_TERMINATED_WARNING); + return needLen; } /* ### ID parsing implementation **************************************************/ diff --git a/icu4c/source/test/cintltst/cloctst.c b/icu4c/source/test/cintltst/cloctst.c index 52a59879b65..61cfd4cb7cf 100644 --- a/icu4c/source/test/cintltst/cloctst.c +++ b/icu4c/source/test/cintltst/cloctst.c @@ -2245,12 +2245,7 @@ static void TestKeywordSetError(void) strcpy(buffer,kwSetTestCases[i].l); status = U_ZERO_ERROR; res = uloc_setKeywordValue(kwSetTestCases[i].k, kwSetTestCases[i].v, buffer, blen, &status); - if(res == blen) { - if(status != U_STRING_NOT_TERMINATED_WARNING) { - log_err("expected not terminated warning on buffer %d got %s, len %d (%s + [%s=%s])\n", blen, u_errorName(status), res, kwSetTestCases[i].l, kwSetTestCases[i].k, kwSetTestCases[i].v); - return; - } - } else if(status != U_BUFFER_OVERFLOW_ERROR) { + if(status != U_BUFFER_OVERFLOW_ERROR) { log_err("expected buffer overflow on buffer %d got %s, len %d (%s + [%s=%s])\n", blen, u_errorName(status), res, kwSetTestCases[i].l, kwSetTestCases[i].k, kwSetTestCases[i].v); return; } diff --git a/icu4c/source/test/intltest/loctest.cpp b/icu4c/source/test/intltest/loctest.cpp index a847a957d98..7ce21c45b3a 100644 --- a/icu4c/source/test/intltest/loctest.cpp +++ b/icu4c/source/test/intltest/loctest.cpp @@ -4154,7 +4154,7 @@ LocaleTest::TestSetKeywordValue(void) { { "calendar", "buddhist" } }; - UErrorCode status = U_ZERO_ERROR; + IcuTestErrorCode status(*this, "TestSetKeywordValue()"); int32_t i = 0; int32_t resultLen = 0; @@ -4176,6 +4176,24 @@ LocaleTest::TestSetKeywordValue(void) { testCases[i].value, testCases[i].keyword, buffer); } } + + // Test long locale + { + status.errIfFailureAndReset(); + const char* input = + "de__POSIX@colnormalization=no;colstrength=primary;currency=eur;" + "em=default;kv=space;lb=strict;lw=normal;measure=metric;" + "numbers=latn;rg=atzzzz;sd=atat1"; + const char* expected = + "de__POSIX@colnormalization=no;colstrength=primary;currency=eur;" + "em=default;kv=space;lb=strict;lw=normal;measure=metric;" + "numbers=latn;rg=atzzzz;sd=atat1;ss=none"; + // Bug ICU-21385 + Locale l2(input); + l2.setKeywordValue("ss", "none", status); + assertEquals("", expected, l2.getName()); + status.errIfFailureAndReset(); + } } void