From c46971af574c4398c01afdbed24862c3c7128edb Mon Sep 17 00:00:00 2001 From: Kamal Ahmad Date: Thu, 5 Jun 2025 17:55:49 +0500 Subject: [PATCH 1/2] Fix serde allowing creation of illegal BoundedVec variants --- Cargo.toml | 1 + src/bounded_vec.rs | 77 ++++++++++++++++++++++++++++------------------ 2 files changed, 48 insertions(+), 30 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 215a8f4..c393b01 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,3 +23,4 @@ arbitrary = ["proptest"] [dev-dependencies] proptest = { version = "1.0.0" } +serde_json = "1.0.140" diff --git a/src/bounded_vec.rs b/src/bounded_vec.rs index 021076d..b0ce069 100644 --- a/src/bounded_vec.rs +++ b/src/bounded_vec.rs @@ -12,7 +12,7 @@ use thiserror::Error; #[derive(PartialEq, Eq, Debug, Clone, Hash, PartialOrd, Ord)] pub struct BoundedVec> { inner: Vec, - _marker: core::marker::PhantomData, + witness: W, } /// BoundedVec errors @@ -41,12 +41,12 @@ pub mod witnesses { // NOTE: we can have proves if needed for some cases like 8/16/32/64 upper bound, so can make memory and serde more compile safe and efficient - /// Compile-time proof of valid bounds. Must be consturcted with same bounds to instantiate `BoundedVec`. - #[derive(Clone, Copy, Debug, PartialEq, Eq)] + /// Compile-time proof of valid bounds. Must be constructed with same bounds to instantiate `BoundedVec`. + #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] pub struct NonEmpty(()); /// Possibly empty vector with upper bound. - #[derive(Clone, Copy, Debug, PartialEq, Eq)] + #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] pub struct Empty(()); /// Type a compile-time proof of valid bounds @@ -88,7 +88,7 @@ impl BoundedVec> { /// BoundedVec::<_, 0, 8, witnesses::Empty<8>>::from_vec(vec![1u8, 2]).unwrap(); /// ``` pub fn from_vec(items: Vec) -> Result { - let _witness = witnesses::empty::(); + let witness = witnesses::empty::(); let len = items.len(); if len > U { Err(BoundedVecOutOfBounds::UpperBoundError { @@ -98,7 +98,7 @@ impl BoundedVec> { } else { Ok(BoundedVec { inner: items, - _marker: core::marker::PhantomData, + witness, }) } } @@ -238,7 +238,7 @@ impl BoundedVec>::from_vec(vec![1u8, 2]).unwrap(); /// ``` pub fn from_vec(items: Vec) -> Result { - let _witness = witnesses::non_empty::(); + let witness = witnesses::non_empty::(); let len = items.len(); if len < L { Err(BoundedVecOutOfBounds::LowerBoundError { @@ -253,7 +253,7 @@ impl BoundedVec BoundedVec>(), - _marker: core::marker::PhantomData, + witness: self.witness, } } @@ -344,7 +344,7 @@ impl BoundedVec>(), - _marker: core::marker::PhantomData, + witness: self.witness, } } @@ -500,7 +500,7 @@ impl From<[T; L]> fn from(arr: [T; L]) -> Self { BoundedVec { inner: arr.into(), - _marker: core::marker::PhantomData, + witness: witnesses::non_empty(), } } } @@ -610,7 +610,7 @@ mod serde_impl { use serde::{Deserialize, Serialize}; // direct impl to unify serde in one place instead of doing attribute on declaration and deserialize here - impl Serialize for BoundedVec { + impl Serialize for BoundedVec { fn serialize(&self, serializer: S) -> Result where S: serde::Serializer, @@ -627,23 +627,17 @@ mod serde_impl { D: serde::Deserializer<'de>, { let inner = Vec::::deserialize(deserializer)?; - if inner.len() < L { - return Err(serde::de::Error::custom(alloc::format!( - "Lower bound violation: got {} (expected >= {})", - inner.len(), - L - ))); - } else if inner.len() > U { - return Err(serde::de::Error::custom(alloc::format!( - "Upper bound violation: got {} (expected <= {})", - inner.len(), - U - ))); - }; - Ok(BoundedVec { - inner, - _marker: core::marker::PhantomData, - }) + BoundedVec::::from_vec(inner).map_err(serde::de::Error::custom) + } + } + + impl<'de, T: Deserialize<'de>, const U: usize> Deserialize<'de> for EmptyBoundedVec { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let inner = Vec::::deserialize(deserializer)?; + EmptyBoundedVec::from_vec(inner).map_err(serde::de::Error::custom) } } @@ -654,7 +648,7 @@ mod serde_impl { use schemars::JsonSchema; // we cannot use attributes, because the do not work with `const`, only numeric literals supported - impl JsonSchema for BoundedVec { + impl JsonSchema for BoundedVec { fn schema_name() -> Cow<'static, str> { alloc::format!("BoundedVec{}Min{}Max{}", T::schema_name(), L, U).into() } @@ -802,6 +796,29 @@ mod tests { } } +#[cfg(feature = "serde")] +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod serde_tests { + use super::*; + use alloc::vec; + #[test] + fn deserialize_nonempty() { + assert_eq!( + serde_json::from_str::>("[1, 2]") + .unwrap() + .as_vec(), + &vec![1, 2] + ); + } + + #[test] + fn deserialize_empty() { + assert!(serde_json::from_str::>("[]").is_err()); + assert!(serde_json::from_str::>("[]").is_ok()); + } +} + #[cfg(feature = "arbitrary")] #[cfg(test)] #[allow(clippy::len_zero)] From bc96cca173e656ed0685ba860cff2555e0e01176 Mon Sep 17 00:00:00 2001 From: Kamal Ahmad Date: Thu, 5 Jun 2025 18:19:20 +0500 Subject: [PATCH 2/2] Run coverage tests with all features enabled --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c32340f..f92511c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -86,7 +86,7 @@ jobs: uses: actions/checkout@v2 - name: Generate code coverage run: | - cargo tarpaulin --avoid-cfg-tarpaulin --timeout=360 --out lcov + cargo tarpaulin --all-features --avoid-cfg-tarpaulin --timeout=360 --out lcov - name: Push code coverage results to coveralls.io uses: coverallsapp/github-action@master with: