Skip to content

Commit

Permalink
ICU-22507 Fix stack overflow in ChineseCalendar::isLeapMonthBetween
Browse files Browse the repository at this point in the history
Rewrite the recursive call to while loop to avoid stack overflow
when the two values have big gap.
Include tests to verify the problem in unit test.
  • Loading branch information
FrankYFTang committed Sep 21, 2023
1 parent 1b980e5 commit 4fcf8d2
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 10 deletions.
34 changes: 25 additions & 9 deletions icu4c/source/i18n/chnsecal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

#include "chnsecal.h"

#include <cstdint>

#if !UCONFIG_NO_FORMATTING

#include "umutex.h"
Expand Down Expand Up @@ -383,7 +385,7 @@ void ChineseCalendar::add(UCalendarDateFields field, int32_t amount, UErrorCode&
int32_t day = get(UCAL_JULIAN_DAY, status) - kEpochStartAsJulianDay; // Get local day
if (U_FAILURE(status)) break;
int32_t moon = day - dom + 1; // New moon
offsetMonth(moon, dom, amount);
offsetMonth(moon, dom, amount, status);
}
break;
default:
Expand Down Expand Up @@ -453,7 +455,7 @@ void ChineseCalendar::roll(UCalendarDateFields field, int32_t amount, UErrorCode
}

if (newM != m) {
offsetMonth(moon, dom, newM - m);
offsetMonth(moon, dom, newM - m, status);
}
}
break;
Expand Down Expand Up @@ -653,9 +655,13 @@ UBool ChineseCalendar::isLeapMonthBetween(int32_t newMoon1, int32_t newMoon2) co
}
#endif

return (newMoon2 >= newMoon1) &&
(isLeapMonthBetween(newMoon1, newMoonNear(newMoon2 - SYNODIC_GAP, false)) ||
hasNoMajorSolarTerm(newMoon2));
while (newMoon2 >= newMoon1) {
if (hasNoMajorSolarTerm(newMoon2)) {
return true;
}
newMoon2 = newMoonNear(newMoon2 - SYNODIC_GAP, false);
}
return false;
}

/**
Expand Down Expand Up @@ -805,12 +811,21 @@ int32_t ChineseCalendar::newYear(int32_t gyear) const {
* @param dom the 1-based day-of-month of the start position
* @param delta the number of months to move forward or backward from
* the start position
* @param status The status.
*/
void ChineseCalendar::offsetMonth(int32_t newMoon, int32_t dom, int32_t delta) {
UErrorCode status = U_ZERO_ERROR;
void ChineseCalendar::offsetMonth(int32_t newMoon, int32_t dom, int32_t delta,
UErrorCode& status) {
if (U_FAILURE(status)) { return; }

// Move to the middle of the month before our target month.
newMoon += (int32_t) (CalendarAstronomer::SYNODIC_MONTH * (delta - 0.5));
double value = newMoon;
value += (CalendarAstronomer::SYNODIC_MONTH *
(static_cast<double>(delta) - 0.5));
if (value < INT32_MIN || value > INT32_MAX) {
status = U_ILLEGAL_ARGUMENT_ERROR;
return;
}
newMoon = static_cast<int32_t>(value);

// Search forward to the target month's new moon
newMoon = newMoonNear(newMoon, true);
Expand Down Expand Up @@ -969,10 +984,11 @@ int32_t ChineseCalendar::internalGetMonth() const {
// months.
UErrorCode status = U_ZERO_ERROR;
temp->roll(UCAL_MONTH, internalGet(UCAL_ORDINAL_MONTH), status);

U_ASSERT(U_SUCCESS(status));

ChineseCalendar *nonConstThis = (ChineseCalendar*)this; // cast away const
nonConstThis->internalSet(UCAL_IS_LEAP_MONTH, temp->get(UCAL_IS_LEAP_MONTH, status));
U_ASSERT(U_SUCCESS(status));
int32_t month = temp->get(UCAL_MONTH, status);
U_ASSERT(U_SUCCESS(status));
nonConstThis->internalSet(UCAL_MONTH, month);
Expand Down
2 changes: 1 addition & 1 deletion icu4c/source/i18n/chnsecal.h
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ class U_I18N_API ChineseCalendar : public Calendar {
virtual void computeChineseFields(int32_t days, int32_t gyear,
int32_t gmonth, UBool setAllFields);
virtual int32_t newYear(int32_t gyear) const;
virtual void offsetMonth(int32_t newMoon, int32_t dom, int32_t delta);
virtual void offsetMonth(int32_t newMoon, int32_t dom, int32_t delta, UErrorCode& status);
const TimeZone* getChineseCalZoneAstroCalc() const;

// UObject stuff
Expand Down
17 changes: 17 additions & 0 deletions icu4c/source/test/intltest/caltest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ void CalendarTest::runIndexedTest( int32_t index, UBool exec, const char* &name,
TESTCASE_AUTO(TestClearMonth);

TESTCASE_AUTO(TestFWWithISO8601);
TESTCASE_AUTO(TestDangiOverflowIsLeapMonthBetween22507);

TESTCASE_AUTO_END;
}
Expand Down Expand Up @@ -5499,6 +5500,22 @@ void CalendarTest::TestChineseCalendarMonthInSpecialYear() {
}
}

// Test the stack will not overflow with dangi calendar during "roll".
void CalendarTest::TestDangiOverflowIsLeapMonthBetween22507() {
Locale locale("en@calendar=dangi");
UErrorCode status = U_ZERO_ERROR;
LocalPointer<Calendar> cal(Calendar::createInstance(
*TimeZone::getGMT(), locale, status));
cal->clear();
status = U_ZERO_ERROR;
cal->add(UCAL_MONTH, 1242972234, status);
status = U_ZERO_ERROR;
cal->roll(UCAL_MONTH, 1249790538, status);
status = U_ZERO_ERROR;
// Without the fix, the stack will overflow during this roll().
cal->roll(UCAL_MONTH, 1246382666, status);
}

void CalendarTest::TestFWWithISO8601() {
// ICU UCAL_SUNDAY is 1, UCAL_MONDAY is 2, ... UCAL_SATURDAY is 7.
const char *locales[] = {
Expand Down
1 change: 1 addition & 0 deletions icu4c/source/test/intltest/caltest.h
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ class CalendarTest: public CalendarTimeZoneTest {
void TestCalendarRollOrdinalMonth();
void TestLimitsOrdinalMonth();
void TestActualLimitsOrdinalMonth();
void TestDangiOverflowIsLeapMonthBetween22507();

void TestFWWithISO8601();

Expand Down

1 comment on commit 4fcf8d2

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 4fcf8d2 Previous: 1b980e5 Ratio
TestCharsetEncoderICU 9.936867734122815 ns/iter 4.062331026351979 ns/iter 2.45

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.