From 70d2e1a0db8defbb70aabc038189b59524c11d01 Mon Sep 17 00:00:00 2001 From: cloudhead Date: Wed, 24 Apr 2024 18:07:45 +0200 Subject: [PATCH] node: Improve `Timestamp` conversion safety Remove `Timestamp::from(u64)` instance, since not all `u64`s are valid Timestamps, and add `try_from` instead. --- radicle-node/src/service/gossip/store.rs | 2 +- radicle-node/src/service/message.rs | 2 +- radicle-node/src/wire.rs | 5 ++- radicle/src/node/routing.rs | 17 ++++++++--- radicle/src/node/timestamp.rs | 39 +++++++++++++++++++----- radicle/src/test/arbitrary.rs | 2 +- 6 files changed, 51 insertions(+), 16 deletions(-) diff --git a/radicle-node/src/service/gossip/store.rs b/radicle-node/src/service/gossip/store.rs index bdeea17c..cd06960b 100644 --- a/radicle-node/src/service/gossip/store.rs +++ b/radicle-node/src/service/gossip/store.rs @@ -71,7 +71,7 @@ impl Store for Database { if let Some(Ok(row)) = stmt.into_iter().next() { return match row.try_read::, _>(0)? { - Some(i) => Ok(Some(Timestamp::from(u64::try_from(i)?))), + Some(i) => Ok(Some(Timestamp::try_from(i)?)), None => Ok(None), }; } diff --git a/radicle-node/src/service/message.rs b/radicle-node/src/service/message.rs index 7c579c84..b18208b1 100644 --- a/radicle-node/src/service/message.rs +++ b/radicle-node/src/service/message.rs @@ -669,7 +669,7 @@ mod tests { fn test_node_announcement_validate() { let ann = NodeAnnouncement { features: node::Features::SEED, - timestamp: Timestamp::from(42491841), + timestamp: Timestamp::try_from(42491841u64).unwrap(), alias: Alias::new("alice"), addresses: BoundedVec::new(), nonce: 0, diff --git a/radicle-node/src/wire.rs b/radicle-node/src/wire.rs index 2edcaa5a..d7e19588 100644 --- a/radicle-node/src/wire.rs +++ b/radicle-node/src/wire.rs @@ -60,6 +60,8 @@ pub enum Error { InvalidProtocolVersion([u8; 4]), #[error("invalid onion address: {0}")] InvalidOnionAddr(#[from] tor::OnionAddrDecodeError), + #[error("invalid timestamp: {0}")] + InvalidTimestamp(u64), #[error("unknown address type `{0}`")] UnknownAddressType(u8), #[error("unknown message type `{0}`")] @@ -537,8 +539,9 @@ impl Encode for Timestamp { impl Decode for Timestamp { fn decode(reader: &mut R) -> Result { let millis = u64::decode(reader)?; + let ts = Timestamp::try_from(millis).map_err(Error::InvalidTimestamp)?; - Ok(Timestamp::from(millis)) + Ok(ts) } } diff --git a/radicle/src/node/routing.rs b/radicle/src/node/routing.rs index f1134a23..001df758 100644 --- a/radicle/src/node/routing.rs +++ b/radicle/src/node/routing.rs @@ -347,10 +347,14 @@ mod test { vec![(id, InsertResult::SeedAdded)] ); assert_eq!( - db.insert([&id], node, Timestamp::from(1)).unwrap(), + db.insert([&id], node, Timestamp::try_from(1u64).unwrap()) + .unwrap(), vec![(id, InsertResult::TimeUpdated)] ); - assert_eq!(db.entry(&id, &node).unwrap(), Some(Timestamp::from(1))); + assert_eq!( + db.entry(&id, &node).unwrap(), + Some(Timestamp::try_from(1u64).unwrap()) + ); } #[test] @@ -372,7 +376,8 @@ mod test { ] ); assert_eq!( - db.insert([&id1, &id2], node, Timestamp::from(1)).unwrap(), + db.insert([&id1, &id2], node, Timestamp::try_from(1u64).unwrap()) + .unwrap(), vec![ (id1, InsertResult::TimeUpdated), (id2, InsertResult::TimeUpdated) @@ -415,7 +420,8 @@ mod test { for node in &nodes { let time = rng.u64(..now.as_millis()); - db.insert(&ids, *node, Timestamp::from(time)).unwrap(); + db.insert(&ids, *node, Timestamp::try_from(time).unwrap()) + .unwrap(); } let ids = arbitrary::vec::(10); @@ -423,7 +429,8 @@ mod test { for node in &nodes { let time = rng.u64(now.as_millis()..i64::MAX as u64); - db.insert(&ids, *node, Timestamp::from(time)).unwrap(); + db.insert(&ids, *node, Timestamp::try_from(time).unwrap()) + .unwrap(); } let pruned = db.prune(now.into(), None).unwrap(); diff --git a/radicle/src/node/timestamp.rs b/radicle/src/node/timestamp.rs index d05a7360..daaeef53 100644 --- a/radicle/src/node/timestamp.rs +++ b/radicle/src/node/timestamp.rs @@ -1,5 +1,6 @@ use std::{ fmt, + num::TryFromIntError, ops::{Add, Deref, Sub}, }; @@ -15,7 +16,7 @@ impl Add for Timestamp { type Output = Timestamp; fn add(self, millis: u64) -> Self::Output { - Self(self.0 + millis) + Self(self.0.saturating_add(millis)) } } @@ -23,7 +24,7 @@ impl Sub for Timestamp { type Output = Timestamp; fn sub(self, millis: u64) -> Self::Output { - Self(self.0 - millis) + Self(self.0.saturating_sub(millis)) } } @@ -69,9 +70,23 @@ impl From for LocalTime { } } -impl From for Timestamp { - fn from(u: u64) -> Self { - Self(u) +impl TryFrom for Timestamp { + type Error = u64; + + fn try_from(u: u64) -> Result { + if u <= *Self::MAX { + Ok(Self(u)) + } else { + Err(u) + } + } +} + +impl TryFrom for Timestamp { + type Error = TryFromIntError; + + fn try_from(i: i64) -> Result { + i.try_into().map(Self) } } @@ -84,7 +99,7 @@ impl TryFrom<&sql::Value> for Timestamp { Ok(u) => Ok(Timestamp(u)), Err(e) => Err(sql::Error { code: None, - message: Some(format!("sql: invalid integer for timestamp: {e}")), + message: Some(format!("sql: invalid integer `{i}` for timestamp: {e}")), }), }, _ => Err(sql::Error { @@ -101,8 +116,18 @@ impl sql::BindableWithIndex for &Timestamp { Ok(integer) => integer.bind(stmt, i), Err(e) => Err(sql::Error { code: None, - message: Some(format!("sql: invalid timestamp: {e}")), + message: Some(format!("sql: invalid timestamp `{self}`: {e}")), }), } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_timestamp_max() { + assert_eq!(i64::try_from(*Timestamp::MAX), Ok(i64::MAX)); + } +} diff --git a/radicle/src/test/arbitrary.rs b/radicle/src/test/arbitrary.rs index 2ffe28d3..dbab87c7 100644 --- a/radicle/src/test/arbitrary.rs +++ b/radicle/src/test/arbitrary.rs @@ -315,6 +315,6 @@ impl Arbitrary for Alias { impl Arbitrary for Timestamp { fn arbitrary(g: &mut qcheck::Gen) -> Self { - Self::from(u64::arbitrary(g)) + Self::try_from(u64::arbitrary(g).min(*Self::MAX)).unwrap() } }