From f11ca0d3630caa149a2c71391536f16574c5d054 Mon Sep 17 00:00:00 2001
From: Shane Carr <shane@unicode.org>
Date: Thu, 19 Apr 2018 01:13:17 +0000
Subject: [PATCH] ICU-13634 Fixing address sanitizer issue involving
backwards-compatible UChar* behavior in CurrencyUnit constructor. The string
passed to the constructor need not be NUL-terminated.
X-SVN-Rev: 41248
---
icu4c/source/i18n/currunit.cpp | 17 ++++++++++++-----
icu4c/source/i18n/decimfmt.cpp | 1 -
icu4c/source/i18n/number_skeletons.cpp | 8 ++++++--
icu4c/source/i18n/unicode/currunit.h | 5 +++--
.../test/intltest/numbertest_skeletons.cpp | 2 ++
.../icu/dev/test/number/NumberSkeletonTest.java | 2 ++
6 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/icu4c/source/i18n/currunit.cpp b/icu4c/source/i18n/currunit.cpp
index 003ce612109..5a6ff6e7e19 100644
--- a/icu4c/source/i18n/currunit.cpp
+++ b/icu4c/source/i18n/currunit.cpp
@@ -17,6 +17,7 @@
#include "unicode/currunit.h"
#include "unicode/ustring.h"
#include "cstring.h"
+#include "uinvchar.h"
static constexpr char16_t kDefaultCurrency[] = u"XXX";
@@ -24,14 +25,20 @@ U_NAMESPACE_BEGIN
CurrencyUnit::CurrencyUnit(ConstChar16Ptr _isoCode, UErrorCode& ec) {
// The constructor always leaves the CurrencyUnit in a valid state (with a 3-character currency code).
+ // 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.
+ const char16_t* isoCodeToUse;
if (U_FAILURE(ec) || _isoCode == nullptr) {
- u_strcpy(isoCode, kDefaultCurrency);
- } else if (u_strlen(_isoCode) != 3) {
- u_strcpy(isoCode, kDefaultCurrency);
- ec = U_ILLEGAL_ARGUMENT_ERROR;
+ isoCodeToUse = kDefaultCurrency;
+ } else if (!uprv_isInvariantUString(_isoCode, 3)) {
+ // TODO: Perform a more strict ASCII check like in ICU4J isAlpha3Code?
+ isoCodeToUse = kDefaultCurrency;
+ ec = U_INVARIANT_CONVERSION_ERROR;
} else {
- u_strcpy(isoCode, _isoCode);
+ isoCodeToUse = _isoCode;
}
+ u_strncpy(isoCode, isoCodeToUse, 3);
+ isoCode[3] = 0;
char simpleIsoCode[4];
u_UCharsToChars(isoCode, simpleIsoCode, 4);
initCurrency(simpleIsoCode);
diff --git a/icu4c/source/i18n/decimfmt.cpp b/icu4c/source/i18n/decimfmt.cpp
index c7192dc4691..42cd90233bf 100644
--- a/icu4c/source/i18n/decimfmt.cpp
+++ b/icu4c/source/i18n/decimfmt.cpp
@@ -992,7 +992,6 @@ void DecimalFormat::setCurrency(const char16_t* theCurrency, UErrorCode& ec) {
void DecimalFormat::setCurrency(const char16_t* theCurrency) {
ErrorCode localStatus;
- NumberFormat::setCurrency(theCurrency, localStatus); // to set field for compatibility
setCurrency(theCurrency, localStatus);
}
diff --git a/icu4c/source/i18n/number_skeletons.cpp b/icu4c/source/i18n/number_skeletons.cpp
index d921b547e13..e998e8e51c3 100644
--- a/icu4c/source/i18n/number_skeletons.cpp
+++ b/icu4c/source/i18n/number_skeletons.cpp
@@ -800,8 +800,12 @@ blueprint_helpers::parseExponentSignOption(const StringSegment& segment, MacroPr
void blueprint_helpers::parseCurrencyOption(const StringSegment& segment, MacroProps& macros,
UErrorCode& status) {
- // Can't use toTempUnicodeString() because getTerminatedBuffer is non-const
- const UChar* currencyCode = segment.toUnicodeString().getTerminatedBuffer();
+ // Unlike ICU4J, have to check length manually because ICU4C CurrencyUnit does not check it for us
+ if (segment.length() != 3) {
+ status = U_NUMBER_SKELETON_SYNTAX_ERROR;
+ return;
+ }
+ const UChar* currencyCode = segment.toTempUnicodeString().getBuffer();
UErrorCode localStatus = U_ZERO_ERROR;
CurrencyUnit currency(currencyCode, localStatus);
if (U_FAILURE(localStatus)) {
diff --git a/icu4c/source/i18n/unicode/currunit.h b/icu4c/source/i18n/unicode/currunit.h
index 5ad23b1f58a..ca90acb7918 100644
--- a/icu4c/source/i18n/unicode/currunit.h
+++ b/icu4c/source/i18n/unicode/currunit.h
@@ -44,8 +44,9 @@ class U_I18N_API CurrencyUnit: public MeasureUnit {
/**
* Construct an object with the given ISO currency code.
- * @param isoCode the 3-letter ISO 4217 currency code; must not be
- * NULL and must have length 3
+ * @param isoCode the 3-letter ISO 4217 currency code; must have
+ * length 3 and need not be NUL-terminated. If NULL, the currency
+ * is initialized to the unknown currency XXX.
* @param ec input-output error code. If the isoCode is invalid,
* then this will be set to a failing value.
* @stable ICU 3.0
diff --git a/icu4c/source/test/intltest/numbertest_skeletons.cpp b/icu4c/source/test/intltest/numbertest_skeletons.cpp
index 33de5c311b2..59ad630ce9c 100644
--- a/icu4c/source/test/intltest/numbertest_skeletons.cpp
+++ b/icu4c/source/test/intltest/numbertest_skeletons.cpp
@@ -68,6 +68,7 @@ void NumberSkeletonTest::validTokens() {
u"measure-unit/energy-joule per-measure-unit/length-meter",
u"currency/XXX",
u"currency/ZZZ",
+ u"currency/usd",
u"group-off",
u"group-min2",
u"group-auto",
@@ -137,6 +138,7 @@ void NumberSkeletonTest::invalidTokens() {
u"scale/0.1.2",
u"scale/français", // non-invariant characters for C++
u"currency/dummy",
+ u"currency/ççç", // three characters but not ASCII
u"measure-unit/foo",
u"integer-width/xxx",
u"integer-width/0+",
diff --git a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberSkeletonTest.java b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberSkeletonTest.java
index 7c7349b5e99..6e854dfbf2c 100644
--- a/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberSkeletonTest.java
+++ b/icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberSkeletonTest.java
@@ -60,6 +60,7 @@ public class NumberSkeletonTest {
"measure-unit/energy-joule per-measure-unit/length-meter",
"currency/XXX",
"currency/ZZZ",
+ "currency/usd",
"group-off",
"group-min2",
"group-auto",
@@ -131,6 +132,7 @@ public class NumberSkeletonTest {
"scale/0.1.2",
"scale/français", // non-invariant characters for C++
"currency/dummy",
+ "currency/ççç", // three characters but not ASCII
"measure-unit/foo",
"integer-width/xxx",
"integer-width/0+",
--
2.40.0