dualfroz
e055ee7ac6
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.
123 lines
4.5 KiB
Markdown
123 lines
4.5 KiB
Markdown
# 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.
|