From eab7b46f22053756394dd59d59ba19055d9b2394 Mon Sep 17 00:00:00 2001 From: DaughterOfMars Date: Mon, 18 Sep 2023 09:25:58 -0400 Subject: [PATCH] Make BTreeSet impls fail on unordered (#70) * Check ordering for btreeset impls * swap checks and add more tests * Update packable/packable/CHANGELOG.md * duplicate set error occur only on sequential items --------- Co-authored-by: Thibault Martinez --- packable/packable-derive-test/Cargo.toml | 2 +- packable/packable/CHANGELOG.md | 10 ++ packable/packable/Cargo.toml | 2 +- .../packable/src/packable/prefix/btreeset.rs | 30 ++++- packable/packable/src/packable/set.rs | 70 +++++++++- packable/packable/tests/boxed_slice_prefix.rs | 26 +--- packable/packable/tests/btreeset.rs | 38 +++++- packable/packable/tests/btreeset_prefix.rs | 123 ++++++++++++++++-- 8 files changed, 254 insertions(+), 47 deletions(-) diff --git a/packable/packable-derive-test/Cargo.toml b/packable/packable-derive-test/Cargo.toml index e1e6941..aeb43fb 100644 --- a/packable/packable-derive-test/Cargo.toml +++ b/packable/packable-derive-test/Cargo.toml @@ -16,7 +16,7 @@ name = "tests" path = "tests/lib.rs" [dev-dependencies] -packable = { version = "=0.8.2", path = "../packable", default-features = false } +packable = { version = "=0.8.3", path = "../packable", default-features = false } rustversion = { version = "1.0.9", default-features = false } trybuild = { version = "1.0.71", default-features = false, features = [ "diff" ] } diff --git a/packable/packable/CHANGELOG.md b/packable/packable/CHANGELOG.md index 95982c1..1594fa4 100644 --- a/packable/packable/CHANGELOG.md +++ b/packable/packable/CHANGELOG.md @@ -19,6 +19,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security --> +## 0.8.3 - 2023-09-18 + +### Added + +- `UnpackOrderedSetError`; + +### Fixed + +- `BTreeSet` impls and `BTreeSetPrefix` unpack now fails if the data is unordered; + ## 0.8.2 - 2023-09-07 ### Added diff --git a/packable/packable/Cargo.toml b/packable/packable/Cargo.toml index 6831492..003d0e3 100644 --- a/packable/packable/Cargo.toml +++ b/packable/packable/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "packable" -version = "0.8.2" +version = "0.8.3" authors = [ "IOTA Stiftung" ] edition = "2021" description = "A crate for packing and unpacking binary representations." diff --git a/packable/packable/src/packable/prefix/btreeset.rs b/packable/packable/src/packable/prefix/btreeset.rs index 30f0c90..1f75cfa 100644 --- a/packable/packable/src/packable/prefix/btreeset.rs +++ b/packable/packable/src/packable/prefix/btreeset.rs @@ -12,7 +12,10 @@ use core::{ use crate::{ error::UnpackError, - packable::{bounded::Bounded, set::UnpackSetError}, + packable::{ + bounded::Bounded, + set::{UnpackOrderedSetError, UnpackSetError}, + }, packer::Packer, unpacker::Unpacker, Packable, @@ -93,7 +96,7 @@ where >::Error: fmt::Debug, Range: Iterator, { - type UnpackError = UnpackSetError; + type UnpackError = UnpackOrderedSetError; type UnpackVisitor = T::UnpackVisitor; #[inline] @@ -118,15 +121,28 @@ where // The length of any dynamically-sized sequence must be prefixed. let len = B::unpack::<_, VERIFY>(unpacker, &()) - .map_packable_err(Self::UnpackError::Prefix)? + .map_packable_err(UnpackSetError::Prefix) + .map_packable_err(Self::UnpackError::from)? .into(); - let mut set = BTreeSet::new(); + let mut set = BTreeSet::::new(); for _ in B::Bounds::default()..len { - let item = T::unpack::<_, VERIFY>(unpacker, visitor).map_packable_err(Self::UnpackError::Item)?; - if set.contains(&item) { - return Err(UnpackError::Packable(Self::UnpackError::DuplicateItem(item))); + let item = T::unpack::<_, VERIFY>(unpacker, visitor) + .map_packable_err(UnpackSetError::Item) + .map_packable_err(Self::UnpackError::from)?; + if let Some(last) = set.last() { + match last.cmp(&item) { + core::cmp::Ordering::Equal => { + return Err(UnpackError::Packable(Self::UnpackError::Set( + UnpackSetError::DuplicateItem(item), + ))); + } + core::cmp::Ordering::Greater => { + return Err(UnpackError::Packable(Self::UnpackError::Unordered)); + } + core::cmp::Ordering::Less => (), + } } set.insert(item); } diff --git a/packable/packable/src/packable/set.rs b/packable/packable/src/packable/set.rs index 7967eca..7fce443 100644 --- a/packable/packable/src/packable/set.rs +++ b/packable/packable/src/packable/set.rs @@ -52,6 +52,52 @@ impl fmt::Display for UnpackSetError { + /// A set error. + Set(UnpackSetError), + /// An unordered set. + Unordered, +} + +impl From> for UnpackOrderedSetError { + fn from(value: UnpackSetError) -> Self { + Self::Set(value) + } +} + +impl fmt::Debug for UnpackOrderedSetError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Set(arg0) => f.debug_tuple("Set").field(arg0).finish(), + Self::Unordered => f.debug_tuple("Unordered").finish(), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for UnpackOrderedSetError +where + I: std::error::Error, + P: std::error::Error, +{ +} + +impl From for UnpackOrderedSetError { + fn from(err: Infallible) -> Self { + match err {} + } +} + +impl fmt::Display for UnpackOrderedSetError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Set(s) => s.fmt(f), + Self::Unordered => write!(f, "unordered set"), + } + } +} + #[cfg(feature = "usize")] mod btreeset { use alloc::collections::BTreeSet; @@ -60,7 +106,7 @@ mod btreeset { use crate::{error::UnpackError, packer::Packer, unpacker::Unpacker, Packable}; impl Packable for BTreeSet { - type UnpackError = UnpackSetError::UnpackError>; + type UnpackError = UnpackOrderedSetError::UnpackError>; type UnpackVisitor = T::UnpackVisitor; #[inline] @@ -85,14 +131,26 @@ mod btreeset { let len = u64::unpack::<_, VERIFY>(unpacker, &()) .coerce()? .try_into() - .map_err(|err| UnpackError::Packable(Self::UnpackError::Prefix(err)))?; + .map_err(|err| UnpackError::Packable(UnpackSetError::Prefix(err).into()))?; - let mut set = BTreeSet::new(); + let mut set = BTreeSet::::new(); for _ in 0..len { - let item = T::unpack::<_, VERIFY>(unpacker, visitor).map_packable_err(Self::UnpackError::Item)?; - if set.contains(&item) { - return Err(UnpackError::Packable(Self::UnpackError::DuplicateItem(item))); + let item = T::unpack::<_, VERIFY>(unpacker, visitor) + .map_packable_err(UnpackSetError::Item) + .map_packable_err(Self::UnpackError::from)?; + if let Some(last) = set.last() { + match last.cmp(&item) { + core::cmp::Ordering::Equal => { + return Err(UnpackError::Packable(Self::UnpackError::Set( + UnpackSetError::DuplicateItem(item), + ))); + } + core::cmp::Ordering::Greater => { + return Err(UnpackError::Packable(Self::UnpackError::Unordered)); + } + core::cmp::Ordering::Less => (), + } } set.insert(item); } diff --git a/packable/packable/tests/boxed_slice_prefix.rs b/packable/packable/tests/boxed_slice_prefix.rs index 33f983e..d56e120 100644 --- a/packable/packable/tests/boxed_slice_prefix.rs +++ b/packable/packable/tests/boxed_slice_prefix.rs @@ -30,7 +30,7 @@ fn boxed_slice_prefix_from_boxed_slice_truncated_error() { } macro_rules! impl_packable_test_for_boxed_slice_prefix { - ($packable_boxed_slice_prefix:ident, $packable_boxed_slice_prefix_invalid_length:ident, $ty:ty) => { + ($packable_boxed_slice_prefix:ident, $ty:ty) => { #[test] fn $packable_boxed_slice_prefix() { assert_eq!( @@ -85,26 +85,10 @@ macro_rules! impl_packable_test_for_bounded_boxed_slice_prefix { }; } -impl_packable_test_for_boxed_slice_prefix!( - packable_boxed_slice_prefix_u8, - packable_boxed_slice_prefix_invalid_length_u8, - u8 -); -impl_packable_test_for_boxed_slice_prefix!( - packable_boxed_slice_prefix_u16, - packable_boxed_slice_prefix_invalid_length_u16, - u16 -); -impl_packable_test_for_boxed_slice_prefix!( - packable_boxed_slice_prefix_u32, - packable_boxed_slice_prefix_invalid_length_u32, - u32 -); -impl_packable_test_for_boxed_slice_prefix!( - packable_boxed_slice_prefix_u64, - packable_boxed_slice_prefix_invalid_length_u64, - u64 -); +impl_packable_test_for_boxed_slice_prefix!(packable_boxed_slice_prefix_u8, u8); +impl_packable_test_for_boxed_slice_prefix!(packable_boxed_slice_prefix_u16, u16); +impl_packable_test_for_boxed_slice_prefix!(packable_boxed_slice_prefix_u32, u32); +impl_packable_test_for_boxed_slice_prefix!(packable_boxed_slice_prefix_u64, u64); impl_packable_test_for_bounded_boxed_slice_prefix!( packable_boxed_slice_prefix_bounded_u8, diff --git a/packable/packable/tests/btreeset.rs b/packable/packable/tests/btreeset.rs index 4428c06..6ea1534 100644 --- a/packable/packable/tests/btreeset.rs +++ b/packable/packable/tests/btreeset.rs @@ -3,14 +3,50 @@ use std::collections::BTreeSet; +use packable::{ + error::UnpackError, + set::{UnpackOrderedSetError, UnpackSetError}, + PackableExt, +}; + mod common; #[test] fn packable_btreeset() { assert_eq!( - common::generic_test(&BTreeSet::from([Some(0u32), None])).0.len(), + common::generic_test(&BTreeSet::from([None, Some(0u32)])).0.len(), core::mem::size_of::() + (core::mem::size_of::() + core::mem::size_of::()) + core::mem::size_of::() ); } + +#[test] +fn invalid_duplicate() { + let bytes = [1, 2, 3, 3, 4]; + let bytes = Vec::from_iter(bytes.len().to_le_bytes().into_iter().chain(bytes)); + + let prefixed = BTreeSet::::unpack_verified(bytes, &()); + + println!("{prefixed:?}"); + + assert!(matches!( + prefixed, + Err(UnpackError::Packable(UnpackOrderedSetError::Set( + UnpackSetError::DuplicateItem(3u8) + ))), + )); +} + +#[test] +fn invalid_unordered() { + let bytes = [1, 2, 4, 3]; + let bytes = Vec::from_iter(bytes.len().to_le_bytes().into_iter().chain(bytes)); + + let prefixed = BTreeSet::::unpack_verified(bytes, &()); + + assert!(matches!( + prefixed, + Err(UnpackError::Packable(UnpackOrderedSetError::Unordered)), + )); +} diff --git a/packable/packable/tests/btreeset_prefix.rs b/packable/packable/tests/btreeset_prefix.rs index b003a39..9b112c7 100644 --- a/packable/packable/tests/btreeset_prefix.rs +++ b/packable/packable/tests/btreeset_prefix.rs @@ -12,7 +12,7 @@ use packable::{ }, error::UnpackError, prefix::BTreeSetPrefix, - set::UnpackSetError, + set::{UnpackOrderedSetError, UnpackSetError}, PackableExt, }; @@ -33,7 +33,11 @@ fn btreeset_prefix_from_btreeset_truncated_error() { } macro_rules! impl_packable_test_for_btreeset_prefix { - ($packable_btreeset_prefix:ident, $packable_btreeset_prefix_invalid_length:ident, $ty:ty) => { + ( + $packable_btreeset_prefix:ident, + $packable_btreeset_prefix_duplicate:ident, + $packable_btreeset_prefix_unordered:ident, + $ty:ty) => { #[test] fn $packable_btreeset_prefix() { assert_eq!( @@ -47,11 +51,59 @@ macro_rules! impl_packable_test_for_btreeset_prefix { + core::mem::size_of::() ); } + + #[test] + fn $packable_btreeset_prefix_duplicate() { + const LEN: usize = 64; + const LEN_AS_TY: $ty = LEN as $ty; + + let mut bytes = (0u8..LEN as u8).collect::>(); + bytes[LEN - 1] = bytes[LEN - 2]; + let dup = bytes[LEN - 1]; + + let bytes = Vec::from_iter(LEN_AS_TY.to_le_bytes().into_iter().chain(bytes)); + + let prefixed = BTreeSetPrefix::::unpack_verified(bytes, &()); + + assert!(matches!( + prefixed, + Err(UnpackError::Packable(UnpackOrderedSetError::Set( + UnpackSetError::DuplicateItem(d) + ))) if d == dup + )); + } + + #[test] + fn $packable_btreeset_prefix_unordered() { + const LEN: usize = 64; + const LEN_AS_TY: $ty = LEN as $ty; + + let mut bytes = (0u8..LEN as u8).collect::>(); + bytes.swap(0, 1); + + let bytes = Vec::from_iter(LEN_AS_TY.to_le_bytes().into_iter().chain(bytes)); + + let prefixed = BTreeSetPrefix::::unpack_verified(bytes, &()); + + assert!(matches!( + prefixed, + Err(UnpackError::Packable(UnpackOrderedSetError::Unordered)), + )); + } }; } macro_rules! impl_packable_test_for_bounded_btreeset_prefix { - ($packable_btreeset_prefix:ident, $packable_btreeset_prefix_invalid_length:ident, $ty:ty, $bounded:ident, $err:ident, $min:expr, $max:expr) => { + ( + $packable_btreeset_prefix:ident, + $packable_btreeset_prefix_invalid_length:ident, + $packable_btreeset_prefix_duplicate:ident, + $packable_btreeset_prefix_unordered:ident, + $ty:ty, + $bounded:ident, + $err:ident, + $min:expr, + $max:expr) => { #[test] fn $packable_btreeset_prefix() { assert_eq!( @@ -70,17 +122,56 @@ macro_rules! impl_packable_test_for_bounded_btreeset_prefix { #[test] fn $packable_btreeset_prefix_invalid_length() { const LEN: usize = $max + 1; + const LEN_AS_TY: $ty = LEN as $ty; - let mut bytes = [0u8; LEN + 1]; - bytes[0] = LEN as u8; + let bytes = Vec::from_iter(LEN_AS_TY.to_le_bytes().into_iter().chain(core::iter::repeat(0).take(LEN + 1))); let prefixed = BTreeSetPrefix::>::unpack_verified(bytes, &()); + assert!(matches!( + prefixed, + Err(UnpackError::Packable(UnpackOrderedSetError::Set( + UnpackSetError::Prefix($err(LEN_AS_TY)) + ))), + )); + } + + #[test] + fn $packable_btreeset_prefix_duplicate() { + const LEN: usize = $max; + const LEN_AS_TY: $ty = LEN as $ty; + + let mut bytes = (0u8..LEN as u8).collect::>(); + bytes[LEN - 1] = bytes[LEN - 2]; + let dup = bytes[LEN - 1]; + + let bytes = Vec::from_iter(LEN_AS_TY.to_le_bytes().into_iter().chain(bytes)); + + let prefixed = BTreeSetPrefix::>::unpack_verified(bytes, &()); + + assert!(matches!( + prefixed, + Err(UnpackError::Packable(UnpackOrderedSetError::Set( + UnpackSetError::DuplicateItem(d) + ))) if d == dup + )); + } + + #[test] + fn $packable_btreeset_prefix_unordered() { + const LEN: usize = $max; const LEN_AS_TY: $ty = LEN as $ty; + let mut bytes = (0u8..LEN as u8).collect::>(); + bytes.swap(0, 1); + + let bytes = Vec::from_iter(LEN_AS_TY.to_le_bytes().into_iter().chain(bytes)); + + let prefixed = BTreeSetPrefix::>::unpack_verified(bytes, &()); + assert!(matches!( prefixed, - Err(UnpackError::Packable(UnpackSetError::Prefix($err(LEN_AS_TY)))), + Err(UnpackError::Packable(UnpackOrderedSetError::Unordered)), )); } }; @@ -88,28 +179,34 @@ macro_rules! impl_packable_test_for_bounded_btreeset_prefix { impl_packable_test_for_btreeset_prefix!( packable_btreeset_prefix_u8, - packable_btreeset_prefix_invalid_length_u8, + packable_btreeset_prefix_duplicate_u8, + packable_btreeset_prefix_unordered_u8, u8 ); impl_packable_test_for_btreeset_prefix!( packable_btreeset_prefix_u16, - packable_btreeset_prefix_invalid_length_u16, + packable_btreeset_prefix_duplicate_u16, + packable_btreeset_prefix_unordered_u16, u16 ); impl_packable_test_for_btreeset_prefix!( packable_btreeset_prefix_u32, - packable_btreeset_prefix_invalid_length_u32, + packable_btreeset_prefix_duplicate_u32, + packable_btreeset_prefix_unordered_u32, u32 ); impl_packable_test_for_btreeset_prefix!( packable_btreeset_prefix_u64, - packable_btreeset_prefix_invalid_length_u64, + packable_btreeset_prefix_duplicate_u64, + packable_btreeset_prefix_unordered_u64, u64 ); impl_packable_test_for_bounded_btreeset_prefix!( packable_btreeset_prefix_bounded_u8, packable_btreeset_prefix_invalid_length_bounded_u8, + packable_btreeset_prefix_duplicate_bounded_u8, + packable_btreeset_prefix_unordered_bounded_u8, u8, BoundedU8, InvalidBoundedU8, @@ -119,6 +216,8 @@ impl_packable_test_for_bounded_btreeset_prefix!( impl_packable_test_for_bounded_btreeset_prefix!( packable_btreeset_prefix_bounded_u16, packable_btreeset_prefix_invalid_length_bounded_u16, + packable_btreeset_prefix_duplicate_bounded_u16, + packable_btreeset_prefix_unordered_bounded_u16, u16, BoundedU16, InvalidBoundedU16, @@ -128,6 +227,8 @@ impl_packable_test_for_bounded_btreeset_prefix!( impl_packable_test_for_bounded_btreeset_prefix!( packable_btreeset_prefix_bounded_u32, packable_btreeset_prefix_invalid_length_bounded_u32, + packable_btreeset_prefix_duplicate_bounded_u32, + packable_btreeset_prefix_unordered_bounded_u32, u32, BoundedU32, InvalidBoundedU32, @@ -137,6 +238,8 @@ impl_packable_test_for_bounded_btreeset_prefix!( impl_packable_test_for_bounded_btreeset_prefix!( packable_btreeset_prefix_bounded_u64, packable_btreeset_prefix_invalid_length_bounded_u64, + packable_btreeset_prefix_duplicate_bounded_u64, + packable_btreeset_prefix_unordered_bounded_u64, u64, BoundedU64, InvalidBoundedU64,