Make array logic fall back to the slice logic

Since serde does not support uniform serialization of fixed-sized arrays.
RustCrypto · Aug 2, 2023 · dbd314b · dbd314b
1 parent 66a73b1
commit dbd314b
Show file tree

Hide file tree

Showing 7 changed files with 150 additions and 158 deletions.
diff --git a/serdect/README.md b/serdect/README.md
@@ -29,17 +29,14 @@ While this crate can't ensure that format implementations don't perform
 other kinds of data-dependent branching on the contents of the serialized data,
 using a constant-time hex serialization with human-readable formats should
 help reduce the overall timing variability.
-The table below lists the crates `serdect` is tested against.
-&#x274c; marks the cases for which the serialization is not constant-size for a given data size (due to the format limitations), and therefore will not be constant-time too.
-
-| Crate                                                              | `array`  | `slice`  |
-|--------------------------------------------------------------------|:--------:|:--------:|
-| [`bincode`](https://crates.io/crates/bincode) v1                   | &#x2705; | &#x2705; |
-| [`ciborium`](https://crates.io/crates/ciborium) v0.2               | &#x274c; | &#x2705; |
-| [`rmp-serde`](https://crates.io/crates/rmp-serde) v1               | &#x274c; | &#x2705; |
-| [`serde-json-core`](https://crates.io/crates/serde-json-core) v0.5 | &#x2705; | &#x2705; |
-| [`serde-json`](https://crates.io/crates/serde-json) v1             | &#x2705; | &#x2705; |
-| [`toml`](https://crates.io/crates/toml) v0.7                       | &#x2705; | &#x2705; |
+
+`serdect` is tested against the following crates:
+- [`bincode`](https://crates.io/crates/bincode) v1
+- [`ciborium`](https://crates.io/crates/ciborium) v0.2
+- [`rmp-serde`](https://crates.io/crates/rmp-serde) v1
+- [`serde-json-core`](https://crates.io/crates/serde-json-core) v0.5
+- [`serde-json`](https://crates.io/crates/serde-json) v1
+- [`toml`](https://crates.io/crates/toml) v0.7
 
 
 ## Minimum Supported Rust Version

diff --git a/serdect/src/array.rs b/serdect/src/array.rs
@@ -1,11 +1,20 @@
 //! Serialization primitives for arrays.
 
-use core::fmt;
+// Unfortunately, we currently cannot assert generically that we are serializing
+// a fixed-size byte array.
+// See https://github.com/serde-rs/serde/issues/2120 for the discussion.
+// Therefore we have to fall back to the slice methods,
+// which will add the size information in the binary formats.
+// The only difference is that for the arrays we require the size of the data
+// to be exactly equal to the size of the buffer during deserialization,
+// while for slices the buffer can be larger than the deserialized data.
+
+use core::marker::PhantomData;
 
-use serde::de::{Error, SeqAccess, Visitor};
-use serde::ser::SerializeTuple;
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
 
+use crate::slice;
+
 #[cfg(feature = "zeroize")]
 use zeroize::Zeroize;
 
@@ -16,17 +25,7 @@ where
     S: Serializer,
     T: AsRef<[u8]>,
 {
-    if serializer.is_human_readable() {
-        crate::serialize_hex::<_, _, false>(value, serializer)
-    } else {
-        let mut seq = serializer.serialize_tuple(value.as_ref().len())?;
-
-        for byte in value.as_ref() {
-            seq.serialize_element(byte)?;
-        }
-
-        seq.end()
-    }
+    slice::serialize_hex_lower_or_bin(value, serializer)
 }
 
 /// Serialize the given type as upper case hex when using human-readable
@@ -36,17 +35,7 @@ where
     S: Serializer,
     T: AsRef<[u8]>,
 {
-    if serializer.is_human_readable() {
-        crate::serialize_hex::<_, _, true>(value, serializer)
-    } else {
-        let mut seq = serializer.serialize_tuple(value.as_ref().len())?;
-
-        for byte in value.as_ref() {
-            seq.serialize_element(byte)?;
-        }
-
-        seq.end()
-    }
+    slice::serialize_hex_upper_or_bin(value, serializer)
 }
 
 /// Deserialize from hex when using human-readable formats or binary if the
@@ -57,56 +46,12 @@ where
     D: Deserializer<'de>,
 {
     if deserializer.is_human_readable() {
-        struct StrVisitor<'b>(&'b mut [u8]);
-
-        impl<'de> Visitor<'de> for StrVisitor<'_> {
-            type Value = ();
-
-            fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
-                write!(formatter, "a string of length {}", self.0.len() * 2)
-            }
-
-            fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
-            where
-                E: Error,
-            {
-                if v.len() != self.0.len() * 2 {
-                    return Err(Error::invalid_length(v.len(), &self));
-                }
-
-                base16ct::mixed::decode(v, self.0).map_err(E::custom)?;
-
-                Ok(())
-            }
-        }
-
-        deserializer.deserialize_str(StrVisitor(buffer))
+        deserializer.deserialize_str(slice::StrVisitor::<slice::ExactLength>(buffer, PhantomData))
     } else {
-        struct ArrayVisitor<'b>(&'b mut [u8]);
-
-        impl<'de> Visitor<'de> for ArrayVisitor<'_> {
-            type Value = ();
-
-            fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
-                write!(formatter, "an array of length {}", self.0.len())
-            }
-
-            fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
-            where
-                A: SeqAccess<'de>,
-            {
-                for (index, byte) in self.0.iter_mut().enumerate() {
-                    *byte = match seq.next_element()? {
-                        Some(byte) => byte,
-                        None => return Err(Error::invalid_length(index, &self)),
-                    };
-                }
-
-                Ok(())
-            }
-        }
-
-        deserializer.deserialize_tuple(buffer.len(), ArrayVisitor(buffer))
+        deserializer.deserialize_byte_buf(slice::SliceVisitor::<slice::ExactLength>(
+            buffer,
+            PhantomData,
+        ))
     }
 }
 

diff --git a/serdect/src/lib.rs b/serdect/src/lib.rs
@@ -49,8 +49,17 @@
 //! let data = SecretData([42; 32]);
 //!
 //! let serialized = bincode::serialize(&data).unwrap();
-//! // bincode, a binary serialization format, is serialized into bytes.
-//! assert_eq!(serialized.as_slice(), [42; 32]);
+//! // bincode, a binary serialization format is serialized into bytes.
+//! assert_eq!(
+//!     serialized.as_slice(),
+//!     [
+//!         // Array size.
+//!         32, 0, 0, 0, 0, 0, 0, 0,
+//!         // Actual data.
+//!         42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42,
+//!         42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42,
+//!     ]
+//! );
 //! # let deserialized: SecretData = bincode::deserialize(&serialized).unwrap();
 //! # assert_eq!(deserialized, data);
 //!
@@ -98,7 +107,7 @@
 //! assert_eq!(
 //!     serialized.as_slice(),
 //!     [
-//!         // Not fixed-size, so a size will be encoded.
+//!         // Slice size.
 //!         32, 0, 0, 0, 0, 0, 0, 0,
 //!         // Actual data.
 //!         42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42, 42,

diff --git a/serdect/src/slice.rs b/serdect/src/slice.rs
@@ -1,6 +1,7 @@
 //! Serialization primitives for slices.
 
 use core::fmt;
+use core::marker::PhantomData;
 
 use serde::de::{Error, Visitor};
 use serde::{Deserializer, Serializer};
@@ -42,84 +43,124 @@ where
     }
 }
 
-/// Deserialize from hex when using human-readable formats or binary if the
-/// format is binary. Fails if the `buffer` is smaller then the resulting
-/// slice.
-pub fn deserialize_hex_or_bin<'de, D>(buffer: &mut [u8], deserializer: D) -> Result<&[u8], D::Error>
-where
-    D: Deserializer<'de>,
-{
-    if deserializer.is_human_readable() {
-        struct StrVisitor<'b>(&'b mut [u8]);
+pub(crate) trait LengthCheck {
+    fn length_check(buffer_length: usize, data_length: usize) -> bool;
+    fn expecting(
+        formatter: &mut fmt::Formatter<'_>,
+        data_type: &str,
+        data_length: usize,
+    ) -> fmt::Result;
+}
 
-        impl<'de, 'b> Visitor<'de> for StrVisitor<'b> {
-            type Value = &'b [u8];
+pub(crate) struct ExactLength;
 
-            fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
-                write!(
-                    formatter,
-                    "a string with a maximum length of {}",
-                    self.0.len() * 2
-                )
-            }
+impl LengthCheck for ExactLength {
+    fn length_check(buffer_length: usize, data_length: usize) -> bool {
+        buffer_length == data_length
+    }
+    fn expecting(
+        formatter: &mut fmt::Formatter<'_>,
+        data_type: &str,
+        data_length: usize,
+    ) -> fmt::Result {
+        write!(formatter, "{} of length {}", data_type, data_length)
+    }
+}
 
-            fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
-            where
-                E: Error,
-            {
-                // TODO: Map `base16ct::Error::InvalidLength` to `Error::invalid_length`.
-                base16ct::mixed::decode(v, self.0).map_err(E::custom)
-            }
+struct UpperBound;
+
+impl LengthCheck for UpperBound {
+    fn length_check(buffer_length: usize, data_length: usize) -> bool {
+        buffer_length >= data_length
+    }
+    fn expecting(
+        formatter: &mut fmt::Formatter<'_>,
+        data_type: &str,
+        data_length: usize,
+    ) -> fmt::Result {
+        write!(
+            formatter,
+            "{} with a maximum length of {}",
+            data_type, data_length
+        )
+    }
+}
+
+pub(crate) struct StrVisitor<'b, T: LengthCheck>(pub &'b mut [u8], pub PhantomData<T>);
+
+impl<'de, 'b, T: LengthCheck> Visitor<'de> for StrVisitor<'b, T> {
+    type Value = ();
+
+    fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+        T::expecting(formatter, "a string", self.0.len() * 2)
+    }
+
+    fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
+    where
+        E: Error,
+    {
+        if !T::length_check(self.0.len() * 2, v.len()) {
+            return Err(Error::invalid_length(v.len(), &self));
         }
+        // TODO: Map `base16ct::Error::InvalidLength` to `Error::invalid_length`.
+        base16ct::mixed::decode(v, self.0)
+            .map(|_| ())
+            .map_err(E::custom)
+    }
+}
 
-        deserializer.deserialize_str(StrVisitor(buffer))
-    } else {
-        struct SliceVisitor<'b>(&'b mut [u8]);
+pub(crate) struct SliceVisitor<'b, T: LengthCheck>(pub &'b mut [u8], pub PhantomData<T>);
 
-        impl<'de, 'b> Visitor<'de> for SliceVisitor<'b> {
-            type Value = &'b [u8];
+impl<'de, 'b, T: LengthCheck> Visitor<'de> for SliceVisitor<'b, T> {
+    type Value = ();
 
-            fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
-                write!(
-                    formatter,
-                    "a slice with a maximum length of {}",
-                    self.0.len()
-                )
-            }
+    fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
+        T::expecting(formatter, "an array", self.0.len())
+    }
 
-            fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
-            where
-                E: Error,
-            {
-                // Workaround for
-                // https://github.com/rust-lang/rfcs/blob/b1de05846d9bc5591d753f611ab8ee84a01fa500/text/2094-nll.md#problem-case-3-conditional-control-flow-across-functions
-                if v.len() <= self.0.len() {
-                    let buffer = &mut self.0[..v.len()];
-                    buffer.copy_from_slice(v);
-                    return Ok(buffer);
-                }
-
-                Err(E::invalid_length(v.len(), &self))
-            }
+    fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
+    where
+        E: Error,
+    {
+        // Workaround for
+        // https://github.com/rust-lang/rfcs/blob/b1de05846d9bc5591d753f611ab8ee84a01fa500/text/2094-nll.md#problem-case-3-conditional-control-flow-across-functions
+        if T::length_check(self.0.len(), v.len()) {
+            let buffer = &mut self.0[..v.len()];
+            buffer.copy_from_slice(v);
+            return Ok(());
+        }
 
-            #[cfg(feature = "alloc")]
-            fn visit_byte_buf<E>(self, mut v: Vec<u8>) -> Result<Self::Value, E>
-            where
-                E: Error,
-            {
-                // Workaround for
-                // https://github.com/rust-lang/rfcs/blob/b1de05846d9bc5591d753f611ab8ee84a01fa500/text/2094-nll.md#problem-case-3-conditional-control-flow-across-functions
-                if v.len() <= self.0.len() {
-                    let buffer = &mut self.0[..v.len()];
-                    buffer.swap_with_slice(&mut v);
-                    return Ok(buffer);
-                }
-
-                Err(E::invalid_length(v.len(), &self))
-            }
+        Err(E::invalid_length(v.len(), &self))
+    }
+
+    #[cfg(feature = "alloc")]
+    fn visit_byte_buf<E>(self, mut v: Vec<u8>) -> Result<Self::Value, E>
+    where
+        E: Error,
+    {
+        // Workaround for
+        // https://github.com/rust-lang/rfcs/blob/b1de05846d9bc5591d753f611ab8ee84a01fa500/text/2094-nll.md#problem-case-3-conditional-control-flow-across-functions
+        if T::length_check(self.0.len(), v.len()) {
+            let buffer = &mut self.0[..v.len()];
+            buffer.swap_with_slice(&mut v);
+            return Ok(());
         }
 
-        deserializer.deserialize_byte_buf(SliceVisitor(buffer))
+        Err(E::invalid_length(v.len(), &self))
+    }
+}
+
+/// Deserialize from hex when using human-readable formats or binary if the
+/// format is binary. Fails if the `buffer` is smaller then the resulting
+/// slice.
+pub fn deserialize_hex_or_bin<'de, D>(buffer: &mut [u8], deserializer: D) -> Result<(), D::Error>
+where
+    D: Deserializer<'de>,
+{
+    if deserializer.is_human_readable() {
+        deserializer.deserialize_str(StrVisitor::<UpperBound>(buffer, PhantomData))
+    } else {
+        deserializer.deserialize_byte_buf(SliceVisitor::<UpperBound>(buffer, PhantomData))
     }
 }
 

diff --git a/serdect/tests/bincode.rs b/serdect/tests/bincode.rs
@@ -14,7 +14,7 @@ const EXAMPLE_BYTES: [u8; 16] = hex!("000102030405060708090A0B0C0D0EFF");
 const BINCODE_SLICE: [u8; 24] = hex!("1000000000000000000102030405060708090A0B0C0D0EFF");
 
 /// bincode serialization of [`EXAMPLE_BYTES`] as an array.
-const BINCODE_ARRAY: [u8; 16] = EXAMPLE_BYTES;
+const BINCODE_ARRAY: [u8; 24] = BINCODE_SLICE;
 
 #[test]
 fn deserialize_slice() {

diff --git a/serdect/tests/cbor.rs b/serdect/tests/cbor.rs
@@ -19,7 +19,7 @@ const CBOR_SLICE: [u8; 17] = hex!("50000102030405060708090A0B0C0D0EFF");
 /// CBOR serialization of [`EXAMPLE_BYTES`] as an array.
 /// (first three bits, `0b100` denote an array, and the last five, `0b10000` denote the length)
 /// Note the 0x18 marker before 0xFF, denoting that the integers are dynamically sized.
-const CBOR_ARRAY: [u8; 18] = hex!("90000102030405060708090A0B0C0D0E18FF");
+const CBOR_ARRAY: [u8; 17] = CBOR_SLICE;
 
 #[test]
 fn deserialize_slice() {