Fix GetPreviousOccurrence returning a day in a disallowed month
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.
This commit is contained in:
1 parent
9daaf8b304
commit
e055ee7ac6
3 files changed
+160
No files matched your search
@@ -0,0 +1,122 @@
|
|||||||
|
# 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):
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
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:
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
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.
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
if (maxDay < CronField.DaysOfMonth.First)
|
||||||
|
{
|
||||||
|
day = default;
|
||||||
|
actualDay = default;
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
## Test
|
||||||
|
|
||||||
|
`tests/Cronos.Tests/CronExpressionReverseFacts.cs`:
|
||||||
|
`GetPreviousOccurrence_DoesNotReturnDayInDisallowedMonth_WhenTimeRollsBackPastFirstOfMonth`
|
||||||
|
- `0 17 * 7 *` from `2017-07-01 14:00:00` expects `2016-07-31 17:00:00`
|
||||||
|
- `37-57/3 17-21 * 7 *` from `2017-07-01 14:20:00` expects `2016-07-31 21:55:00`
|
||||||
|
- `0 0 * 3 *` from `2020-03-01 00:00:00` expects `2019-03-31 00:00:00`
|
||||||
|
- Control `0 17 * * *` from `2017-07-01 14:00:00` expects `2017-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.
|
||||||
@@ -995,6 +995,20 @@ namespace Cronos
|
|||||||
var lastDayOfMonth = CalendarHelper.GetDaysInMonth(year, month);
|
var lastDayOfMonth = CalendarHelper.GetDaysInMonth(year, month);
|
||||||
var maxDay = dayLimit > lastDayOfMonth ? lastDayOfMonth : dayLimit;
|
var maxDay = dayLimit > lastDayOfMonth ? lastDayOfMonth : dayLimit;
|
||||||
|
|
||||||
|
// When the time-of-day search rolls back past the 1st of the month, dayLimit
|
||||||
|
// (and therefore maxDay) can drop below the first valid day of month. In that
|
||||||
|
// case there is no matching day left in this month, so the caller must move to
|
||||||
|
// the previous month. Without this guard the day-of-month "*" fast path below
|
||||||
|
// would return actualDay = 0, and DateTimeToTicks(year, month, 0, ...) would
|
||||||
|
// silently underflow into the last day of the previous month, producing an
|
||||||
|
// occurrence in a month that is not allowed by the month field.
|
||||||
|
if (maxDay < CronField.DaysOfMonth.First)
|
||||||
|
{
|
||||||
|
day = default;
|
||||||
|
actualDay = default;
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
if (HasFlag(CronExpressionFlag.NearestWeekday))
|
if (HasFlag(CronExpressionFlag.NearestWeekday))
|
||||||
{
|
{
|
||||||
day = HasFlag(CronExpressionFlag.DayOfMonthLast)
|
day = HasFlag(CronExpressionFlag.DayOfMonthLast)
|
||||||
|
|||||||
@@ -125,6 +125,30 @@ namespace Cronos.Tests
|
|||||||
Assert.Equal(GetUtcDateTime(expectedString), previous);
|
Assert.Equal(GetUtcDateTime(expectedString), previous);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Theory]
|
||||||
|
|
||||||
|
// 'from' is the 1st of an allowed month, but earlier than the first matching
|
||||||
|
// time-of-day. The search rolls the time back past midnight of the 1st, so it
|
||||||
|
// must skip to the previous allowed month instead of returning a day in the
|
||||||
|
// preceding (not allowed) month.
|
||||||
|
[InlineData("0 17 * 7 *", "2017-07-01 14:00:00", "2016-07-31 17:00:00")]
|
||||||
|
[InlineData("37-57/3 17-21 * 7 *", "2017-07-01 14:20:00", "2016-07-31 21:55:00")]
|
||||||
|
[InlineData("0 0 * 3 *", "2020-03-01 00:00:00", "2019-03-31 00:00:00")]
|
||||||
|
|
||||||
|
// Control: when the month field is unrestricted, rolling back into the previous
|
||||||
|
// month is correct and must not be skipped.
|
||||||
|
[InlineData("0 17 * * *", "2017-07-01 14:00:00", "2017-06-30 17:00:00")]
|
||||||
|
public void GetPreviousOccurrence_DoesNotReturnDayInDisallowedMonth_WhenTimeRollsBackPastFirstOfMonth(
|
||||||
|
string cronExpression, string fromString, string expectedString)
|
||||||
|
{
|
||||||
|
var expression = CronExpression.Parse(cronExpression, CronFormat.Standard);
|
||||||
|
var from = GetUtcDateTime(fromString);
|
||||||
|
|
||||||
|
var previous = expression.GetPreviousOccurrence(from, inclusive: false);
|
||||||
|
|
||||||
|
Assert.Equal(GetUtcDateTime(expectedString), previous);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public void GetPreviousOccurrence_ReturnsCorrectDate_WhenMacroExpressionHasJitterSeed()
|
public void GetPreviousOccurrence_ReturnsCorrectDate_WhenMacroExpressionHasJitterSeed()
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in new issue
Block a user