From 643e8756c8f8d2a7f2ffe306467e28a9896d7986 Mon Sep 17 00:00:00 2001
From: Peter Edberg <pedberg@apple.com>
Date: Sat, 11 Aug 2018 17:52:00 -0700
Subject: [PATCH] ICU-20065 Prevent crash on Collator::makeInstance fail when
 optimized, add test

---
 icu4c/source/i18n/coll.cpp            |  7 ++++++
 icu4c/source/test/cintltst/callcoll.c | 35 +++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/icu4c/source/i18n/coll.cpp b/icu4c/source/i18n/coll.cpp
index 8775d29cfe1..f6d94be3553 100644
--- 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 191f0650e9a..c69e3ee0034 100644
--- 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 */
-- 
2.40.0