Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DateTime out-of-range panics #1048

Closed
wants to merge 4 commits into from

Conversation

pitdicker
Copy link
Collaborator

@pitdicker pitdicker commented May 6, 2023

If the timestamp of a DateTime is near the end of the range of NaiveDateTime and the offset pushes the timestamp beyond that range, all kinds of methods currently panic. About 40 methods that are part of the public API. With the Debug implementation panicking being the most fun. See #1047.

Most of these methods fall in two categories:

  • They panic on an intermediate value, but should be able to return a meaningful result. Examples are formatting, year(), hour().
  • They can return None. Examples are checked_add_days, with_month.

A somewhat easy fix is to slightly restrict the range of NaiveDate, so that we have some buffer space to represent the out-of-range datetimes. Everything that needs just the intermediate value will be fixed by this. But care should be taken to never let an invalid intermediate value escape to the library user.


This PR grew larger than hoped. I'll describe the various commits to hopefully help review.

Adjust MIN_YEAR and MAX_YEAR

I made MIN_YEAR and MAX_YEAR smaller, so that we have 1 year of buffer space. 1 day would have been enough, but having the minimum date be January 2 and the maximum date December 30 is just strange.

NaiveDate::MIN and NaiveDate::MAX are derived from these. There is a very helpful test_date_bounds to confirm the flags and oridinal are correct.

There is something subtly wrong in the calculation of MIN_DAYS_FROM_YEAR_0, which is only used in tests. It was only correct if MIN_YEAR was a leap year. I changed it to a correct formula that tries to be less smart, but couldn't figure out the problem in the derivation yet. Added a comment.

checked_add_offset and unchecked_add_offset

Instead of panicking in the Add or Sub implementation of NaiveDateTime with FixedOffset, we need a way to be informed of out-of-range result. Or in other cases we need to be able to construct a value in the buffer space for intermediate use.

I added the following methods (not public for now):

  • NaiveTime::overflowing_add_offset and NaiveTime::overflowing_sub_offset
  • NaiveDateTime::checked_add_offset and NaiveDateTime::checked_sub_offset
  • NaiveDateTime::unchecked_add_offset (in a later commit)

The Add and Sub implementations of FixedOffset with NaiveTime, NaiveDateTime and DateTime are reimplemented using of these methods. This fixes a code comment:

this should be implemented more efficiently, but for the time being, this is generic right now.

The best place to put the Add and Sub implementations for DateTime where in the module of DateTime, because there they have access to its private fields. I have moved all implementations to the module of their output type for consistency.

Simplify implementation

Adding an offset works differently from adding a duration. Adding a duration tries to do something smart—but still rudimentary—with leap seconds. Adding an offset should not touch the leap seconds at all. So the methods that operate on NaiveTime should be different.

I extracted the part that operates on the NaiveDate that could be shared into an add_days methods. Previously NaiveDate::checked_add_days would convert the days to a Duration in seconds, and later devide them to get the value in days again. This now works in a less roundabout way. Might also help with the const implementation later.

Fix creating a DateTime with NaiveDateTime::{MIN, MAX}

The creation of a DateTime in TimeZone::from_local_datetime should use checked_sub_offset, and return LocalResult::None on overflow.

The implementation of Local had grown into a mess (sorry, that is the best word for it). With the last commit in #1017 they should just use the provided implementation and pick up this fix.

The actual fixes

The new method DateTime::overflowing_naive_local is not public, and can be used to create an out-of-bounds NaiveDateTime for use as an intermediate value.

Most of the problematic methods on DateTime simply work when converted to use overflowing_naive_local. If they don't return a DateTime, NaiveDateTime or NaiveDate there is also no worry the intermediate value may be exposed outside chrono.

The DateTime::with_* methods that are part of the Datelike trait have an interesting property: if the local NaiveDateTime would be out-of-range, there is only exactly one year/month/day/ordinal they can be set to that would result in a valid DateTime: the one that is already there. This is thanks to the restriction that offset is always less then 24h. To prevent creating an out-of-range NaiveDateTime all these methods short-circuit when possible.

The only two methods on NaiveDate (note: not DateTime) that had to change are checked_add_months and checked_sub_months. Both had a short-circuiting behaviour that with the changes in this PR could return invalid dates. When the input is out of range and months == 0, checked_*_months didn't check whether the result would be valid.

Not fixed: parsing

All the relevant methods on Parsed are public.

Methods in there like Parsed::to_naive_date, Parsed::to_naive_datetime_with_offset and NaiveDateTime::from_timestamp_opt can't be converted to return an intermediate out-of-range value.

I have left it as is. Parsing doesn't panic, it just can't round-trip some DateTime's.

Tests

About 275 lines in this PR are tests, so its size may not be as bad as it looks. @jtmoon79 I tried to behave 😇

test_min_max_datetimes is in my opinion the interesting one, testing all methods that would previously panic.

@pitdicker pitdicker changed the title Datetime min max panics Fix DateTime out-of-range panics May 6, 2023
@pitdicker pitdicker force-pushed the datetime_min_max_panics branch 2 times, most recently from 89029fe to 4074ba9 Compare May 7, 2023 12:36
@pitdicker
Copy link
Collaborator Author

Found another somewhat related panic.

NaiveWeek::last_day creates an intermediate date with NaiveWeek::first_day, and adds 7. For weeks near NaiveDate::MIN the intermediate date could be out of range, while the final value is representable.

This was easily fixable in the implementation of last_day.

let date_min = NaiveDate::MIN;
assert!(date_min.week(Weekday::Mon).last_day() >= date_min);

@pitdicker pitdicker force-pushed the datetime_min_max_panics branch 3 times, most recently from 7b11303 to c02648d Compare May 15, 2023 17:17
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this PR is too large for me to digest. If you have to preface with a bunch of prose in the PR description to make it all make sense, you've already lost me. Please split this in smaller chunks that I can easily review (here's some early feedback from the first couple of commits).

- (MIN_YEAR + 400_000) / 100
+ (MIN_YEAR + 400_000) / 400
- 146_097_000;
const MIN_DAYS_FROM_YEAR_0: i32 =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like this commit to be split in multiple parts that do one thing each. This seems to do refactoring (divide by 1000) which is distracting from the more material change of the value, making it harder to review.

src/naive/date.rs Outdated Show resolved Hide resolved
src/naive/date.rs Outdated Show resolved Hide resolved
@pitdicker
Copy link
Collaborator Author

If you have to preface with a bunch of prose in the PR description to make it all make sense, you've already left.

Haha. That's fair. I'll see what I can do to split it up.
At least this PR shows how it all comes together to avoid the panics 😄.

@pitdicker
Copy link
Collaborator Author

I split out three PRs from this one. It now depends on #1071, which in turn depends on #1069. I'll try to clean up the commits here a bit more, so setting the status to draft for now.

@pitdicker pitdicker force-pushed the datetime_min_max_panics branch 3 times, most recently from b3d7ac0 to 0601202 Compare May 19, 2023 17:46
@pitdicker
Copy link
Collaborator Author

Okay, I have done my best to split this up into smaller commits.
The ones that are not part of #1071 start at "Fix *_DAYS_FROM_YEAR_0 calculation:.

@pitdicker pitdicker marked this pull request as ready for review May 19, 2023 17:48
@pitdicker pitdicker force-pushed the datetime_min_max_panics branch 2 times, most recently from b0109c1 to 86a980f Compare May 23, 2023 12:11
@pitdicker pitdicker force-pushed the datetime_min_max_panics branch 2 times, most recently from 9473718 to 2d317cc Compare June 8, 2023 17:54
@pitdicker pitdicker force-pushed the datetime_min_max_panics branch 3 times, most recently from 99f1679 to 9113d91 Compare August 6, 2023 19:22
@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Merging #1048 (ffee507) into 0.4.x (604b92a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            0.4.x    #1048      +/-   ##
==========================================
+ Coverage   91.50%   91.55%   +0.04%     
==========================================
  Files          38       38              
  Lines       17314    17385      +71     
==========================================
+ Hits        15844    15916      +72     
+ Misses       1470     1469       -1     
Files Coverage Δ
src/datetime/mod.rs 89.35% <100.00%> (+0.06%) ⬆️
src/datetime/tests.rs 96.92% <100.00%> (+0.13%) ⬆️
src/naive/date.rs 96.29% <100.00%> (+0.06%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@djc
Copy link
Member

djc commented Sep 8, 2023

FWIW, would help me to start the PR description out with other PRs that it depends on.

@pitdicker pitdicker force-pushed the datetime_min_max_panics branch 3 times, most recently from 4bb6c09 to bea69bb Compare September 8, 2023 08:15
@pitdicker pitdicker force-pushed the datetime_min_max_panics branch 3 times, most recently from 80de015 to 69c434f Compare September 23, 2023 14:02
@pitdicker pitdicker force-pushed the datetime_min_max_panics branch 2 times, most recently from 40b0150 to 2ab9ff1 Compare September 26, 2023 11:00
@pitdicker pitdicker marked this pull request as draft September 26, 2023 11:00
@pitdicker pitdicker force-pushed the datetime_min_max_panics branch 2 times, most recently from 261c543 to 0dfb0c2 Compare September 26, 2023 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants