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 <thibault.martinez.30@gmail.com>

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" ] }

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

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."

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
     <B as TryFrom<usize>>::Error: fmt::Debug,
     Range<B::Bounds>: Iterator<Item = B::Bounds>,
 {
-    type UnpackError = UnpackSetError<T, T::UnpackError, B::UnpackError>;
+    type UnpackError = UnpackOrderedSetError<T, T::UnpackError, B::UnpackError>;
     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::<T>::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);
         }

packable/packable/src/packable/set.rs

@@ -52,6 +52,52 @@ impl<T, I: fmt::Display, P: fmt::Display> fmt::Display for UnpackSetError<T, I,
     }
 }
 
+/// Error type raised when a semantic error occurs while unpacking an ordered set.
+pub enum UnpackOrderedSetError<T, I, P> {
+    /// A set error.
+    Set(UnpackSetError<T, I, P>),
+    /// An unordered set.
+    Unordered,
+}
+
+impl<T, I, P> From<UnpackSetError<T, I, P>> for UnpackOrderedSetError<T, I, P> {
+    fn from(value: UnpackSetError<T, I, P>) -> Self {
+        Self::Set(value)
+    }
+}
+
+impl<T, I: fmt::Debug, P: fmt::Debug> fmt::Debug for UnpackOrderedSetError<T, I, P> {
+    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<T, I, P> std::error::Error for UnpackOrderedSetError<T, I, P>
+where
+    I: std::error::Error,
+    P: std::error::Error,
+{
+}
+
+impl<T, I, P> From<Infallible> for UnpackOrderedSetError<T, I, P> {
+    fn from(err: Infallible) -> Self {
+        match err {}
+    }
+}
+
+impl<T, I: fmt::Display, P: fmt::Display> fmt::Display for UnpackOrderedSetError<T, I, P> {
+    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<T: Packable + Ord> Packable for BTreeSet<T> {
-        type UnpackError = UnpackSetError<T, T::UnpackError, <usize as Packable>::UnpackError>;
+        type UnpackError = UnpackOrderedSetError<T, T::UnpackError, <usize as Packable>::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::<T>::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);
             }

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,

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::<u64>()
             + (core::mem::size_of::<u8>() + core::mem::size_of::<u32>())
             + core::mem::size_of::<u8>()
     );
 }
+
+#[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::<u8>::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::<u8>::unpack_verified(bytes, &());
+
+    assert!(matches!(
+        prefixed,
+        Err(UnpackError::Packable(UnpackOrderedSetError::Unordered)),
+    ));
+}