Merge pull request #56 from barkanido/handle-ttl-overflow

protect overflow when computing expiration
moka-rs · Dec 18, 2021 · 68e5ce0 · 68e5ce0
2 parents 611d719 + bb6daae
commit 68e5ce0
Show file tree

Hide file tree

Showing 11 changed files with 167 additions and 22 deletions.
diff --git a/README.md b/README.md
@@ -52,6 +52,11 @@ exceeded.
     - Time to live
     - Time to idle
 
+## A note on expiration policies
+The cache builders will panic if configured with either time to live/ time to idle
+higher than 1000 years. This is done to protect against overflow when computing key
+expiration.
+
 
 ## Moka in Production
 
@@ -327,6 +332,7 @@ available on crates.io, such as the [aHash][ahash-crate] crate.
 [ahash-crate]: https://crates.io/crates/ahash
 
 
+
 ## Minimum Supported Rust Versions
 
 This crate's minimum supported Rust versions (MSRV) are the followings:

diff --git a/src/common.rs b/src/common.rs
@@ -8,8 +8,10 @@ pub(crate) mod unsafe_weak_pointer;
 // https://github.com/rust-lang/rust/issues/32976
 // #[cfg_attr(target_has_atomic = "64", path = "common/time_atomic64.rs")]
 
-#[cfg_attr(feature = "atomic64", path = "common/time_atomic64.rs")]
-#[cfg_attr(not(feature = "atomic64"), path = "common/time_compat.rs")]
+#[cfg_attr(feature = "atomic64", path = "common/atomic_time.rs")]
+#[cfg_attr(not(feature = "atomic64"), path = "common/atomic_time_compat.rs")]
+pub(crate) mod atomic_time;
+
 pub(crate) mod time;
 
 use time::Instant;

diff --git a/src/common/time_atomic64.rs → src/common/atomic_time.rs b/src/common/time_atomic64.rs → src/common/atomic_time.rs
@@ -1,10 +1,6 @@
 use std::sync::atomic::{AtomicU64, Ordering};
 
-pub(crate) type Instant = quanta::Instant;
-pub(crate) type Clock = quanta::Clock;
-
-#[cfg(test)]
-pub(crate) type Mock = quanta::Mock;
+use super::time::Instant;
 
 pub(crate) struct AtomicInstant {
     instant: AtomicU64,
@@ -37,6 +33,6 @@ impl AtomicInstant {
     }
 
     pub(crate) fn set_instant(&self, instant: Instant) {
-        self.instant.store(instant.as_u64(), Ordering::Release);
+        self.instant.store(instant.0.as_u64(), Ordering::Release);
     }
 }
diff --git a/src/common/time_compat.rs → src/common/atomic_time_compat.rs b/src/common/time_compat.rs → src/common/atomic_time_compat.rs
@@ -1,10 +1,5 @@
 use parking_lot::RwLock;
-
-pub(crate) type Instant = quanta::Instant;
-pub(crate) type Clock = quanta::Clock;
-
-#[cfg(test)]
-pub(crate) type Mock = quanta::Mock;
+use super::time::Instant;
 
 pub(crate) struct AtomicInstant {
     instant: RwLock<Option<Instant>>,

diff --git a/src/common/time.rs b/src/common/time.rs
@@ -0,0 +1,30 @@
+use std::time::Duration;
+
+pub(crate) type Clock = quanta::Clock;
+#[cfg(test)]
+pub(crate) type Mock = quanta::Mock;
+
+/// a wrapper type over qunta::Instant to force checked additions and prevent
+/// unintentioal overflow. The type preserve the Copy semnatics for the wrapped
+#[derive(PartialEq, PartialOrd, Clone, Copy)]
+pub(crate) struct Instant(pub quanta::Instant);
+
+pub(crate) trait CheckedTimeOps {
+    fn checked_add(&self, duration: Duration)-> Option<Self> where Self: Sized;
+} 
+
+impl Instant {
+    pub(crate) fn new(instant: quanta::Instant) -> Instant {
+        Instant(instant)
+    }
+
+    pub(crate) fn now()-> Instant {
+        Instant(quanta::Instant::now())
+    }
+}
+
+impl CheckedTimeOps for Instant {
+    fn checked_add(&self, duration: Duration) -> Option<Instant> {
+        self.0.checked_add(duration).map(Instant)
+    }
+}
diff --git a/src/future/builder.rs b/src/future/builder.rs
@@ -7,6 +7,8 @@ use std::{
     time::Duration,
 };
 
+const YEAR_SECONDS: u64 = 365 * 24 * 3600;
+
 /// Builds a [`Cache`][cache-struct] with various configuration knobs.
 ///
 /// [cache-struct]: ./struct.Cache.html
@@ -68,6 +70,17 @@ where
     /// Builds a `Cache<K, V>`.
     pub fn build(self) -> Cache<K, V, RandomState> {
         let build_hasher = RandomState::default();
+        self.time_to_live.map(|d| if Duration::from_secs(1_000 * YEAR_SECONDS) < d {
+                panic!("time_to_live is longer than 1000 years");
+            } else {
+                d
+            }
+        );
+        self.time_to_idle.map(|d| if Duration::from_secs(1_000 * YEAR_SECONDS) < d {
+            panic!("time_to_idle is longer than 1000 years");
+        } else {
+            d
+        });
         Cache::with_everything(
             self.max_capacity,
             self.initial_capacity,
@@ -144,6 +157,7 @@ impl<C> CacheBuilder<C> {
 mod tests {
     use super::CacheBuilder;
 
+    use super::Cache;
     use std::time::Duration;
 
     #[tokio::test]
@@ -172,4 +186,22 @@ mod tests {
         cache.insert('a', "Alice").await;
         assert_eq!(cache.get(&'a'), Some("Alice"));
     }
+
+    #[tokio::test]
+    #[should_panic(expected = "time_to_live is longer than 1000 years")]
+    async fn build_cache_too_long_ttl() {
+        let thousand_years_secs: u64 = 1000 * 365 * 24 * 3600;
+        let builder: CacheBuilder<Cache<char, String>> = CacheBuilder::new(100);
+        let duration = Duration::from_secs(thousand_years_secs);
+        builder.time_to_live(duration + Duration::from_secs(1)).build();
+    }
+
+    #[tokio::test]
+    #[should_panic(expected = "time_to_idle is longer than 1000 years")]
+    async fn build_cache_too_long_tti() {
+        let thousand_years_secs: u64 = 1000 * 365 * 24 * 3600;
+        let builder: CacheBuilder<Cache<char, String>> = CacheBuilder::new(100);
+        let duration = Duration::from_secs(thousand_years_secs);
+        builder.time_to_idle(duration + Duration::from_secs(1)).build();
+    }
 }
diff --git a/src/sync.rs b/src/sync.rs
@@ -2,7 +2,8 @@
 
 use crate::common::{
     deque::DeqNode,
-    time::{AtomicInstant, Instant},
+    atomic_time::AtomicInstant,
+    time::Instant,
     AccessTime,
 };
 

diff --git a/src/sync/base_cache.rs b/src/sync/base_cache.rs
@@ -8,7 +8,8 @@ use crate::{
     common::{
         deque::{CacheRegion, DeqNode, Deque},
         frequency_sketch::FrequencySketch,
-        time::{AtomicInstant, Clock, Instant},
+        atomic_time::AtomicInstant,
+        time::{Clock, Instant, CheckedTimeOps},
         AccessTime,
     },
     PredicateError,
@@ -518,11 +519,13 @@ where
     #[inline]
     fn current_time_from_expiration_clock(&self) -> Instant {
         if self.has_expiration_clock.load(Ordering::Relaxed) {
+            Instant::new(
             self.expiration_clock
                 .read()
                 .as_ref()
                 .expect("Cannot get the expiration clock")
                 .now()
+            )
         } else {
             Instant::now()
         }
@@ -1046,7 +1049,11 @@ fn is_expired_entry_ao(
             }
         }
         if let Some(tti) = time_to_idle {
-            return ts + *tti <= now;
+            let checked_add = ts.checked_add(*tti);
+            if checked_add.is_none() {
+                panic!("ttl overflow")
+            }
+            return checked_add.unwrap() <= now;
         }
     }
     false
@@ -1066,7 +1073,11 @@ fn is_expired_entry_wo(
             }
         }
         if let Some(ttl) = time_to_live {
-            return ts + *ttl <= now;
+            let checked_add = ts.checked_add(*ttl);
+            if checked_add.is_none() {
+                panic!("ttl overflow");
+            }
+            return checked_add.unwrap() <= now;
         }
     }
     false

diff --git a/src/sync/builder.rs b/src/sync/builder.rs
@@ -7,6 +7,8 @@ use std::{
     time::Duration,
 };
 
+const YEAR_SECONDS: u64 = 365 * 24 *3600;
+
 /// Builds a [`Cache`][cache-struct] or [`SegmentedCache`][seg-cache-struct]
 /// with various configuration knobs.
 ///
@@ -92,6 +94,17 @@ where
     /// calling this method.
     pub fn build(self) -> Cache<K, V, RandomState> {
         let build_hasher = RandomState::default();
+        self.time_to_live.map(|d| if Duration::from_secs(1_000 * YEAR_SECONDS) < d {
+                panic!("time_to_live is longer than 1000 years");
+            } else {
+                d
+            }
+        );
+        self.time_to_idle.map(|d| if Duration::from_secs(1_000 * YEAR_SECONDS) < d {
+            panic!("time_to_idle is longer than 1000 years");
+        } else {
+            d
+        });
         Cache::with_everything(
             self.max_capacity,
             self.initial_capacity,
@@ -212,6 +225,7 @@ impl<C> CacheBuilder<C> {
 #[cfg(test)]
 mod tests {
     use super::CacheBuilder;
+    use super::Cache;
 
     use std::time::Duration;
 
@@ -269,4 +283,22 @@ mod tests {
         cache.insert('b', "Bob");
         assert_eq!(cache.get(&'b'), Some("Bob"));
     }
+
+    #[tokio::test]
+    #[should_panic(expected = "time_to_live is longer than 1000 years")]
+    async fn build_cache_too_long_ttl() {
+        let thousand_years_secs: u64 = 1000 * 365 * 24 * 3600;
+        let builder: CacheBuilder<Cache<char, String>> = CacheBuilder::new(100);
+        let duration = Duration::from_secs(thousand_years_secs);
+        builder.time_to_live(duration + Duration::from_secs(1)).build();
+    }
+
+    #[tokio::test]
+    #[should_panic(expected = "time_to_idle is longer than 1000 years")]
+    async fn build_cache_too_long_tti() {
+        let thousand_years_secs: u64 = 1000 * 365 * 24 * 3600;
+        let builder: CacheBuilder<Cache<char, String>> = CacheBuilder::new(100);
+        let duration = Duration::from_secs(thousand_years_secs);
+        builder.time_to_idle(duration + Duration::from_secs(1)).build();
+    }
 }
diff --git a/src/unsync/builder.rs b/src/unsync/builder.rs
@@ -7,6 +7,8 @@ use std::{
     time::Duration,
 };
 
+const YEAR_SECONDS: u64 = 365 * 24 * 3600;
+
 /// Builds a [`Cache`][cache-struct] with various configuration knobs.
 ///
 /// [cache-struct]: ./struct.Cache.html
@@ -63,6 +65,17 @@ where
     /// Builds a `Cache<K, V>`.
     pub fn build(self) -> Cache<K, V, RandomState> {
         let build_hasher = RandomState::default();
+        self.time_to_live.map(|d| if Duration::from_secs(1_000 * YEAR_SECONDS) < d {
+                panic!("time_to_live is longer than 1000 years");
+            } else {
+                d
+            }
+        );
+        self.time_to_idle.map(|d| if Duration::from_secs(1_000 * YEAR_SECONDS) < d {
+            panic!("time_to_idle is longer than 1000 years");
+        } else {
+            d
+        });
         Cache::with_everything(
             self.max_capacity,
             self.initial_capacity,
@@ -122,6 +135,7 @@ impl<C> CacheBuilder<C> {
 #[cfg(test)]
 mod tests {
     use super::CacheBuilder;
+    use super::Cache;
 
     use std::time::Duration;
 
@@ -149,4 +163,22 @@ mod tests {
         cache.insert('a', "Alice");
         assert_eq!(cache.get(&'a'), Some(&"Alice"));
     }
+
+    #[tokio::test]
+    #[should_panic(expected = "time_to_live is longer than 1000 years")]
+    async fn build_cache_too_long_ttl() {
+        let thousand_years_secs: u64 = 1000 * 365 * 24 * 3600;
+        let builder: CacheBuilder<Cache<char, String>> = CacheBuilder::new(100);
+        let duration = Duration::from_secs(thousand_years_secs);
+        builder.time_to_live(duration + Duration::from_secs(1)).build();
+    }
+
+    #[tokio::test]
+    #[should_panic(expected = "time_to_idle is longer than 1000 years")]
+    async fn build_cache_too_long_tti() {
+        let thousand_years_secs: u64 = 1000 * 365 * 24 * 3600;
+        let builder: CacheBuilder<Cache<char, String>> = CacheBuilder::new(100);
+        let duration = Duration::from_secs(thousand_years_secs);
+        builder.time_to_idle(duration + Duration::from_secs(1)).build();
+    }
 }
diff --git a/src/unsync/cache.rs b/src/unsync/cache.rs
@@ -2,7 +2,7 @@ use super::{deques::Deques, KeyDate, KeyHashDate, ValueEntry};
 use crate::common::{
     deque::{CacheRegion, DeqNode, Deque},
     frequency_sketch::FrequencySketch,
-    time::{Clock, Instant},
+    time::{Clock, Instant, CheckedTimeOps},
     AccessTime,
 };
 
@@ -342,7 +342,7 @@ where
     #[inline]
     fn current_time_from_expiration_clock(&self) -> Instant {
         if let Some(clock) = &self.expiration_clock {
-            clock.now()
+            Instant::new(clock.now())
         } else {
             Instant::now()
         }
@@ -355,7 +355,11 @@ where
         now: Instant,
     ) -> bool {
         if let (Some(ts), Some(tti)) = (entry.last_accessed(), time_to_idle) {
-            return ts + *tti <= now;
+            let checked_add = ts.checked_add(*tti);
+            if checked_add.is_none() {
+                panic!("ttl overflow")
+            }
+            return checked_add.unwrap() <= now;
         }
         false
     }
@@ -367,7 +371,11 @@ where
         now: Instant,
     ) -> bool {
         if let (Some(ts), Some(ttl)) = (entry.last_modified(), time_to_live) {
-            return ts + *ttl <= now;
+            let checked_add = ts.checked_add(*ttl);
+            if checked_add.is_none() {
+                panic!("ttl overflow")
+            }
+            return checked_add.unwrap() <= now;
         }
         false
     }