fix: read the binary encoding in one call

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "base64-bytes"
-version = "0.1.1"
+version = "0.1.2"
 authors = ["Espresso Systems <hello@espressosys.com>"]
 description = "Binary blobs with intelligent serialization"
 license = "MIT"
@@ -9,9 +9,15 @@ edition = "2021"
 [dependencies]
 base64 = "0.22"
 serde = "1.0"
+serde_bytes = "0.11"
 
 [dev-dependencies]
 bincode = "1.3"
+criterion = { version = "0.5", default-features = false, features = ["cargo_bench_support"] }
 rand = "0.8"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
+
+[[bench]]
+name = "bytes"
+harness = false

README.md

@@ -13,6 +13,22 @@ of an array introduces substantial overhead, and the resulting array of opaque b
 particularly readable anyways.
 
 `base64-bytes` uses the [`is_human_readable`](https://docs.rs/serde/latest/serde/trait.Serializer.html#method.is_human_readable)
-property of a serializer to distinguish these cases. For binary formats, it uses the default
-`Vec<u8>` serialization. For human-readable formats, it uses a much more compact and conventional
-base 64 encoding.
+property of a serializer to distinguish these cases. For binary formats, it emits the blob in a
+single `serialize_bytes` call and reads it back in a single `deserialize_byte_buf` call. For
+human-readable formats, it uses a much more compact and conventional base 64 encoding.
+
+Length-prefixed binary formats such as `bincode` and `postcard` encode this identically to the
+byte-at-a-time `Vec<u8>` serialization, so the wire format is unchanged. Self-describing formats
+that distinguish byte strings from arrays, such as CBOR and MessagePack, now emit a byte string;
+deserialization still accepts either.
+
+## Reading untrusted input
+
+Binary deserialization sizes its buffer from the format's length prefix before reading any bytes,
+so a peer that claims a large length gets a large allocation for free. Readers of untrusted input
+must bound this at the format layer, e.g. `bincode::options().with_limit(n)`, which rejects the
+claim before allocating. Unbounded streaming readers such as `bincode::deserialize_from` over a
+socket do not. Deserializing from an in-memory slice is bounded by the slice itself and is
+unaffected, as is anything already applying a size limit.
+
+This is the same exposure any `String` field already carries under those readers.

benches/bytes.rs

@@ -0,0 +1,111 @@
+//! Compares this crate's encoding against a plain `Vec<u8>` field, which is what it replaces.
+//!
+//! Throughput is reported over the payload length, not the encoded length, so the two formats share
+//! a denominator. JSON therefore moves roughly 1.37x more bytes on the wire than its number says.
+
+use criterion::{
+    criterion_group, criterion_main, measurement::WallTime, BenchmarkGroup, BenchmarkId, Criterion,
+    Throughput,
+};
+use rand::RngCore;
+use serde::{de::DeserializeOwned, Deserialize, Serialize};
+
+#[derive(Deserialize, Serialize)]
+struct Blob {
+    #[serde(with = "base64_bytes")]
+    bytes: Vec<u8>,
+}
+
+#[derive(Deserialize, Serialize)]
+struct Plain {
+    bytes: Vec<u8>,
+}
+
+const KIBI: usize = 1024;
+const MEBI: usize = KIBI * KIBI;
+const SIZES: &[usize] = &[KIBI, 64 * KIBI, MEBI, 4 * MEBI];
+
+fn sample(len: usize) -> Vec<u8> {
+    let mut bytes = vec![0; len];
+    rand::thread_rng().fill_bytes(&mut bytes);
+    bytes
+}
+
+fn label(len: usize) -> String {
+    if len >= MEBI {
+        format!("{}MiB", len / MEBI)
+    } else {
+        format!("{}KiB", len / KIBI)
+    }
+}
+
+fn bench_bincode<T: Serialize + DeserializeOwned>(
+    g: &mut BenchmarkGroup<WallTime>,
+    kind: &str,
+    len: usize,
+    t: &T,
+) {
+    let encoded = bincode::serialize(t).unwrap();
+    g.bench_function(
+        BenchmarkId::new(format!("serialize/{kind}"), label(len)),
+        |b| b.iter(|| bincode::serialize(t).unwrap()),
+    );
+    g.bench_function(
+        BenchmarkId::new(format!("deserialize/{kind}"), label(len)),
+        |b| b.iter(|| bincode::deserialize::<T>(&encoded).unwrap()),
+    );
+}
+
+fn bench_json<T: Serialize + DeserializeOwned>(
+    g: &mut BenchmarkGroup<WallTime>,
+    kind: &str,
+    len: usize,
+    t: &T,
+) {
+    let encoded = serde_json::to_vec(t).unwrap();
+    g.bench_function(
+        BenchmarkId::new(format!("serialize/{kind}"), label(len)),
+        |b| b.iter(|| serde_json::to_vec(t).unwrap()),
+    );
+    g.bench_function(
+        BenchmarkId::new(format!("deserialize/{kind}"), label(len)),
+        |b| b.iter(|| serde_json::from_slice::<T>(&encoded).unwrap()),
+    );
+}
+
+fn bench(c: &mut Criterion) {
+    let mut g = c.benchmark_group("bincode");
+    for &len in SIZES {
+        let bytes = sample(len);
+        g.throughput(Throughput::Bytes(len as u64));
+        bench_bincode(
+            &mut g,
+            "base64-bytes",
+            len,
+            &Blob {
+                bytes: bytes.clone(),
+            },
+        );
+        bench_bincode(&mut g, "vec-u8", len, &Plain { bytes });
+    }
+    g.finish();
+
+    let mut g = c.benchmark_group("json");
+    for &len in SIZES {
+        let bytes = sample(len);
+        g.throughput(Throughput::Bytes(len as u64));
+        bench_json(
+            &mut g,
+            "base64-bytes",
+            len,
+            &Blob {
+                bytes: bytes.clone(),
+            },
+        );
+        bench_json(&mut g, "vec-u8", len, &Plain { bytes });
+    }
+    g.finish();
+}
+
+criterion_group!(benches, bench);
+criterion_main!(benches);

src/lib.rs

@@ -11,9 +11,15 @@
 //! array of opaque bytes isn't particularly readable anyways.
 //!
 //! `base64-bytes` uses the [`is_human_readable`](serde::Serializer::is_human_readable) property of
-//! a serializer to distinguish these cases. For binary formats, it uses the default `Vec<u8>`
-//! serialization. For human-readable formats, it uses a much more compact and conventional base 64
-//! encoding.
+//! a serializer to distinguish these cases. For binary formats, it emits the blob in a single
+//! [`serialize_bytes`](serde::Serializer::serialize_bytes) call and reads it back in a single
+//! [`deserialize_byte_buf`](serde::Deserializer::deserialize_byte_buf) call. For human-readable
+//! formats, it uses a much more compact and conventional base 64 encoding.
+//!
+//! Length-prefixed binary formats such as `bincode` and `postcard` encode this identically to the
+//! byte-at-a-time `Vec<u8>` serialization, so the wire format is unchanged. Self-describing formats
+//! that distinguish byte strings from arrays, such as CBOR and MessagePack, now emit a byte string;
+//! deserialization still accepts either.
 //!
 //! # Usage
 //!
@@ -63,21 +69,30 @@ pub fn serialize<S: Serializer, T: AsRef<[u8]>>(v: &T, s: S) -> Result<S::Ok, S:
 }
 
 /// Deserialize a byte vector.
+///
+/// The binary branch asks the format for the whole blob at once, so the destination buffer is
+/// sized from the format's length prefix before any bytes are read. Readers of untrusted input
+/// must bound that themselves, e.g. `bincode::options().with_limit(n)`; an unbounded
+/// `bincode::deserialize_from` over a socket will allocate whatever length the peer claims.
 pub fn deserialize<'a, D: Deserializer<'a>>(d: D) -> Result<Vec<u8>, D::Error> {
     if d.is_human_readable() {
         Ok(BASE64
             .decode(String::deserialize(d)?)
             .map_err(|err| D::Error::custom(format!("invalid base64: {err}")))?)
     } else {
-        Ok(Vec::deserialize(d)?)
+        serde_bytes::deserialize(d)
     }
 }
 
 #[cfg(test)]
 mod test {
+    use std::io::Cursor;
+
     use crate::BASE64;
     use base64::Engine;
+    use bincode::Options;
     use rand::RngCore;
+    use serde::de::{value::Error as ValueError, Deserializer, Error, Visitor};
     use serde::{Deserialize, Serialize};
 
     #[derive(Debug, PartialEq, Eq, Deserialize, Serialize)]
@@ -125,6 +140,54 @@ mod test {
         }
     }
 
+    /// A deserializer which serves the binary branch only if the blob is requested in one call.
+    ///
+    /// Anything else -- notably serde's default `Vec<u8>` path, which asks for a sequence and then
+    /// visits each byte -- lands in `deserialize_any` and fails.
+    struct WholeBlobOnly<'a> {
+        bytes: &'a [u8],
+    }
+
+    impl<'de> Deserializer<'de> for WholeBlobOnly<'_> {
+        type Error = ValueError;
+
+        fn is_human_readable(&self) -> bool {
+            false
+        }
+
+        fn deserialize_bytes<V: Visitor<'de>>(self, v: V) -> Result<V::Value, Self::Error> {
+            v.visit_bytes(self.bytes)
+        }
+
+        fn deserialize_byte_buf<V: Visitor<'de>>(self, v: V) -> Result<V::Value, Self::Error> {
+            v.visit_byte_buf(self.bytes.to_vec())
+        }
+
+        fn deserialize_any<V: Visitor<'de>>(self, _: V) -> Result<V::Value, Self::Error> {
+            Err(ValueError::custom("blob was not requested in one call"))
+        }
+
+        serde::forward_to_deserialize_any! {
+            bool i8 i16 i32 i64 i128 u8 u16 u32 u64 u128 f32 f64 char str string option unit
+            unit_struct newtype_struct seq tuple tuple_struct map struct enum identifier
+            ignored_any
+        }
+    }
+
+    #[test]
+    fn binary_deserialization_requests_the_blob_in_one_call() {
+        for (bytes, ..) in V0_1_0 {
+            // The per-element path this crate used to take must not be served, or the assertion
+            // below would hold no matter what `crate::deserialize` does.
+            <Vec<u8>>::deserialize(WholeBlobOnly { bytes }).unwrap_err();
+
+            assert_eq!(
+                crate::deserialize(WholeBlobOnly { bytes }).unwrap(),
+                bytes.to_vec()
+            );
+        }
+    }
+
     #[test]
     fn test_bytes_serde() {
         let mut rng = rand::thread_rng();
@@ -150,4 +213,23 @@ mod test {
             assert_eq!(t, serde_json::from_value::<Test>(json).unwrap());
         }
     }
+
+    /// A hostile stream can name a length far larger than what it will deliver, and the binary
+    /// branch sizes its buffer from that length before reading any of it. bincode charges the
+    /// length against its size limit first, so a reader configured with one rejects the claim
+    /// without allocating; unlimited readers do not. The `deserialize` docs point callers here.
+    #[test]
+    fn a_size_limit_rejects_a_hostile_length_before_allocating() {
+        let mut input = (256u64 * 1024 * 1024).to_le_bytes().to_vec();
+        input.extend_from_slice(&[1, 2, 3, 4]);
+
+        let err = bincode::DefaultOptions::new()
+            .with_limit(1024 * 1024)
+            .with_little_endian()
+            .with_fixint_encoding()
+            .deserialize_from::<_, Test>(Cursor::new(&input))
+            .unwrap_err();
+
+        assert!(matches!(*err, bincode::ErrorKind::SizeLimit), "{err}");
+    }
 }