Fixing datetime arithmetic overflow/underflow. - #5781
Conversation
v-isyamauchi-gh
added
Bug
There was a problem hiding this comment.
🟡 Changes recommended
The new safe arithmetic helpers and one call site can still produce incorrect results or overflow in edge cases (notably long.MinValue guard overflow and boundary-day semantics).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR prevents DateTime/DateTimeOffset arithmetic overflows in the FHIR search expression rewriting pipeline (notably SQL Server visitors and “ap” date comparator handling) by introducing saturating date arithmetic helpers and applying them to existing rewrite/optimization code paths.
Changes:
- Added
DateTimeSafeExtensions(SafeAddTicks/SafeAddDays) to clamp to min/max instead of throwing on overflow. - Updated SQL Server search expression visitors to use the safe arithmetic helpers in rewrite logic.
- Updated Core search expression builder logic for
apdate comparator to use safe tick arithmetic, and added unit tests for the new extension methods.
File summaries
| File | Description |
|---|---|
| src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/ScalarTemporalEqualityRewriter.cs | Uses safe datetime arithmetic in precision classification for temporal equality rewrites. |
| src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/LastUpdatedToResourceSurrogateIdRewriter.cs | Uses safe tick arithmetic when shifting millisecond-truncated _lastUpdated bounds into surrogate id space. |
| src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/DateTimeBoundedRangeRewriter.cs | Uses safe tick arithmetic when generating bounded datetime range optimizations. |
| src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/Parsers/SearchValueExpressionBuilderHelper.cs | Uses safe tick arithmetic for “ap” comparator approximate range generation. |
| src/Microsoft.Health.Fhir.Core/Extensions/DateTimeSafeExtensions.cs | New saturating arithmetic helpers for DateTime / DateTimeOffset. |
| src/Microsoft.Health.Fhir.Core.UnitTests/Extensions/DateTimeSafeExtensionsTests.cs | New unit tests covering common and near-boundary behaviors of safe arithmetic helpers. |
Review details
Suppressed comments (2)
src/Microsoft.Health.Fhir.Core/Extensions/DateTimeSafeExtensions.cs:51
- The DateTimeOffset SafeAddTicks guard has the same long.MinValue overflow issue (MinValue.Ticks - ticks overflows when ticks == long.MinValue), which can throw before clamping.
if (ticks < 0 && value.Ticks < DateTimeOffset.MinValue.Ticks - ticks)
{
return DateTimeOffset.MinValue;
}
src/Microsoft.Health.Fhir.Core/Extensions/DateTimeSafeExtensions.cs:63
- SafeAddDays computes ticks via
days * TimeSpan.TicksPerDay, but that multiplication can overflow long for large |days|, producing an incorrect wrapped tick count and defeating the clamping behavior.
public static DateTime SafeAddDays(this DateTime value, int days)
{
long ticks = days * TimeSpan.TicksPerDay;
return value.SafeAddTicks(ticks);
}
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The new “safe” DateTime helpers currently have correctness gaps (e.g., clamping loses DateTime.Kind and SafeAddDays can overflow via unchecked multiplication) that can still lead to incorrect behavior in the rewritten search pipeline.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.Health.Fhir.Core.UnitTests/Extensions/DateTimeSafeExtensionsTests.cs:10
- The using for Microsoft.Health.Test.Utilities appears unused in this test file and may trigger CS8019 (unnecessary using directive). Remove it to keep the test project warning-clean.
src/Microsoft.Health.Fhir.Core/Extensions/DateTimeSafeExtensions.cs:75
- SafeAddDays multiplies days * TimeSpan.TicksPerDay, which can overflow long for large |days| values (unchecked arithmetic) and produce an incorrect wrapped tick count. Since these helpers are explicitly meant to be overflow-safe, compute the clamp using division (no multiplication overflow) before converting days→ticks.
public static DateTime SafeAddDays(this DateTime value, int days)
{
long ticks = days * TimeSpan.TicksPerDay;
return value.SafeAddTicks(ticks);
}
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new DateTimeOffset safe helpers/documentation/tests have inconsistencies and edge cases where overflow handling can still throw (notably with non-zero/negative offsets), undermining the PR’s goal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
src/Microsoft.Health.Fhir.Core/Extensions/DateTimeSafeExtensions.cs:95
- SafeAddDays(DateTimeOffset) can throw for negative offsets when days overflows (e.g., int.MaxValue) because new DateTimeOffset(DateTimeOffset.MaxValue.Ticks, value.Offset) is not representable for offsets < 0 (it would push the implied UTC time past DateTime.MaxValue). This reintroduces the overflow exceptions this PR is trying to eliminate.
if (days > MaxDaysBeforeTicksOverflow || days < -MaxDaysBeforeTicksOverflow)
{
return days > 0 ? new DateTimeOffset(DateTimeOffset.MaxValue.Ticks, value.Offset) : new DateTimeOffset(DateTimeOffset.MinValue.Ticks, value.Offset);
}
src/Microsoft.Health.Fhir.Core/Extensions/DateTimeSafeExtensions.cs:88
- The XML doc for SafeAddDays(DateTimeOffset) claims it clamps to DateTimeOffset.MinValue/MaxValue, but the current implementation returns a value with the original offset (which is not equal to DateTimeOffset.MinValue/MaxValue unless the offset is zero). Update the doc comment to match the actual behavior.
/// <summary>
/// Adds the specified number of days to a <see cref="DateTimeOffset"/>, clamping the result
/// to <see cref="DateTimeOffset.MinValue"/> or <see cref="DateTimeOffset.MaxValue"/> on overflow.
/// </summary>
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
DateTimeOffset clamping can still throw for non-zero offsets and one added unit test does not correctly validate kind preservation (and may introduce an unused-variable warning).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
src/Microsoft.Health.Fhir.Core/Extensions/DateTimeSafeExtensions.cs:105
SafeAddDays(DateTimeOffset, int)has the same non-zero-offset problem asSafeAddTickswhen it clamps: constructingnew DateTimeOffset(DateTimeOffset.MinValue/MaxValue.Ticks, value.Offset)may itself throw. It’s safer to route the saturation case throughSafeAddTicksso the offset-specific representable bounds are applied consistently.
// Detect if days * TimeSpan.TicksPerDay would overflow long.
if (days > MaxDaysBeforeTicksOverflow || days < -MaxDaysBeforeTicksOverflow)
{
return days > 0 ? new DateTimeOffset(DateTimeOffset.MaxValue.Ticks, value.Offset) : new DateTimeOffset(DateTimeOffset.MinValue.Ticks, value.Offset);
}
src/Microsoft.Health.Fhir.Core.UnitTests/Extensions/DateTimeSafeExtensionsTests.cs:79
- This test doesn’t actually verify kind preservation: the
dtlocal is unused and the assertion expectsUnspecifiedbecause it’s usingDateTime.MaxValue(which isUnspecified) as the input. This can also introduce an unused-variable warning depending on build settings.
public void GivenUtcDateTime_WhenSafeAddTicksClampsToMaxValue_ThenPreservesKind()
{
var dt = new DateTime(2024, 1, 1, 0, 0, 0, DateTimeKind.Utc);
DateTime result = DateTime.MaxValue.AddTicks(-1).SafeAddTicks(TimeSpan.TicksPerDay);
Assert.Equal(DateTimeKind.Unspecified, result.Kind);
src/Microsoft.Health.Fhir.Core.UnitTests/Extensions/DateTimeSafeExtensionsTests.cs:132
- The implementation is intended to be safe for extreme values; add offset edge-case tests that would have thrown previously (underflow at +offset and overflow at -offset) to prevent regressions, especially since the clamping logic depends on UTC/local bound interactions.
[Fact]
public void GivenDateTimeOffset_WhenSafeAddTicksClampsWithNonZeroOffset_ThenPreservesOffset()
{
var offset = TimeSpan.FromHours(5);
var dto = new DateTimeOffset(9999, 12, 31, 23, 59, 59, offset);
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
_lastUpdated rewriting can still throw when converting large dates to surrogate IDs (ToSurrogateId enforces <= IdHelper.MaxDateTime), so some extreme-date queries may still result in 500s.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
One critical and two moderate unresolved findings remain in datetime rewrite logic.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/DateTimeBoundedRangeRewriter.cs:45
- When the left predicate is
GreaterThanand its value is within one day ofDateTimeOffset.MinValue, this clamp changes the derived optimization's meaning: the mathematical boundX - 1 dayis below the representable range, so every representableDateTimeStart(includingDateTime.MinValue) satisfies it, butDateTimeStart > DateTime.MinValueexcludes the exact-minimum row. Detect this underflow and make the bound inclusive, or skip the optimization for that case to avoid false negatives.
new BinaryExpression(left.BinaryOperator, FieldName.DateTimeStart, left.ComponentIndex, ((DateTimeOffset)left.Value).SafeAddTicks(-TimeSpan.TicksPerDay)),
src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/ScalarTemporalEqualityRewriter.cs:137
- For
birthdate=9999-12-31,start.SafeAddDays(1)saturates toDateTimeOffset.MaxValue, and subtracting one tick yieldsMaxValue - 1 tick; however,DateTimeSearchValue.Endfor that date isMaxValue. The exact-day value therefore failsPrecision.ExactDayand misses the end-only SQL optimization. Preserve the last representable day's true inclusive end when the day increment saturates.
if (end == start.SafeAddDays(1).SafeAddTicks(-1))
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical test compilation errors and a moderate underflow-boundary rewrite bug remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/DateTimeBoundedRangeRewriter.cs:45
- When the matched left predicate is
DateTimeEnd > XandXis within one day of the minimum representable date, this clamp turnsX - 1 dayinto the minimum value but keeps the strict>operator. The optimized branch therefore emitsDateTimeStart > DateTime.MinValueand drops valid short ranges whose start is exactly the minimum; adjust the bound/operator on underflow or skip this optimization at that boundary.
src/Microsoft.Health.Fhir.Core.UnitTests/Extensions/DateTimeSafeExtensionsTests.cs:481
ILogger.Log<TState>is generic, but thesedefaultliterals provide no type from which the compiler can inferTState, so this assertion fails with CS0411. Specify the state type (for example,Log<object>(...)) as done by the existing logger tests.
logger.ReceivedWithAnyArgs().Log(default, default, default, default, default!);
src/Microsoft.Health.Fhir.Core.UnitTests/Extensions/DateTimeSafeExtensionsTests.cs:493
ILogger.Log<TState>is generic, but thesedefaultliterals provide no type from which the compiler can inferTState, so this assertion fails with CS0411. Specify the state type (for example,Log<object>(...)) as done by the existing logger tests.
logger.DidNotReceiveWithAnyArgs().Log(default, default, default, default, default!);
src/Microsoft.Health.Fhir.Core.UnitTests/Extensions/DateTimeSafeExtensionsTests.cs:505
ILogger.Log<TState>is generic, but thesedefaultliterals provide no type from which the compiler can inferTState, so this assertion fails with CS0411. Specify the state type (for example,Log<object>(...)) as done by the existing logger tests.
logger.ReceivedWithAnyArgs().Log(default, default, default, default, default!);
src/Microsoft.Health.Fhir.Core.UnitTests/Extensions/DateTimeSafeExtensionsTests.cs:529
ILogger.Log<TState>is generic, but thesedefaultliterals provide no type from which the compiler can inferTState, so this assertion fails with CS0411. Specify the state type (for example,Log<object>(...)) as done by the existing logger tests.
logger.ReceivedWithAnyArgs().Log(default, default, default, default, default!);
src/Microsoft.Health.Fhir.Core.UnitTests/Extensions/DateTimeSafeExtensionsTests.cs:517
ILogger.Log<TState>is generic, but thesedefaultliterals provide no type from which the compiler can inferTState, so this assertion fails with CS0411. Specify the state type (for example,Log<object>(...)) as done by the existing logger tests.
logger.ReceivedWithAnyArgs().Log(default, default, default, default, default!);
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate boundary-correctness findings, each with 2 votes, remain unresolved.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/DateTimeBoundedRangeRewriter.cs:45
- When
left.BinaryOperatorisGreaterThanand subtracting a day underflows,SafeAddTicksclamps the value toDateTimeOffset.MinValuebut this line preserves the strict>operator. The derived optimization then requiresDateTimeStart > MinValue; a valid short range starting exactly atMinValuecan satisfy the originalDateTimeEnd > Xpredicate but be filtered out by this branch. Detect the underflow and either omit this added bound or change it to>= MinValue, with a regression test for agtsearch at the minimum date.
src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/ScalarTemporalEqualityRewriter.cs:137
- For
start = 9999-12-31T00:00:00Z,SafeAddDays(1)clamps toDateTimeOffset.MaxValue, so the chainedSafeAddTicks(-1)computesMaxValue - 1 tickinstead of the actual inclusive end of that day (MaxValue). Consequently the exact-day optimization does not recognizebirthdate=9999-12-31and can classify a non-full-dayMaxValue - 1 tickendpoint as exact. Treat a clamped next day as the end directly rather than subtracting a tick after clamping.
if (end == start.SafeAddDays(1).SafeAddTicks(-1))
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Add regression coverage for extreme future dates in the ap parser path.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.Health.Fhir.Core/Features/Search/Expressions/Parsers/SearchValueExpressionBuilderHelper.cs:106
- The new overflow-safe calls in the
appath are not covered by a regression test: the existingGivenADateWithApComparatorcases stop at year 2220, while the reported failure involves values such asap9500-01-01whose widened start bound exceedsDateTime.MaxValue. Add a parser/unit test that exercises an extreme future date and verifies the expression is built without throwing and the bounds are clamped.
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Description
Fix: Prevent DateTime overflow in search expression rewriters
FHIR search queries with extreme dates (e.g., _lastUpdated=gt9999-12-31, birthdate=ap9500-01-01) caused OverflowException in the SQL rewriting pipeline → 500 response. The overflow came from internal date arithmetic (AddTicks/AddDays) for SQL optimization, not invalid user input.
Changes:
Related issues
Addresses [issue #181592].
Bug 181592: [Edge case] : 500 returned due to ArgumentOutOfRangeException for datetime
Testing
Tested by adding some UTs.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)