Fix panic caused by arithmetic overflow

[cardigan1008]

Fix 11. and 12. Arithmetic overflow bug and in #244
Consider using saturating_sub and saturating_mul to avoid the panic.

The problem lies in

hifitime/src/epoch.rs

Line 735 in 822c0a0

let years_since_1900 = year - 1900;

where year_since_1900 is overflowing to i32.

hifitime/src/epoch.rs

Line 736 in 822c0a0

let mut duration_wrt_1900 = Unit::Day * i64::from(365 * years_since_1900);

where 365 * yeaer_since_1900 is overflowing to i32.

I've opted to use saturating_sub and saturating_mul. These functions ensure that there's a maximum boundary, preventing overflow panic.

PS: As a newcomer to the Rust project, I apologize for any inconvenience this may cause.

[codecov]

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 81.70%. Comparing base (822c0a0) to head (5a96875).

Files	Patch %	Lines
src/epoch.rs	71.42%	2 Missing ⚠️
src/errors.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
+ Coverage   81.67%   81.70%   +0.03%     
==========================================
  Files          16       16              
  Lines        3836     3838       +2     
==========================================
+ Hits         3133     3136       +3     
+ Misses        703      702       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I'm proposing another way to fix this overflow bug because hifitime should error not hide these errors.

[ChristopherRabotin]

I'm proposing another way to fix this overflow bug because hifitime should error not hide these errors. Instead, could you apply the following patch to your branch (I wasn't sure if I had push permissions to your branch).

This patch reports an error to prevent an underflow or overflow, instead of hiding the error.

--- a/src/epoch.rs
+++ b/src/epoch.rs
@@ -732,8 +732,13 @@ impl Epoch {
             return Err(Errors::Carry);
         }
 
-        let years_since_1900 = year.saturating_sub(1900);
-        let mut duration_wrt_1900 = Unit::Day * i64::from(years_since_1900.saturating_mul(365));
+        let (years_since_1900, mut duration_wrt_1900) = match year.checked_sub(1900) {
+            None => return Err(Errors::Overflow),
+            Some(years_since_1900) => match years_since_1900.checked_mul(365) {
+                None => return Err(Errors::Overflow),
+                Some(days) => (years_since_1900, Unit::Day * i64::from(days)),
+            },
+        };
 
         // count leap years
         if years_since_1900 > 0 {
diff --git a/src/errors.rs b/src/errors.rs
index 473b05d..b8d9e03 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -76,10 +76,7 @@ impl fmt::Display for Errors {
             Self::ConversionOverlapError(hi, lo) => {
                 write!(f, "hi and lo values overlap: {}, {}", hi, lo)
             }
-            Self::Overflow => write!(
-                f,
-                "overflow occurred when trying to convert Duration information"
-            ),
+            Self::Overflow => write!(f, "prevented overflow/underflow"),
             Self::SystemTimeError => write!(f, "std::time::SystemTime returned an error"),
         }
     }

[cardigan1008]

Thank you for your patch, @ChristopherRabotin ! I've been actively working on automatically fixing Rust panics, and this patch is a result of my efforts using a simple tool. I came across #244 and tried to apply the tool on these panics.
I'm curious about whether the original patch is suitable for addressing a panic, especially considering that I've come across other patches that utilize saturating_sub/mul. Your response would be immensely helpful for me to further my research! However, I understand if this discussion might be disruptive, and I hope it doesn't inconvenience you.

[ChristopherRabotin]

Hi Yunbo,

First off, thanks for contributing, I'm always happy to review and see how people use hifitime and they want to improve on it (even if I'm not always the fastest reviewer as gwbres knows).

In general, I think that the checked_ call works for most applications. That's why I had initially approved the changes before thinking about it some more during the day. In the case of hifitime however, and especially in this case where we try to convert a potentially wrong date, we want to fail early and give an indication of what has happened, and prevent the user from thinking that the date they provided was correct when in fact the library hid the error and uses the minimum valid date for example.

I hope this helps, and again, thanks for your contribution!

[cardigan1008]

Thank you for taking the time to review, and I appreciate your insights. I completely understand your perspective, especially regarding the importance of early failure detection. It's crucial to provide clear feedback to users and prevent any confusion or incorrect assumptions about the validity of their input.

Your feedback is invaluable, and I'll make sure to keep it in mind for future research. Once again, thank you for your support and guidance!

[cardigan1008]

In general, I think that the checked_ call works for most applications. That's why I had initially approved the changes before thinking about it some more during the day.

By the way, I noticed your comment here about the checked_ call. Just to clarify, did you mean to refer to the saturating_ call instead?

[ChristopherRabotin]

In general, I think that the checked_ call works for most applications. That's why I had initially approved the changes before thinking about it some more during the day.

By the way, I noticed your comment here about the checked_ call. Just to clarify, did you mean to refer to the saturating_ call instead?

Yes indeed!

[ChristopherRabotin]

The failed pipelines is something I've started seeing recently. I can look into it this weekend, but probably not before, sorry.