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
| case BinaryOperator.GreaterThan: | ||
| return Expression.GreaterThanOrEqual( | ||
| SqlFieldName.ResourceSurrogateId, | ||
| null, | ||
| new DateTimeOffset(truncated.AddTicks(TimeSpan.TicksPerMillisecond)).ToSurrogateId()); | ||
| new DateTimeOffset(truncated.SafeAddTicks(TimeSpan.TicksPerMillisecond)).ToSurrogateId()); |
Replace guard-based overflow/underflow detection with exception-based handling in SafeAddTicks and SafeAddDays for both DateTime and DateTimeOffset. The try-catch approach is more reliable because: - It delegates validation to .NET's trusted implementation - It catches any exception that arithmetic throws (including edge cases) - Guard conditions cannot cover all edge cases comprehensively SafeAddDays still detects multiplication overflow upfront to avoid calculating invalid tick values before passing to SafeAddTicks. DateTimeOffset clamping remains offset-aware to preserve the offset when returning a clamped value. All 28 existing tests pass with this approach. Fixes: PR #5781 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The PR includes a machine-specific SQL Server connection string in launchSettings.json that should be reverted to a repo-friendly default.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
This reverts commit fe822c5.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5781 +/- ##
==========================================
+ Coverage 78.28% 78.95% +0.66%
==========================================
Files 1016 1017 +1
Lines 36940 36974 +34
Branches 5619 5633 +14
==========================================
+ Hits 28920 29193 +273
+ Misses 6644 6400 -244
- Partials 1376 1381 +5 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🔵 Needs a closer look
The _lastUpdated surrogate-id rewrite can change strict comparison semantics at the max representable boundary when clamping occurs, and should explicitly handle the “clamped to max” case.
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/LastUpdatedToResourceSurrogateIdRewriter.cs:60
- When the "next millisecond" calculation overflows (e.g., original value is at/near DateTimeOffset.MaxValue), SafeAddTicks clamps to DateTime.MaxValue. In that case this rewrite still uses GreaterThanOrEqual, which changes strict ">" semantics into ">= max" and could incorrectly match a boundary row if one ever exists at the max surrogate id. Consider detecting the clamp and emitting a strict GreaterThan in the overflow case.
This issue also appears on line 81 of the same file.
src/Microsoft.Health.Fhir.SqlServer/Features/Search/Expressions/Visitors/LastUpdatedToResourceSurrogateIdRewriter.cs:85
- When the "next millisecond" calculation overflows and clamps to DateTime.MaxValue, rewriting "<=" as "< next" becomes "< max", which is no longer equivalent to "<= original" at the representable upper bound (it would exclude a row exactly at max surrogate id). In the clamp case, use LessThanOrEqual against the clamped surrogate id instead of LessThan.
case BinaryOperator.LessThanOrEqual:
return Expression.LessThan(
SqlFieldName.ResourceSurrogateId,
null,
new DateTimeOffset(truncated.SafeAddTicks(TimeSpan.TicksPerMillisecond)).ToSurrogateId());
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped, consistently applied to the known overflow points, and backed by targeted unit tests for key edge cases.
Review details
- Files reviewed: 6/6 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)