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

Debug impl of DateTime can panic #1047

Closed
pitdicker opened this issue May 4, 2023 · 4 comments
Closed

Debug impl of DateTime can panic #1047

pitdicker opened this issue May 4, 2023 · 4 comments
Labels

Comments

@pitdicker
Copy link
Collaborator

If the timestamp of a DateTime is near the end of the range of NaiveDateTime and the offset pushes the timestamp beyond that range, the Debug implementation can panic.

    #[test]
    fn test_datetime_debug_min_max() {
        let offset = FixedOffset::west_opt(3600).unwrap();
        let local_min = offset.from_utc_datetime(&NaiveDateTime::MIN);
        assert_eq!(format!("{:?}", local_min), "-262144-12-31T23:00:00-01:00");
        /* not yet sure this is the proper string */

        let offset = FixedOffset::east_opt(3600).unwrap();
        let local_max = offset.from_utc_datetime(&NaiveDateTime::MAX);
        assert_eq!(format!("{:?}", local_max), "+262144-01-01T00:59:59.999999999");
        /* not yet sure this is the proper string */
    }

This is extra fun when the panic happens while panicking, where you may want to write the debug value of the DateTime. (i.e. (signal: 6, SIGABRT: process abort signal)).

@pitdicker
Copy link
Collaborator Author

pitdicker commented May 4, 2023

I am trying to figure out what is the least hacky way to prevent panics for DateTimes near the end of the range of NaiveDateTime. Everything that uses DateTime::naive_local can panic:

  • DateTime::naive_local
  • DateTime::date_naive
  • to_rfc2822 edit: doesn't support years with more than 4 digits, so panics anyway.
  • to_rfc3339
  • to_rfc3339_opt
  • format_with_items
  • format
  • format_localized_with_items
  • year, month, month0, day, day0, ordinal, ordinal0, weekday, iso_week
  • hour, minute, second, nanosecond

Methods that return Option should be fixed to handle out-of-range properly:

  • DateTime::checked_add_months and DateTime::checked_sub_months
  • DateTime::checked_add_days and DateTime::checked_sub_days
  • DateTime::map_local (private)
  • with_year, with_month, with_month0, with_day, with_day0, with_ordinal, with_ordinal0
  • with_hour, with_minute, with_second, with_nanosecond

And I suppose there are some issues in parsing.

@pitdicker
Copy link
Collaborator Author

I think a lot of this is fixable.

  • It would be easiest if we can make the range of acceptable values of NaiveDateTime 24 hours smaller on the min and max side, so adding an offset always succeeds. With this we are at least guaranteed to be able to represent an intermediate value when formatting etc.
  • DateTime::naive_local should still panic when the result is outside of that range. Otherwise library users can easily circumvent the range of valid NaiveDateTimes.
  • We should have an internal DateTime::naive_local_unchecked and use it in the methods that can currently panic. It should probably also return a bool while at it to indicate the value was out of range.

Is there something against restricting the range of values in NaiveDateTime a little, and preventing a potential panic in all these methods?

@pitdicker
Copy link
Collaborator Author

Better to make the range a full year smaller. Having the minimum date be January 2 and the maximum date December 30 is just strange.

@pitdicker
Copy link
Collaborator Author

A lot of the work to fix this has already been completed. Groundwork was done in #1310, #1313, #1069 and #1312, and fixes landed in #1070, #1317 and #1071. Only #1333 left for the last five panic cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant