From: Shane Date: Thu, 20 Sep 2018 21:44:48 +0000 (-0700) Subject: ICU-13850 Make CurrencyUnit safe with 1-length and 2-length strings. (#133) X-Git-Tag: release-63-rc~33 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=71ad5adf4a2ab77e5a21979c3e321282d6bcada5;p=icu ICU-13850 Make CurrencyUnit safe with 1-length and 2-length strings. (#133) Also adds expectError to IcuTestErrorCode. --- diff --git a/icu4c/source/i18n/currunit.cpp b/icu4c/source/i18n/currunit.cpp index bd6059366cd..7f3490d406b 100644 --- a/icu4c/source/i18n/currunit.cpp +++ b/icu4c/source/i18n/currunit.cpp @@ -28,9 +28,13 @@ CurrencyUnit::CurrencyUnit(ConstChar16Ptr _isoCode, UErrorCode& ec) { // Note: in ICU4J Currency.getInstance(), we check string length for 3, but in ICU4C we allow a // non-NUL-terminated string to be passed as an argument, so it is not possible to check length. // However, we allow a NUL-terminated empty string, which should have the same behavior as nullptr. + // Consider NUL-terminated strings of length 1 or 2 as invalid. const char16_t* isoCodeToUse; if (U_FAILURE(ec) || _isoCode == nullptr || _isoCode[0] == 0) { isoCodeToUse = kDefaultCurrency; + } else if (_isoCode[1] == 0 || _isoCode[2] == 0) { + isoCodeToUse = kDefaultCurrency; + ec = U_ILLEGAL_ARGUMENT_ERROR; } else if (!uprv_isInvariantUString(_isoCode, 3)) { // TODO: Perform a more strict ASCII check like in ICU4J isAlpha3Code? isoCodeToUse = kDefaultCurrency; diff --git a/icu4c/source/test/intltest/itutil.cpp b/icu4c/source/test/intltest/itutil.cpp index 7b2995f7ccd..4d3466120cf 100644 --- a/icu4c/source/test/intltest/itutil.cpp +++ b/icu4c/source/test/intltest/itutil.cpp @@ -300,7 +300,7 @@ void ErrorCodeTest::TestIcuTestErrorCode() { helper.test = this; // Test destructor message - helper.expectedErrln = u"AAA failure: U_ILLEGAL_PAD_POSITION"; + helper.expectedErrln = u"AAA destructor: expected success but got error: U_ILLEGAL_PAD_POSITION"; helper.expectedDataErr = FALSE; helper.seenError = FALSE; { @@ -310,7 +310,7 @@ void ErrorCodeTest::TestIcuTestErrorCode() { assertTrue("Should have seen an error", helper.seenError); // Test destructor message with scope - helper.expectedErrln = u"BBB failure: U_ILLEGAL_PAD_POSITION scope: foo"; + helper.expectedErrln = u"BBB destructor: expected success but got error: U_ILLEGAL_PAD_POSITION scope: foo"; helper.expectedDataErr = FALSE; helper.seenError = FALSE; { @@ -321,7 +321,7 @@ void ErrorCodeTest::TestIcuTestErrorCode() { assertTrue("Should have seen an error", helper.seenError); // Check errIfFailure message with scope - helper.expectedErrln = u"CCC failure: U_ILLEGAL_PAD_POSITION scope: foo"; + helper.expectedErrln = u"CCC expected success but got error: U_ILLEGAL_PAD_POSITION scope: foo"; helper.expectedDataErr = FALSE; helper.seenError = FALSE; { @@ -331,14 +331,14 @@ void ErrorCodeTest::TestIcuTestErrorCode() { testStatus.errIfFailureAndReset(); assertTrue("Should have seen an error", helper.seenError); helper.seenError = FALSE; - helper.expectedErrln = u"CCC failure: U_ILLEGAL_CHAR_FOUND scope: foo - 5.4300"; + helper.expectedErrln = u"CCC expected success but got error: U_ILLEGAL_CHAR_FOUND scope: foo - 5.4300"; testStatus.set(U_ILLEGAL_CHAR_FOUND); testStatus.errIfFailureAndReset("%6.4f", 5.43); assertTrue("Should have seen an error", helper.seenError); } // Check errDataIfFailure message without scope - helper.expectedErrln = u"DDD failure: U_ILLEGAL_PAD_POSITION"; + helper.expectedErrln = u"DDD data: expected success but got error: U_ILLEGAL_PAD_POSITION"; helper.expectedDataErr = TRUE; helper.seenError = FALSE; { @@ -347,11 +347,33 @@ void ErrorCodeTest::TestIcuTestErrorCode() { testStatus.errDataIfFailureAndReset(); assertTrue("Should have seen an error", helper.seenError); helper.seenError = FALSE; - helper.expectedErrln = u"DDD failure: U_ILLEGAL_CHAR_FOUND - 5.4300"; + helper.expectedErrln = u"DDD data: expected success but got error: U_ILLEGAL_CHAR_FOUND - 5.4300"; testStatus.set(U_ILLEGAL_CHAR_FOUND); testStatus.errDataIfFailureAndReset("%6.4f", 5.43); assertTrue("Should have seen an error", helper.seenError); } + + // Check expectFailure + helper.expectedErrln = u"EEE expected: U_ILLEGAL_CHAR_FOUND but got error: U_ILLEGAL_PAD_POSITION"; + helper.expectedDataErr = FALSE; + helper.seenError = FALSE; + { + IcuTestErrorCode testStatus(helper, "EEE"); + testStatus.set(U_ILLEGAL_PAD_POSITION); + testStatus.expectErrorAndReset(U_ILLEGAL_PAD_POSITION); + assertFalse("Should NOT have seen an error", helper.seenError); + testStatus.set(U_ILLEGAL_PAD_POSITION); + testStatus.expectErrorAndReset(U_ILLEGAL_CHAR_FOUND); + assertTrue("Should have seen an error", helper.seenError); + helper.seenError = FALSE; + helper.expectedErrln = u"EEE expected: U_ILLEGAL_CHAR_FOUND but got error: U_ZERO_ERROR scope: scopety scope - 5.4300"; + testStatus.setScope("scopety scope"); + testStatus.set(U_ILLEGAL_PAD_POSITION); + testStatus.expectErrorAndReset(U_ILLEGAL_PAD_POSITION, "%6.4f", 5.43); + assertFalse("Should NOT have seen an error", helper.seenError); + testStatus.expectErrorAndReset(U_ILLEGAL_CHAR_FOUND, "%6.4f", 5.43); + assertTrue("Should have seen an error", helper.seenError); + } } diff --git a/icu4c/source/test/intltest/measfmttest.cpp b/icu4c/source/test/intltest/measfmttest.cpp index 8d7ec8165b0..4928962d395 100644 --- a/icu4c/source/test/intltest/measfmttest.cpp +++ b/icu4c/source/test/intltest/measfmttest.cpp @@ -1857,9 +1857,14 @@ void MeasureFormatTest::TestGram() { } void MeasureFormatTest::TestCurrencies() { - UChar USD[4]; + UChar USD[4] = {}; u_uastrcpy(USD, "USD"); UErrorCode status = U_ZERO_ERROR; + CurrencyUnit USD_unit(USD, status); + assertEquals("Currency Unit", USD, USD_unit.getISOCurrency()); + if (!assertSuccess("Error creating CurrencyUnit", status)) { + return; + } CurrencyAmount USD_1(1.0, USD, status); assertEquals("Currency Code", USD, USD_1.getISOCurrency()); CurrencyAmount USD_2(2.0, USD, status); diff --git a/icu4c/source/test/intltest/numfmtst.cpp b/icu4c/source/test/intltest/numfmtst.cpp index 77d4283441b..ecd6f95380d 100644 --- a/icu4c/source/test/intltest/numfmtst.cpp +++ b/icu4c/source/test/intltest/numfmtst.cpp @@ -9241,22 +9241,40 @@ void NumberFormatTest::Test13840_ParseLongStringCrash() { void NumberFormatTest::Test13850_EmptyStringCurrency() { IcuTestErrorCode status(*this, "Test13840_EmptyStringCurrency"); - LocalPointer nf(NumberFormat::createCurrencyInstance("en-US", status), status); - if (status.errIfFailureAndReset()) { return; } - UnicodeString actual; - nf->format(1, actual, status); - assertEquals("Should format with US currency", u"$1.00", actual); - nf->setCurrency(u"", status); - nf->format(1, actual.remove(), status); - assertEquals("Should unset the currency on empty string", u"XXX\u00A01.00", actual); - - // Try with nullptr - nf.adoptInstead(NumberFormat::createCurrencyInstance("en-US", status)); - nf->format(1, actual.remove(), status); - assertEquals("Should format with US currency", u"$1.00", actual); - nf->setCurrency(nullptr, status); - nf->format(1, actual.remove(), status); - assertEquals("Should unset the currency on nullptr", u"XXX\u00A01.00", actual); + struct TestCase { + const char16_t* currencyArg; + UErrorCode expectedError; + } cases[] = { + {u"", U_ZERO_ERROR}, + {u"U", U_ILLEGAL_ARGUMENT_ERROR}, + {u"Us", U_ILLEGAL_ARGUMENT_ERROR}, + {nullptr, U_ZERO_ERROR}, + {u"U$D", U_INVARIANT_CONVERSION_ERROR}, + {u"Xxx", U_ZERO_ERROR} + }; + for (const auto& cas : cases) { + UnicodeString message(u"with currency arg: "); + if (cas.currencyArg == nullptr) { + message += u"nullptr"; + } else { + message += UnicodeString(cas.currencyArg); + } + status.setScope(message); + LocalPointer nf(NumberFormat::createCurrencyInstance("en-US", status), status); + if (status.errIfFailureAndReset()) { return; } + UnicodeString actual; + nf->format(1, actual, status); + status.errIfFailureAndReset(); + assertEquals(u"Should format with US currency " + message, u"$1.00", actual); + nf->setCurrency(cas.currencyArg, status); + if (status.expectErrorAndReset(cas.expectedError)) { + // If an error occurred, do not check formatting. + continue; + } + nf->format(1, actual.remove(), status); + assertEquals(u"Should unset the currency " + message, u"XXX\u00A01.00", actual); + status.errIfFailureAndReset(); + } } #endif /* #if !UCONFIG_NO_FORMATTING */ diff --git a/icu4c/source/tools/ctestfw/tstdtmod.cpp b/icu4c/source/tools/ctestfw/tstdtmod.cpp index 797803c5cd9..f4580b165da 100644 --- a/icu4c/source/tools/ctestfw/tstdtmod.cpp +++ b/icu4c/source/tools/ctestfw/tstdtmod.cpp @@ -21,13 +21,13 @@ TestLog::~TestLog() {} IcuTestErrorCode::~IcuTestErrorCode() { // Safe because our errlog() does not throw exceptions. if(isFailure()) { - errlog(FALSE, nullptr); + errlog(FALSE, u"destructor: expected success", nullptr); } } UBool IcuTestErrorCode::errIfFailureAndReset() { if(isFailure()) { - errlog(FALSE, nullptr); + errlog(FALSE, u"expected success", nullptr); reset(); return TRUE; } else { @@ -43,7 +43,7 @@ UBool IcuTestErrorCode::errIfFailureAndReset(const char *fmt, ...) { va_start(ap, fmt); vsprintf(buffer, fmt, ap); va_end(ap); - errlog(FALSE, buffer); + errlog(FALSE, u"expected success", buffer); reset(); return TRUE; } else { @@ -54,7 +54,7 @@ UBool IcuTestErrorCode::errIfFailureAndReset(const char *fmt, ...) { UBool IcuTestErrorCode::errDataIfFailureAndReset() { if(isFailure()) { - errlog(TRUE, nullptr); + errlog(TRUE, u"data: expected success", nullptr); reset(); return TRUE; } else { @@ -70,7 +70,7 @@ UBool IcuTestErrorCode::errDataIfFailureAndReset(const char *fmt, ...) { va_start(ap, fmt); vsprintf(buffer, fmt, ap); va_end(ap); - errlog(TRUE, buffer); + errlog(TRUE, u"data: expected success", buffer); reset(); return TRUE; } else { @@ -79,6 +79,29 @@ UBool IcuTestErrorCode::errDataIfFailureAndReset(const char *fmt, ...) { } } +UBool IcuTestErrorCode::expectErrorAndReset(UErrorCode expectedError) { + if(get() != expectedError) { + errlog(FALSE, UnicodeString(u"expected: ") + u_errorName(expectedError), nullptr); + } + UBool retval = isFailure(); + reset(); + return retval; +} + +UBool IcuTestErrorCode::expectErrorAndReset(UErrorCode expectedError, const char *fmt, ...) { + if(get() != expectedError) { + char buffer[4000]; + va_list ap; + va_start(ap, fmt); + vsprintf(buffer, fmt, ap); + va_end(ap); + errlog(FALSE, UnicodeString(u"expected: ") + u_errorName(expectedError), buffer); + } + UBool retval = isFailure(); + reset(); + return retval; +} + void IcuTestErrorCode::setScope(const char* message) { scopeMessage.remove().append({ message, -1, US_INV }); } @@ -88,12 +111,13 @@ void IcuTestErrorCode::setScope(const UnicodeString& message) { } void IcuTestErrorCode::handleFailure() const { - errlog(FALSE, nullptr); + errlog(FALSE, u"(handleFailure)", nullptr); } -void IcuTestErrorCode::errlog(UBool dataErr, const char* extraMessage) const { +void IcuTestErrorCode::errlog(UBool dataErr, const UnicodeString& mainMessage, const char* extraMessage) const { UnicodeString msg(testName, -1, US_INV); - msg.append(u" failure: ").append(UnicodeString(errorName(), -1, US_INV)); + msg.append(u' ').append(mainMessage); + msg.append(u" but got error: ").append(UnicodeString(errorName(), -1, US_INV)); if (!scopeMessage.isEmpty()) { msg.append(u" scope: ").append(scopeMessage); diff --git a/icu4c/source/tools/ctestfw/unicode/testlog.h b/icu4c/source/tools/ctestfw/unicode/testlog.h index 11b1c6e3b11..9db35b9ca85 100644 --- a/icu4c/source/tools/ctestfw/unicode/testlog.h +++ b/icu4c/source/tools/ctestfw/unicode/testlog.h @@ -41,6 +41,8 @@ public: UBool errIfFailureAndReset(const char *fmt, ...); UBool errDataIfFailureAndReset(); UBool errDataIfFailureAndReset(const char *fmt, ...); + UBool expectErrorAndReset(UErrorCode expectedError); + UBool expectErrorAndReset(UErrorCode expectedError, const char *fmt, ...); /** Sets an additional message string to be appended to failure output. */ void setScope(const char* message); @@ -54,7 +56,7 @@ private: const char *const testName; UnicodeString scopeMessage; - void errlog(UBool dataErr, const char* extraMessage) const; + void errlog(UBool dataErr, const UnicodeString& mainMessage, const char* extraMessage) const; }; #endif