chrono icon indicating copy to clipboard operation
chrono copied to clipboard

`Debug` impl of `DateTime` can panic

Open pitdicker opened this issue 2 years ago • 4 comments
trafficstars

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 avatar May 04 '23 07:05 pitdicker

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 avatar May 04 '23 10:05 pitdicker

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 avatar May 04 '23 10:05 pitdicker

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 avatar May 04 '23 13:05 pitdicker

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.

pitdicker avatar Feb 02 '24 12:02 pitdicker