ICU-20065 Prevent crash on Collator::makeInstance fail when optimized, add test

diff --git a/icu4c/source/i18n/coll.cpp b/icu4c/source/i18n/coll.cpp
index 8775d29cfe1be8d9273c949334092f6e16ae8679..f6d94be3553382ac4c05f6df4b394c2d2bbb58de 100644 (file)
--- a/icu4c/source/i18n/coll.cpp
+++ b/icu4c/source/i18n/coll.cpp
@@ -448,6 +448,13 @@ Collator* U_EXPORT2 Collator::createInstance(const Locale& desiredLocale,
 #endif
     {
         coll = makeInstance(desiredLocale, status);
+        // Either returns NULL with U_FAILURE(status), or non-NULL with U_SUCCESS(status)
+    }
+    // The use of *coll in setAttributesFromKeywords can cause causes the NULL check
+    // to be optimized out of the delete even though setAttributesFromKeywords returns
+    // immediately if U_FAILURE(status), so we add a check here.
+    if (U_FAILURE(status)) {
+        return NULL;
     }
     setAttributesFromKeywords(desiredLocale, *coll, status);
     if (U_FAILURE(status)) {

diff --git a/icu4c/source/test/cintltst/callcoll.c b/icu4c/source/test/cintltst/callcoll.c
index 191f0650e9a7ae14ed00cc4d2b3dd21f0ad96721..c69e3ee0034e519b1e018d83919c135dfc21ee89 100644 (file)
--- a/icu4c/source/test/cintltst/callcoll.c
+++ b/icu4c/source/test/cintltst/callcoll.c
@@ -93,6 +93,8 @@ static void TestFCDCrash(void);
 
 static void TestJ5298(void);
 
+static void TestBadKey(void);
+
 const UCollationResult results[] = {
     UCOL_LESS,
     UCOL_LESS, /*UCOL_GREATER,*/
@@ -210,6 +212,7 @@ void addAllCollTest(TestNode** root)
     addTest(root, &TestJitterbug1098, "tscoll/callcoll/TestJitterbug1098");
     addTest(root, &TestFCDCrash, "tscoll/callcoll/TestFCDCrash");
     addTest(root, &TestJ5298, "tscoll/callcoll/TestJ5298");
+    addTest(root, &TestBadKey, "tscoll/callcoll/TestBadKey");
 }
 
 UBool hasCollationElements(const char *locName) {
@@ -1343,4 +1346,36 @@ static void TestJ5298(void)
     uenum_close(values);
     log_verbose("\n");
 }
+
+static const char* badKeyLocales[] = {
+       "@calendar=japanese;collation=search", // ucol_open OK
+       "@calendar=japanese", // ucol_open OK
+       "en@calendar=x", // ucol_open OK
+       "ja@calendar=x", // ucol_open OK
+       "en@collation=x", // ucol_open OK
+       "ja@collation=x", // ucol_open OK
+       "ja@collation=private-kana", // ucol_open fails, verify it does not crash
+       "en@collation=\x80", // (x80 undef in ASCII,EBCDIC) ucol_open fails, verify it does not crash
+       NULL
+};
+
+// Mainly this is to check that we don't have a crash, but we check
+// for correct NULL return and FAILURE/SUCCESS status as a bonus.
+static void TestBadKey(void)
+{
+    const char* badLoc;
+    const char** badLocsPtr = badKeyLocales;
+    while ((badLoc = *badLocsPtr++) != NULL) {
+        UErrorCode status = U_ZERO_ERROR;
+        UCollator* uc = ucol_open(badLoc, &status);
+        if ( U_SUCCESS(status) ) {
+            if (uc == NULL) {
+                log_err("ucol_open sets SUCCESS but returns NULL, locale: %s\n", badLoc);
+            }
+            ucol_close(uc);
+        } else if (uc != NULL) {
+            log_err("ucol_open sets FAILURE but returns non-NULL, locale: %s\n", badLoc);
+        }
+    }
+}
 #endif /* #if !UCONFIG_NO_COLLATION */