Potential overflows detected by Prusti

[djc]

See https://github.com/viperproject/prusti-dev.

[xemwebe]

While looking into this, I stumbled over how to deal with cases, where potential overflow is detected by prust, but considering the actual range of parameters can never happen. E.g. consider DateTime::years_since (the first hit in your list):

    pub fn years_since(&self, base: Self) -> Option<u32> {
        let mut years = self.year() - base.year();
        ...
    }

Since the allowed year range is -262144..=262143, the subtraction can never underflow nor overflow with years as i32. However, this might change if the allow year range should be eventually extended to cover the full range of i32.

[xemwebe]

I have marked all detected code pieces and added some fixes here: https://github.com/xemwebe/chrono.git

[jtmoon79]

I have marked all detected code pieces and added some fixes here: https://github.com/xemwebe/chrono.git

Thanks @xemwebe . Can you post a direct link to the diff that maintainers should look at?

[xemwebe]

@jtmoon79 : sure, here is the link: https://github.com/xemwebe/chrono/compare/main..fix-overflows
Please note that this branch is yet far from completing the issue, but merely a basis for discussions.

[xemwebe]

TimeDelta has methods add, neg, mul and div could all overflow. How would you like to handle these? Mark these functions as deprecated and write new methods with and appending _opt and returning an Option?

Also, these methods

const fn div_mod_floor_64(this: i64, other: i64) -> (i64, i64);
const fn div_floor_64(this: i64, other: i64) -> i64;
const fn mod_floor_64(this: i64, other: i64) -> i64;
const fn div_rem_64(this: i64, other: i64) -> (i64, i64);

seem to duplicate the methods of the Integer trait, except for the const feature. Could these functions be merged to remove duplicates?

There is also the issue, that all cases of diversions a/b or a%b could overflow, if a is the minimal integer (e.g. i32::MIN) and b==-1 since -i32::MIN==i32::MAX+1. At least some of these cases seem to be undetected by prusti.

[xemwebe]

I have updated my findings.

[jtmoon79]

TimeDelta has methods add, neg, mul and div could all overflow. How would you like to handle these? Mark these functions as deprecated and write new methods with and appending _opt and returning an Option?

At first glance, that seems a reasonable way to slowly introduce more safety, and matches the approach done for dates.

I'd wait for @djc and/or @esheppa chime in.

---

Also, these methods ... seem to duplicate the methods of the Integer trait, except for the const feature. Could these functions be merged to remove duplicates?

I know what you're getting at but it would help me if you created a new Draft PR or your own new branch to demonstrate exactly your intent. Could that be done?

---

There is also the issue, that all cases of diversions a/b or a%b could overflow, if a is the minimal integer (e.g. i32::MIN) and b==-1 since -i32::MIN==i32::MAX+1. At least some of these cases seem to be undetected by prusti.

Is this covered by your latest code in src/utils.rs?

    fn safe_mod(self, other: Self) -> Self {

(https://github.com/xemwebe/chrono/compare/main..fix-overflows#diff-23af5356f6001cd3993cd3c801fb7716ea02d1e504081b9fb569332db6107e80R49-R55)

[djc]

So I think the current plan is to redefine TimeDelta as roughly enum TimeDelta { Forward(std::core::Duration), Backward(std::core::Duration) }, and we would probably not define any methods on it. I think @esheppa started some PRs in this direction.

[esheppa]

In the latest PR's I've defined it using Result rather than an entirely new enum, (as that allows using the same principle everywhere, eg with Days, Weeks and Months if desired) - which would prevent us adding any methods anyway

[xemwebe]

Also, these methods ... seem to duplicate the methods of the Integer trait, except for the const feature. Could these functions be merged to remove duplicates?

I know what you're getting at but it would help me if you created a new Draft PR or your own new branch to demonstrate exactly your intent. Could that be done?

Yes, of course, I could try if this would work without problems.

There is also the issue, that all cases of diversions a/b or a%b could overflow, if a is the minimal integer (e.g. i32::MIN) and b==-1 since -i32::MIN==i32::MAX+1. At least some of these cases seem to be undetected by prusti.

Is this covered by your latest code in src/utils.rs?

    fn safe_mod(self, other: Self) -> Self {

(https://github.com/xemwebe/chrono/compare/main..fix-overflows#diff-23af5356f6001cd3993cd3c801fb7716ea02d1e504081b9fb569332db6107e80R49-R55)

Yes, but only for a%b, which is the simpler case, since i32::MIN%(-1)==0, which does not introduce the requirement for introducing Option or Result, other than the case i32::MIN/(-1)==i32::MAX+1.

[xemwebe]

In the latest PR's I've defined it using Result rather than an entirely new enum, (as that allows using the same principle everywhere, eg with Days, Weeks and Months if desired) - which would prevent us adding any methods anyway

Which PR is this?

[xemwebe]

Also, these methods ... seem to duplicate the methods of the Integer trait, except for the const feature. Could these functions be merged to remove duplicates?

I know what you're getting at but it would help me if you created a new Draft PR or your own new branch to demonstrate exactly your intent. Could that be done?

Yes, of course, I could try if this would work without problems.
const fn are not allowed in traits, therefore this functions could not be removed.

[xemwebe]

update after some overflows have been fixed by merge to 0.4.x

This rises a question: What merge policy do you follow with respect to 0.4.x, 0.5.x, and main? Is 0.4.x regularly merged to 0.5-alpha, which will eventually be merged to main?

[xemwebe]

I was trying to prepare a PR for at least those fixes that are not related to TimeDelta (which won't stay at it is) and do not require to break any signature (which would be required for some timestamp methods and the div related method in the Integer trait).
I am a bit confused what would be the best target branch to merge to. 0.5-alpha is kind of out-of-date. Is this branch still active? Would main be the better target (the Integer trait which will be effected, is a recent addition to the main branch)?

[pitdicker]

List of all the potential overflows in this issue and their status:

• DateTime::years_since no overflow
• format::scan::number no overflow
• NaiveWeek::first_day fixed in Fix out-of-range panic in NaiveWeek::last_day #1070
• NaiveDate::from_num_days_from_ce_opt fixed in Fix panic in from_num_days_from_ce_opt #1052
• NaiveDate::from_weekday_of_month_opt no overflow
• NaiveDate::checked_sub_days no overflow
• NaiveDate::succ_opt no overflow
• NaiveDate::pred_opt no overflow
• NaiveDate::years_since no overflow
• NaiveDate::month0 no overflow
• NaiveDate::day0 no overflow
• NaiveDate::ordinal0 no overflow
• NaiveDate::with_month0 fixed in Guard against overflow in NaiveDate::with_*0 methods #1023
• NaiveDate::with_day0 fixed in Guard against overflow in NaiveDate::with_*0 methods #1023
• NaiveDate::with_ordinal0 fixed in Guard against overflow in NaiveDate::with_*0 methods #1023
• NaiveDateTime::timestamp no overflow
• NaiveDateTime::timestamp_millis no overflow
• NaiveDateTime::timestamp_micros no overflow
• NaiveDateTime::timestamp_nanos panic made explicit in Ensure timestamp_nanos panics on overflow in release builds #1123
• NaiveTime::overflowing_sub_signed no overflow
• NaiveTime::overflowing_sub_signed no overflow
• NaiveTime::with_minute no overflow
• NaiveTime::with_second no overflow
• FixedOffset::utc_minus_local no overflow
• impl Sub<FixedOffset> for NaiveTime no overflow
• impl Sub<FixedOffset> for NaiveDateTime no overflow
• impl Sub<FixedOffset> for DateTime<Tz> no overflow
• offset::local::tz_rule::rule::unix_time no overflow
• offset::local::tz_info::saturationg_abs removed in Stop vendoring saturating_abs #1124
• DurationRound::duration_round accidentally fixed by Fix DurationRound panics from issue #1010 #1093
• Duration::num_milliseconds no overflow
• Duration::abs no overflow
• Duration::neg no overflow
• Duration::add no overflow
• Duration::sub no overflow
• Duration::mul
• Duration::div
• div_floor_64 replaced in Remove num-integer dependency #1037
• Datelike::year_ce no overflow
• Datelike::num_seconds_from_midnight no overflow

It is curious that this didn't catch the overflows in #1048 and #1093.

[djc]

Awesome work, thanks.