When the time-of-day search rolls back past the 1st of the month, dayLimit (and maxDay) can drop below the first valid day of month. The day-of-month '*' fast path in TryGetPreviousDay returned actualDay = 0, and DateTimeToTicks(year, month, 0, ...) underflowed into the last day of the previous month, which is not re-validated against the month field. Guard against maxDay below the first day of month so the caller advances to the previous allowed month instead.
4.5 KiB
Fix GetPreviousOccurrence returning a day in a month excluded by the month field
Problem
CronExpression.GetPreviousOccurrence can return an occurrence whose month is not
allowed by the month field. It happens when the search starts on the 1st day of an
allowed month at a time-of-day that is earlier than the first matching time of that
day, and the previous calendar month is not part of the month field.
Example (UTC):
var expr = CronExpression.Parse("0 17 * 7 *"); // 17:00 on any day in July
var from = new DateTime(2017, 7, 1, 14, 0, 0, DateTimeKind.Utc);
var previous = expr.GetPreviousOccurrence(from, inclusive: false);
// returns 2017-06-30T17:00:00Z <-- June, which the expression never allows
// expected 2016-07-31T17:00:00Z
The returned value (June 30th) violates the most basic contract of the expression: the month field restricts occurrences to July only.
Root cause
src/Cronos/CronExpression.cs, TryGetPreviousDay.
FindPreviousOccurrence first rolls the time-of-day backwards. When from is on the
1st of the month and its time is earlier than the earliest matching time, the hour
underflows and day is decremented to 0 before TryGetPreviousDay(year, month, dayLimit: 0, ...)
is called.
Inside TryGetPreviousDay, maxDay becomes 0. For the common day-of-month == *
(and day-of-week == *) case there is a fast path:
day = maxDay; // 0
actualDay = day; // 0
if (_dayOfWeek == CronField.DaysOfWeek.AllBits)
{
return true; // reports day 0 as a valid match
}
It returns actualDay = 0 without checking that maxDay is a real day of month.
The caller then calls CalendarHelper.DateTimeToTicks(year, month, 0, ...), whose
day arithmetic (... + day - 1) silently underflows day 0 into the last day of the
previous month. That preceding month is never re-validated against the month field,
so the disallowed month leaks into the result.
The other branches of TryGetPreviousDay (nearest-weekday, last-day-of-month, and the
non-fast day-of-month path) already reject maxDay < 1 via their <= maxDay checks or
MoveBackDay; only the */* fast path was missing the guard.
Fix
Return false from TryGetPreviousDay when maxDay is below the first valid day of
month. That makes the caller advance to the previous month (RetryMonth), where the
month field is honored again. The guard is placed once at the top so it also documents
the invariant for every branch.
if (maxDay < CronField.DaysOfMonth.First)
{
day = default;
actualDay = default;
return false;
}
Test
tests/Cronos.Tests/CronExpressionReverseFacts.cs:
GetPreviousOccurrence_DoesNotReturnDayInDisallowedMonth_WhenTimeRollsBackPastFirstOfMonth
0 17 * 7 *from2017-07-01 14:00:00expects2016-07-31 17:00:0037-57/3 17-21 * 7 *from2017-07-01 14:20:00expects2016-07-31 21:55:000 0 * 3 *from2020-03-01 00:00:00expects2019-03-31 00:00:00- Control
0 17 * * *from2017-07-01 14:00:00expects2017-06-30 17:00:00(month field is unrestricted, so rolling back into the previous month is correct and must not be skipped).
The bug was originally discovered with a differential fuzzer that compares
GetNextOccurrence/GetPreviousOccurrence against an independent minute-resolution
scan and self-consistency invariants (inclusive round-trip). After the fix the fuzzer
reports no discrepancies.
Verification
Target framework built and tested: net6.0 (from the project's
net462;net6.0;net8.0 test matrix).
New test, before the fix:
dotnet test tests/Cronos.Tests/Cronos.Tests.csproj -f net6.0 \
--filter FullyQualifiedName~GetPreviousOccurrence_DoesNotReturnDayInDisallowedMonth
Failed! - Failed: 2, Passed: 2, Total: 4
Assert.Equal() Failure
Expected: 2016-07-31T17:00:00.0000000Z
Actual: 2017-06-30T17:00:00.0000000Z
New test, after the fix:
Passed! - Failed: 0, Passed: 4, Skipped: 0, Total: 4
Full test project on net6.0 after the fix:
Failed! - Failed: 12, Passed: 1860, Skipped: 0, Total: 1872
The 12 failures are pre-existing and unrelated to this change: they are the DST /
time-zone cases
(GetNextOccurrence_HandleDifficultDSTCases_*,
GetNextOccurrence_HandleDST_WhenTheClockJumpsForwardAndFromIsAroundDST,
GetNextOccurrence_ReturnsAGreaterValue_EvenWhenMillisecondTruncationRuleIsAppliedDueToDST)
that depend on the host IANA time-zone database and fail identically on the unmodified
main branch in this environment. This change adds 4 passing tests and introduces no
new failures.