ICU-21000 Fix abort called by DateTimePatternGenerator::getDefaultHourCycle

If you call the API getDefaultHourCycle on an empty DateTimePatternGenerator
instance (ie: no locale) then it calls UPRV_UNREACHABLE which calls abort().
We should return an error code instead of aborting.

diff --git a/icu4c/source/i18n/dtptngen.cpp b/icu4c/source/i18n/dtptngen.cpp
index 4d4a3e5b2675696a5ff90135eee45d7052af1ea9..26ebee2d2021b3a8cdaa5e249bf4c87d423d52a0 100644 (file)
--- a/icu4c/source/i18n/dtptngen.cpp
+++ b/icu4c/source/i18n/dtptngen.cpp
@@ -713,19 +713,28 @@ void DateTimePatternGenerator::getAllowedHourFormats(const Locale &locale, UErro
 }
 
 UDateFormatHourCycle
-DateTimePatternGenerator::getDefaultHourCycle(UErrorCode& /*status*/) const {
-  switch(fDefaultHourFormatChar) {
-    case CAP_K:
-      return UDAT_HOUR_CYCLE_11;
-    case LOW_H:
-      return UDAT_HOUR_CYCLE_12;
-    case CAP_H:
-      return UDAT_HOUR_CYCLE_23;
-    case LOW_K:
-      return UDAT_HOUR_CYCLE_24;
-    default:
-      UPRV_UNREACHABLE;
-  }
+DateTimePatternGenerator::getDefaultHourCycle(UErrorCode& status) const {
+    if (U_FAILURE(status)) {
+        return UDAT_HOUR_CYCLE_23;
+    }
+    if (fDefaultHourFormatChar == 0) {
+        // We need to return something, but the caller should ignore it
+        // anyways since the returned status is a failure.
+        status = U_UNSUPPORTED_ERROR;
+        return UDAT_HOUR_CYCLE_23;
+    }
+    switch (fDefaultHourFormatChar) {
+        case CAP_K:
+            return UDAT_HOUR_CYCLE_11;
+        case LOW_H:
+            return UDAT_HOUR_CYCLE_12;
+        case CAP_H:
+            return UDAT_HOUR_CYCLE_23;
+        case LOW_K:
+            return UDAT_HOUR_CYCLE_24;
+        default:
+            UPRV_UNREACHABLE;
+    }
 }
 
 UnicodeString

diff --git a/icu4c/source/i18n/unicode/dtptngen.h b/icu4c/source/i18n/unicode/dtptngen.h
index af74059c745cc004c77c18694f5a59c6eaa11414..35736a0ea3f1fcf03b13d91063a2ce33f9293f58 100644 (file)
--- a/icu4c/source/i18n/unicode/dtptngen.h
+++ b/icu4c/source/i18n/unicode/dtptngen.h
@@ -485,9 +485,14 @@ public:
 
 #ifndef U_HIDE_DRAFT_API
     /**
-     * Get the default hour cycle.
-     * @param status  Output param set to success/failure code on exit,
-     *               which must not indicate a failure before the function call.
+     * Get the default hour cycle for a locale. Uses the locale that the
+     * DateTimePatternGenerator was initially created with.
+     * 
+     * Cannot be used on an empty DateTimePatternGenerator instance.
+     * 
+     * @param status  Output param set to success/failure code on exit, which
+     *                which must not indicate a failure before the function call.
+     *                Set to U_UNSUPPORTED_ERROR if used on an empty instance.
      * @return the default hour cycle.
      * @draft ICU 67
      */

diff --git a/icu4c/source/i18n/unicode/udatpg.h b/icu4c/source/i18n/unicode/udatpg.h
index 2b3982894264f0622b1b69d283420cf8ba1077d0..74c812ff85b5ebb655ab68c540b7bf296df81056 100644 (file)
--- a/icu4c/source/i18n/unicode/udatpg.h
+++ b/icu4c/source/i18n/unicode/udatpg.h
@@ -654,11 +654,15 @@ udatpg_getPatternForSkeleton(const UDateTimePatternGenerator *dtpg,
 
 #ifndef U_HIDE_DRAFT_API
 /**
- * Return the default hour cycle.
- *
+ * Return the default hour cycle for a locale. Uses the locale that the
+ * UDateTimePatternGenerator was initially created with.
+ * 
+ * Cannot be used on an empty UDateTimePatternGenerator instance.
+ * 
  * @param dtpg a pointer to UDateTimePatternGenerator.
  * @param pErrorCode a pointer to the UErrorCode which must not indicate a
- *                   failure before the function call.
+ *                   failure before the function call. Set to U_UNSUPPORTED_ERROR
+ *                   if used on an empty instance.
  * @return the default hour cycle.
  * @draft ICU 67
  */

diff --git a/icu4c/source/test/cintltst/udatpg_test.c b/icu4c/source/test/cintltst/udatpg_test.c
index b7d8cd79ed9c984f0d2f7207b75e287d64f729ea..3dc145d15d283454d7a36093a6a87dd52f918445 100644 (file)
--- a/icu4c/source/test/cintltst/udatpg_test.c
+++ b/icu4c/source/test/cintltst/udatpg_test.c
@@ -44,6 +44,7 @@ static void TestBuilder(void);
 static void TestOptions(void);
 static void TestGetFieldDisplayNames(void);
 static void TestGetDefaultHourCycle(void);
+static void TestGetDefaultHourCycleOnEmptyInstance(void);
 
 void addDateTimePatternGeneratorTest(TestNode** root) {
     TESTCASE(TestOpenClose);
@@ -52,6 +53,7 @@ void addDateTimePatternGeneratorTest(TestNode** root) {
     TESTCASE(TestOptions);
     TESTCASE(TestGetFieldDisplayNames);
     TESTCASE(TestGetDefaultHourCycle);
+    TESTCASE(TestGetDefaultHourCycleOnEmptyInstance);
 }
 
 /*
@@ -554,4 +556,28 @@ static void TestGetDefaultHourCycle() {
     }
 }
 
+// Ensure that calling udatpg_getDefaultHourCycle on an empty instance doesn't call UPRV_UNREACHABLE/abort.
+static void TestGetDefaultHourCycleOnEmptyInstance() {
+    UErrorCode status = U_ZERO_ERROR;
+    UDateTimePatternGenerator * dtpgen = udatpg_openEmpty(&status);
+
+    if (U_FAILURE(status)) {
+        log_data_err("ERROR udatpg_openEmpty failed, status: %s \n", myErrorName(status));
+        return;
+    }
+
+    (void)udatpg_getDefaultHourCycle(dtpgen, &status);
+    if (!U_FAILURE(status)) {
+        log_data_err("ERROR expected udatpg_getDefaultHourCycle on an empty instance to fail, status: %s", myErrorName(status));
+    }
+
+    status = U_USELESS_COLLATOR_ERROR;
+    (void)udatpg_getDefaultHourCycle(dtpgen, &status);
+    if (status != U_USELESS_COLLATOR_ERROR) {
+        log_data_err("ERROR udatpg_getDefaultHourCycle shouldn't modify status if it is already failed, status: %s", myErrorName(status));
+    }
+
+    udatpg_close(dtpgen);
+}
+
 #endif

diff --git a/icu4c/source/test/intltest/dtptngts.cpp b/icu4c/source/test/intltest/dtptngts.cpp
index 0a9c5eaf9b3dfac057581924396b31ea13507993..3a0f9edbc1059e250b5837e7bfde4fa64308af5c 100644 (file)
--- a/icu4c/source/test/intltest/dtptngts.cpp
+++ b/icu4c/source/test/intltest/dtptngts.cpp
@@ -43,6 +43,7 @@ void IntlTestDateTimePatternGeneratorAPI::runIndexedTest( int32_t index, UBool e
         TESTCASE(7, testJjMapping);
         TESTCASE(8, test20640_HourCyclArsEnNH);
         TESTCASE(9, testFallbackWithDefaultRootLocale);
+        TESTCASE(10, testGetDefaultHourCycle_OnEmptyInstance);
         default: name = ""; break;
     }
 }
@@ -1483,4 +1484,27 @@ void IntlTestDateTimePatternGeneratorAPI::testFallbackWithDefaultRootLocale() {
     }
 }
 
+// ICU-21000 Ensure that calling getDefaultHourCycle on an empty instance doesn't call UPRV_UNREACHABLE/abort.
+void IntlTestDateTimePatternGeneratorAPI::testGetDefaultHourCycle_OnEmptyInstance() {
+    UErrorCode status = U_ZERO_ERROR;
+
+    LocalPointer<DateTimePatternGenerator> dtpg(DateTimePatternGenerator::createEmptyInstance(status), status);
+    if (U_FAILURE(status)) {
+        errln("ERROR: createEmptyInstance failed, status: %s", u_errorName(status));
+        return;
+    }
+    (void)dtpg->getDefaultHourCycle(status);
+    if (!U_FAILURE(status)) {
+        errln("ERROR: expected getDefaultHourCycle on an empty instance to fail, status: %s", u_errorName(status));
+        return;
+    }
+
+    status = U_USELESS_COLLATOR_ERROR;
+    (void)dtpg->getDefaultHourCycle(status);
+    if (status != U_USELESS_COLLATOR_ERROR) {
+        errln("ERROR: getDefaultHourCycle shouldn't modify status if it is already failed, status: %s", u_errorName(status));
+        return;
+    }
+}
+
 #endif /* #if !UCONFIG_NO_FORMATTING */

diff --git a/icu4c/source/test/intltest/dtptngts.h b/icu4c/source/test/intltest/dtptngts.h
index c0bebccd3e34192fa1707f74c6aec858ae96f0d2..90f65c94288e8692d14365141bc08c5458209fe6 100644 (file)
--- a/icu4c/source/test/intltest/dtptngts.h
+++ b/icu4c/source/test/intltest/dtptngts.h
@@ -35,6 +35,7 @@ private:
     void testJjMapping();
     void test20640_HourCyclArsEnNH();
     void testFallbackWithDefaultRootLocale();
+    void testGetDefaultHourCycle_OnEmptyInstance();
 };
 
 #endif /* #if !UCONFIG_NO_FORMATTING */