From e055ee7ac6d20da2903d13fcfd74469736068967 Mon Sep 17 00:00:00 2001 From: dualfroz Date: Fri, 4 Sep 2026 09:32:17 +0200 Subject: [PATCH] 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. --- PR.md | 122 ++++++++++++++++++ src/Cronos/CronExpression.cs | 14 ++ .../CronExpressionReverseFacts.cs | 24 ++++ 3 files changed, 160 insertions(+) create mode 100644 PR.md diff --git a/PR.md b/PR.md new file mode 100644 index 0000000..3be67ae --- /dev/null +++ b/PR.md @@ -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. diff --git a/src/Cronos/CronExpression.cs b/src/Cronos/CronExpression.cs index 9197940..8a55ccb 100644 --- a/src/Cronos/CronExpression.cs +++ b/src/Cronos/CronExpression.cs @@ -995,6 +995,20 @@ namespace Cronos var lastDayOfMonth = CalendarHelper.GetDaysInMonth(year, month); 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)) { day = HasFlag(CronExpressionFlag.DayOfMonthLast) diff --git a/tests/Cronos.Tests/CronExpressionReverseFacts.cs b/tests/Cronos.Tests/CronExpressionReverseFacts.cs index 437e9dd..dd6dd80 100644 --- a/tests/Cronos.Tests/CronExpressionReverseFacts.cs +++ b/tests/Cronos.Tests/CronExpressionReverseFacts.cs @@ -125,6 +125,30 @@ namespace Cronos.Tests 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] public void GetPreviousOccurrence_ReturnsCorrectDate_WhenMacroExpressionHasJitterSeed() {