commit ddadc9427b243e5224f3b398d96e0b99d9301642
author Steven R. Loomis <srloomis@us.ibm.com> Wed Aug 28 17:04:27 2019 -0700
committer Fredrik Roubert <fredrik@roubert.name> Wed Sep 04 23:20:31 2019 +0200
tree a4420246c92a116f6c2578843ef5178ef7a4102d
parent 0b5b3bcdb10f0086e77d2eaed7c812232c3246b3

ICU-13745 fix undefined behavior: GregorianCalendar::setGregorianChange()

- Julian days outside of INT32_MIN..INT32_MAX are normalized
- Add a test case

icu4c/source/i18n/gregocal.cpp

diff --git a/icu4c/source/i18n/gregocal.cpp b/icu4c/source/i18n/gregocal.cpp
index ee15b52..6b15171 100644
--- a/icu4c/source/i18n/gregocal.cpp
+++ b/icu4c/source/i18n/gregocal.cpp
@@ -324,26 +324,26 @@
     if (U_FAILURE(status)) 
         return;
 
-    fGregorianCutover = date;
-
     // Precompute two internal variables which we use to do the actual
     // cutover computations.  These are the normalized cutover, which is the
     // midnight at or before the cutover, and the cutover year.  The
     // normalized cutover is in pure date milliseconds; it contains no time
     // of day or timezone component, and it used to compare against other
     // pure date values.
-    int32_t cutoverDay = (int32_t)ClockMath::floorDivide(fGregorianCutover, (double)kOneDay);
-    fNormalizedGregorianCutover = cutoverDay * kOneDay;
+    double cutoverDay = ClockMath::floorDivide(date, (double)kOneDay);
 
-    // Handle the rare case of numeric overflow.  If the user specifies a
-    // change of UDate(Long.MIN_VALUE), in order to get a pure Gregorian
-    // calendar, then the epoch day is -106751991168, which when multiplied
-    // by ONE_DAY gives 9223372036794351616 -- the negative value is too
-    // large for 64 bits, and overflows into a positive value.  We correct
-    // this by using the next day, which for all intents is semantically
-    // equivalent.
-    if (cutoverDay < 0 && fNormalizedGregorianCutover > 0) {
-        fNormalizedGregorianCutover = (cutoverDay + 1) * kOneDay;
+    // Handle the rare case of numeric overflow where the user specifies a time
+    // outside of INT32_MIN .. INT32_MAX number of days.
+    
+    if (cutoverDay <= INT32_MIN) {
+        cutoverDay = INT32_MIN;
+        fGregorianCutover = fNormalizedGregorianCutover = cutoverDay * kOneDay;
+    } else if (cutoverDay >= INT32_MAX) {
+        cutoverDay = INT32_MAX;
+        fGregorianCutover = fNormalizedGregorianCutover = cutoverDay * kOneDay;
+    } else {
+        fNormalizedGregorianCutover = cutoverDay * kOneDay;
+        fGregorianCutover = date;
     }
 
     // Normalize the year so BC values are represented as 0 and negative
@@ -360,7 +360,7 @@
     fGregorianCutoverYear = cal->get(UCAL_YEAR, status);
     if (cal->get(UCAL_ERA, status) == BC) 
         fGregorianCutoverYear = 1 - fGregorianCutoverYear;
-    fCutoverJulianDay = cutoverDay;
+    fCutoverJulianDay = (int32_t)cutoverDay;
     delete cal;
 }
 

icu4c/source/test/intltest/calregts.cpp

diff --git a/icu4c/source/test/intltest/calregts.cpp b/icu4c/source/test/intltest/calregts.cpp
index 8806d04..744ca69 100644
--- a/icu4c/source/test/intltest/calregts.cpp
+++ b/icu4c/source/test/intltest/calregts.cpp
@@ -96,6 +96,7 @@
         CASE(52,TestPersianCalOverflow);
         CASE(53,TestIslamicCalOverflow);
         CASE(54,TestWeekOfYear13548);
+        CASE(55,Test13745);
     default: name = ""; break;
     }
 }
@@ -1536,6 +1537,41 @@
     delete cal2;
 }
 
+const UDate MILLIS_IN_DAY = 86400000.0;
+/**
+ * ICU-13745
+ * GregorianCalendar::setGregorianChange() overflow
+ */
+void CalendarRegressionTest::Test13745()
+{
+    UErrorCode status = U_ZERO_ERROR;
+    GregorianCalendar *cal = new GregorianCalendar(status);
+    if(U_FAILURE(status)) {
+      dataerrln("Error creating calendar %s", u_errorName(status));
+      delete cal;
+      return;
+    }
+
+    // this line would overflow before fix 13745
+    cal->setGregorianChange(((double)INT32_MAX+1.0) * MILLIS_IN_DAY, status);
+    if(U_FAILURE(status)) {
+        errln("%s:%d Failure setting INT32_MAX+1 change on calendar: %s\n", __FILE__, __LINE__, u_errorName(status));
+        return;
+    }
+    assertEquals("getGregorianChange()", (double)INT32_MAX * MILLIS_IN_DAY, cal->getGregorianChange());
+
+    // test underflow
+    cal->setGregorianChange(((double)INT32_MIN-1.0) * MILLIS_IN_DAY, status);
+    if(U_FAILURE(status)) {
+        errln("%s:%d Failure setting INT32_MAX-1 change on calendar: %s\n", __FILE__, __LINE__, u_errorName(status));
+        return;
+    }
+    assertEquals("getGregorianChange()", (double)INT32_MIN * MILLIS_IN_DAY, cal->getGregorianChange());
+
+    delete cal;
+}
+
+
 /**
  * @bug 4142933
  * Bug states that ArrayIndexOutOfBoundsException is thrown by GregorianCalendar::roll()

icu4c/source/test/intltest/calregts.h

diff --git a/icu4c/source/test/intltest/calregts.h b/icu4c/source/test/intltest/calregts.h
index b4166a0..6fc4bdd 100644
--- a/icu4c/source/test/intltest/calregts.h
+++ b/icu4c/source/test/intltest/calregts.h
@@ -82,6 +82,8 @@
     void TestIslamicCalOverflow(void);
     void TestWeekOfYear13548(void);
 
+    void Test13745(void);
+
     void printdate(GregorianCalendar *cal, const char *string);
     void dowTest(UBool lenient) ;