Fix TimeSpan constructor overflow - #134113
Open
asad24-dev wants to merge 2 commits into
Open
asad24-dev wants to merge 2 commits into
asad24-dev wants to merge 2 commits into
Conversation
Accumulate day-based constructor inputs with widened arithmetic before\nrange checking so oversized durations cannot wrap.\n\nAdd regression coverage for the affected overloads.\n\nFix dotnet#134109
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-datetime |
Author
|
@dotnet-policy-service agree |
tarekgh
reviewed
Sep 16, 2026
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Address the moderate performance concern regarding widened multiplication overhead in TimeSpan.cs.
Pull request overview
Fixes overflow in day-based TimeSpan constructors by widening intermediate arithmetic before range validation.
Changes:
- Uses
Int128for intermediate calculations. - Adds regression tests for overflow and wraparound cases.
File summaries
| File | Summary |
|---|---|
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/TimeSpanTests.cs |
Adds constructor overflow regression coverage. |
src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs |
Prevents intermediate arithmetic overflow. |
Review details
Suppressed comments (1)
src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs:298
- Because all constructor inputs are
int, only the day product needs a widened multiplication: evenint.MaxValue * MicrosecondsPerHourfits inlong, and the smaller-unit products are narrower. Calling the full 64-bitMath.BigMulfor each of these four terms adds unnecessary high-half multiplications to every normal constructor call; keep theInt128accumulator but use ordinarylongproducts here (or provide benchmark evidence for the added cost).
+ Math.BigMul(hours, MicrosecondsPerHour)
+ Math.BigMul(minutes, MicrosecondsPerMinute)
+ Math.BigMul(seconds, MicrosecondsPerSecond)
+ Math.BigMul(milliseconds, MicrosecondsPerMillisecond)
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
Report overflow regressions independently and cover valid day-based boundaries.\n\nFix dotnet#134109
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The remaining performance comment is a non-blocking nit.
Review details
Suppressed comments (1)
src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs:298
- Because all four non-day constructor components are
int, each product fits inlong(evenint.MaxValue * MicrosecondsPerHouris belowlong.MaxValue), while theInt128accumulator already prevents overflow in the additions. CallingMath.BigMulfor hours/minutes/seconds/milliseconds therefore performs four unnecessary full-width multiplications on every constructor call (including the 4- and 5-argument overloads, which delegate here). Keep the widened day product, but use ordinarylongproducts for these terms.
+ Math.BigMul(hours, MicrosecondsPerHour)
+ Math.BigMul(minutes, MicrosecondsPerMinute)
+ Math.BigMul(seconds, MicrosecondsPerSecond)
+ Math.BigMul(milliseconds, MicrosecondsPerMillisecond)
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes an overflow in the day-based TimeSpan constructors where intermediate microsecond arithmetic could wrap before the existing range check.
The calculation now uses widened Int128 arithmetic before validating against the supported TimeSpan range and narrowing to long.
Regression coverage has been added for positive and negative overflow, including the reported wraparound case and the affected constructor overloads.
Local validation:
Resolves #134109