From 2b676743b415cfed015190693cd4469d22f0d297 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 15 Apr 2021 20:02:35 +1000 Subject: [PATCH 1/5] Add functions for serializing and deserializing split arrays In Transaction::V5, Zcash splits some types into multiple arrays, with a single prefix count before the first array. Add utility functions for serializing and deserializing the subsequent arrays, with a paramater for the original array's length. --- zebra-chain/src/serialization.rs | 9 +- .../src/serialization/zcash_deserialize.rs | 95 ++++++++++++++++--- .../src/serialization/zcash_serialize.rs | 46 ++++++++- 3 files changed, 130 insertions(+), 20 deletions(-) diff --git a/zebra-chain/src/serialization.rs b/zebra-chain/src/serialization.rs index 61031fb2a81..10bb913af70 100644 --- a/zebra-chain/src/serialization.rs +++ b/zebra-chain/src/serialization.rs @@ -19,8 +19,13 @@ pub mod sha256d; pub use error::SerializationError; pub use read_zcash::ReadZcashExt; pub use write_zcash::WriteZcashExt; -pub use zcash_deserialize::{TrustedPreallocate, ZcashDeserialize, ZcashDeserializeInto}; -pub use zcash_serialize::{ZcashSerialize, MAX_PROTOCOL_MESSAGE_LEN}; +pub use zcash_deserialize::{ + zcash_deserialize_bytes_external_count, zcash_deserialize_external_count, TrustedPreallocate, + ZcashDeserialize, ZcashDeserializeInto, +}; +pub use zcash_serialize::{ + zcash_serialize_external_count, ZcashSerialize, MAX_PROTOCOL_MESSAGE_LEN, +}; #[cfg(test)] mod proptests; diff --git a/zebra-chain/src/serialization/zcash_deserialize.rs b/zebra-chain/src/serialization/zcash_deserialize.rs index 70cfb8c99b7..5885be08c66 100644 --- a/zebra-chain/src/serialization/zcash_deserialize.rs +++ b/zebra-chain/src/serialization/zcash_deserialize.rs @@ -1,7 +1,9 @@ -use std::{convert::TryInto, io}; +use std::{ + convert::{TryFrom, TryInto}, + io, +}; use super::{ReadZcashExt, SerializationError, MAX_PROTOCOL_MESSAGE_LEN}; -use byteorder::ReadBytesExt; /// Consensus-critical serialization for Zcash. /// @@ -18,27 +20,92 @@ pub trait ZcashDeserialize: Sized { fn zcash_deserialize(reader: R) -> Result; } +/// Deserialize a `Vec`, where the number of items is set by a compactsize +/// prefix in the data. This is the most common format in Zcash. +/// +/// See `zcash_deserialize_external_count` for more details, and usage +/// information. impl ZcashDeserialize for Vec { fn zcash_deserialize(mut reader: R) -> Result { - let len = reader.read_compactsize()?; - if len > T::max_allocation() { + let len = reader.read_compactsize()?.try_into()?; + zcash_deserialize_external_count(len, reader) + } +} + +/// Implement ZcashDeserialize for Vec directly instead of using the blanket Vec implementation +/// +/// This allows us to optimize the inner loop into a single call to `read_exact()` +/// Note that we don't implement TrustedPreallocate for u8. +/// This allows the optimization without relying on specialization. +impl ZcashDeserialize for Vec { + fn zcash_deserialize(mut reader: R) -> Result { + let len = reader.read_compactsize()?.try_into()?; + zcash_deserialize_bytes_external_count(len, reader) + } +} + +/// Deserialize a `Vec` containing `external_count` items. +/// +/// In Zcash, most arrays are stored as a compactsize, followed by that number +/// of items of type `T`. But in `Transaction::V5`, some types are serialized as +/// multiple arrays in different locations, with a single compactsize before the +/// first array. +/// +/// ## Usage +/// +/// Use `zcash_deserialize_external_count` when the array count is determined by +/// other data, or a consensus rule. +/// +/// Use `Vec::zcash_deserialize` for data that contains compactsize count, +/// followed by the data array. +/// +/// For example, when a single count applies to multiple arrays: +/// 1. Use `Vec::zcash_deserialize` for the array that has a data count. +/// 2. Use `zcash_deserialize_external_count` for the arrays with no count in the +/// data, passing the length of the first array. +/// +/// This function has a `zcash_` prefix to alert the reader that the +/// serialization in use is consensus-critical serialization, rather than +/// some other kind of serialization. +pub fn zcash_deserialize_external_count( + external_count: usize, + mut reader: R, +) -> Result, SerializationError> { + match u64::try_from(external_count) { + Ok(external_count) if external_count > T::max_allocation() => { return Err(SerializationError::Parse( "Vector longer than max_allocation", - )); - } - let mut vec = Vec::with_capacity(len.try_into()?); - for _ in 0..len { - vec.push(T::zcash_deserialize(&mut reader)?); + )) } - Ok(vec) + Ok(_) => {} + Err(_) => return Err(SerializationError::Parse("Vector longer than u64::MAX")), + } + let mut vec = Vec::with_capacity(external_count); + for _ in 0..external_count { + vec.push(T::zcash_deserialize(&mut reader)?); } + Ok(vec) } -/// Read a byte. -impl ZcashDeserialize for u8 { - fn zcash_deserialize(mut reader: R) -> Result { - Ok(reader.read_u8()?) +/// `zcash_deserialize_external_count`, specialised for raw bytes. +/// +/// This allows us to optimize the inner loop into a single call to `read_exact()`. +/// +/// This function has a `zcash_` prefix to alert the reader that the +/// serialization in use is consensus-critical serialization, rather than +/// some other kind of serialization. +pub fn zcash_deserialize_bytes_external_count( + external_count: usize, + mut reader: R, +) -> Result, SerializationError> { + if external_count > MAX_U8_ALLOCATION { + return Err(SerializationError::Parse( + "Byte vector longer than MAX_U8_ALLOCATION", + )); } + let mut vec = vec![0u8; external_count]; + reader.read_exact(&mut vec)?; + Ok(vec) } /// Read a Bitcoin-encoded UTF-8 string. diff --git a/zebra-chain/src/serialization/zcash_serialize.rs b/zebra-chain/src/serialization/zcash_serialize.rs index f1219841147..14717158f33 100644 --- a/zebra-chain/src/serialization/zcash_serialize.rs +++ b/zebra-chain/src/serialization/zcash_serialize.rs @@ -29,14 +29,52 @@ pub trait ZcashSerialize: Sized { } } +/// Serialize a `Vec` as a compactsize number of items, then the items. This is +/// the most common format in Zcash. +/// +/// See `zcash_serialize_external_count` for more details, and usage information. impl ZcashSerialize for Vec { fn zcash_serialize(&self, mut writer: W) -> Result<(), io::Error> { writer.write_compactsize(self.len() as u64)?; - for x in self { - x.zcash_serialize(&mut writer)?; - } - Ok(()) + zcash_serialize_external_count(self, writer) + } +} + +/// Serialize a `Vec` **without** writing the number of items as a compactsize. +/// +/// In Zcash, most arrays are stored as a compactsize, followed by that number +/// of items of type `T`. But in `Transaction::V5`, some types are serialized as +/// multiple arrays in different locations, with a single compactsize before the +/// first array. +/// +/// ## Usage +/// +/// Use `zcash_serialize_external_count` when the array count is determined by +/// other data, or a consensus rule. (For raw byte vectors and slices, use +/// `writer.write_all(&vec)`.) +/// +/// Use `Vec::zcash_serialize` for data that contains compactsize count, +/// followed by the data array. +/// +/// For example, when a single count applies to multiple arrays: +/// 1. Use `Vec::zcash_serialize` for the array that has a data count. +/// 2. Use `zcash_serialize_external_count` for the arrays with no count in the +/// data, passing the length of the first array. +/// +/// This function has a `zcash_` prefix to alert the reader that the +/// serialization in use is consensus-critical serialization, rather than +/// some other kind of serialization. +// +// we specifically want to serialize `Vec`s here, rather than generic slices +#[allow(clippy::ptr_arg)] +pub fn zcash_serialize_external_count( + vec: &Vec, + mut writer: W, +) -> Result<(), io::Error> { + for x in vec { + x.zcash_serialize(&mut writer)?; } + Ok(()) } /// The maximum length of a Zcash message, in bytes. From 114fb996c2e4b3c612ec98b4f87d908dc65de5a0 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 15 Apr 2021 20:05:09 +1000 Subject: [PATCH 2/5] Use zcash_deserialize_bytes_external_count in zebra-network --- zebra-network/src/protocol/external.rs | 3 ++- zebra-network/src/protocol/external/codec.rs | 18 ++++++------------ 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/zebra-network/src/protocol/external.rs b/zebra-network/src/protocol/external.rs index 9a1e2472bba..8698f488b39 100644 --- a/zebra-network/src/protocol/external.rs +++ b/zebra-network/src/protocol/external.rs @@ -12,6 +12,7 @@ mod arbitrary; #[cfg(test)] mod tests; -pub use codec::{Codec, MAX_PROTOCOL_MESSAGE_LEN}; +pub use codec::Codec; pub use inv::InventoryHash; pub use message::Message; +pub use zebra_chain::serialization::MAX_PROTOCOL_MESSAGE_LEN; diff --git a/zebra-network/src/protocol/external/codec.rs b/zebra-network/src/protocol/external/codec.rs index 3ccc27a5c25..fb54f514567 100644 --- a/zebra-network/src/protocol/external/codec.rs +++ b/zebra-network/src/protocol/external/codec.rs @@ -15,8 +15,8 @@ use zebra_chain::{ block::{self, Block}, parameters::Network, serialization::{ - sha256d, ReadZcashExt, SerializationError as Error, WriteZcashExt, ZcashDeserialize, - ZcashSerialize, + sha256d, zcash_deserialize_bytes_external_count, ReadZcashExt, SerializationError as Error, + WriteZcashExt, ZcashDeserialize, ZcashSerialize, MAX_PROTOCOL_MESSAGE_LEN, }, transaction::Transaction, }; @@ -31,9 +31,6 @@ use super::{ /// The length of a Bitcoin message header. const HEADER_LEN: usize = 24usize; -/// Maximum size of a protocol message body. -pub use zebra_chain::serialization::MAX_PROTOCOL_MESSAGE_LEN; - /// A codec which produces Bitcoin messages from byte streams and vice versa. pub struct Codec { builder: Builder, @@ -597,10 +594,9 @@ impl Codec { return Err(Error::Parse("Invalid filterload message body length.")); } + // Memory Denial of Service: we just limited the untrusted parsed length let filter_length: usize = body_len - FILTERLOAD_REMAINDER_LENGTH; - - let mut filter_bytes = vec![0; filter_length]; - reader.read_exact(&mut filter_bytes)?; + let filter_bytes = zcash_deserialize_bytes_external_count(filter_length, &mut reader)?; Ok(Message::FilterLoad { filter: Filter(filter_bytes), @@ -613,11 +609,9 @@ impl Codec { fn read_filteradd(&self, mut reader: R, body_len: usize) -> Result { const MAX_FILTERADD_LENGTH: usize = 520; + // Memory Denial of Service: limit the untrusted parsed length let filter_length: usize = min(body_len, MAX_FILTERADD_LENGTH); - - // Memory Denial of Service: this length has just been bounded - let mut filter_bytes = vec![0; filter_length]; - reader.read_exact(&mut filter_bytes)?; + let filter_bytes = zcash_deserialize_bytes_external_count(filter_length, &mut reader)?; Ok(Message::FilterAdd { data: filter_bytes }) } From bbc03268fade30a481abbec74b319610075a80fc Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 15 Apr 2021 20:05:40 +1000 Subject: [PATCH 3/5] Move some preallocate proptests to their own file And fix the test module structure so it is consistent with the rest of zebra-chain. --- zebra-chain/src/serialization.rs | 2 +- zebra-chain/src/serialization/tests.rs | 2 + .../src/serialization/tests/preallocate.rs | 101 +++++++++++++++ .../{proptests.rs => tests/prop.rs} | 6 +- .../src/serialization/zcash_deserialize.rs | 118 +----------------- 5 files changed, 107 insertions(+), 122 deletions(-) create mode 100644 zebra-chain/src/serialization/tests.rs create mode 100644 zebra-chain/src/serialization/tests/preallocate.rs rename zebra-chain/src/serialization/{proptests.rs => tests/prop.rs} (91%) diff --git a/zebra-chain/src/serialization.rs b/zebra-chain/src/serialization.rs index 10bb913af70..18bee6870b0 100644 --- a/zebra-chain/src/serialization.rs +++ b/zebra-chain/src/serialization.rs @@ -28,4 +28,4 @@ pub use zcash_serialize::{ }; #[cfg(test)] -mod proptests; +mod tests; diff --git a/zebra-chain/src/serialization/tests.rs b/zebra-chain/src/serialization/tests.rs new file mode 100644 index 00000000000..a7ac2ac35a7 --- /dev/null +++ b/zebra-chain/src/serialization/tests.rs @@ -0,0 +1,2 @@ +mod preallocate; +mod prop; diff --git a/zebra-chain/src/serialization/tests/preallocate.rs b/zebra-chain/src/serialization/tests/preallocate.rs new file mode 100644 index 00000000000..34d635f1109 --- /dev/null +++ b/zebra-chain/src/serialization/tests/preallocate.rs @@ -0,0 +1,101 @@ +//! Tests for trusted preallocation during deserialization. + +use proptest::{collection::size_range, prelude::*}; + +use std::matches; + +use crate::serialization::{ + zcash_deserialize::MAX_U8_ALLOCATION, SerializationError, ZcashDeserialize, ZcashSerialize, + MAX_PROTOCOL_MESSAGE_LEN, +}; + +// Allow direct serialization of Vec for these tests. We don't usually +// allow this because some types have specific rules for about serialization +// of their inner Vec. This method could be easily misused if it applied +// more generally. +impl ZcashSerialize for u8 { + fn zcash_serialize(&self, mut writer: W) -> Result<(), std::io::Error> { + writer.write_all(&[*self]) + } +} + +proptest! { +#![proptest_config(ProptestConfig::with_cases(4))] + + #[test] + /// Confirm that deserialize yields the expected result for any vec smaller than `MAX_U8_ALLOCATION` + fn u8_ser_deser_roundtrip(input in any_with::>(size_range(MAX_U8_ALLOCATION).lift()) ) { + let serialized = input.zcash_serialize_to_vec().expect("Serialization to vec must succeed"); + let cursor = std::io::Cursor::new(serialized); + let deserialized = >::zcash_deserialize(cursor).expect("deserialization from vec must succeed"); + prop_assert_eq!(deserialized, input) + } +} + +#[test] +/// Confirm that deserialize allows vectors with length up to and including `MAX_U8_ALLOCATION` +fn u8_deser_accepts_max_valid_input() { + let serialized = vec![0u8; MAX_U8_ALLOCATION] + .zcash_serialize_to_vec() + .expect("Serialization to vec must succeed"); + let cursor = std::io::Cursor::new(serialized); + let deserialized = >::zcash_deserialize(cursor); + assert!(deserialized.is_ok()) +} + +#[test] +/// Confirm that rejects vectors longer than `MAX_U8_ALLOCATION` +fn u8_deser_throws_when_input_too_large() { + let serialized = vec![0u8; MAX_U8_ALLOCATION + 1] + .zcash_serialize_to_vec() + .expect("Serialization to vec must succeed"); + let cursor = std::io::Cursor::new(serialized); + let deserialized = >::zcash_deserialize(cursor); + + assert!(matches!( + deserialized, + Err(SerializationError::Parse( + "Byte vector longer than MAX_U8_ALLOCATION" + )) + )) +} + +#[test] +/// Confirm that every u8 takes exactly 1 byte when serialized. +/// This verifies that our calculated `MAX_U8_ALLOCATION` is indeed an upper bound. +fn u8_size_is_correct() { + for byte in std::u8::MIN..=std::u8::MAX { + let serialized = byte + .zcash_serialize_to_vec() + .expect("Serialization to vec must succeed"); + assert!(serialized.len() == 1) + } +} + +#[test] +/// Verify that... +/// 1. The smallest disallowed `Vec` is too big to include in a Zcash Wire Protocol message +/// 2. The largest allowed `Vec`is exactly the size of a maximal Zcash Wire Protocol message +fn u8_max_allocation_is_correct() { + let mut shortest_disallowed_vec = vec![0u8; MAX_U8_ALLOCATION + 1]; + let shortest_disallowed_serialized = shortest_disallowed_vec + .zcash_serialize_to_vec() + .expect("Serialization to vec must succeed"); + + // Confirm that shortest_disallowed_vec is only one item larger than the limit + assert_eq!((shortest_disallowed_vec.len() - 1), MAX_U8_ALLOCATION); + // Confirm that shortest_disallowed_vec is too large to be included in a valid zcash message + assert!(shortest_disallowed_serialized.len() > MAX_PROTOCOL_MESSAGE_LEN); + + // Create largest_allowed_vec by removing one element from smallest_disallowed_vec without copying (for efficiency) + shortest_disallowed_vec.pop(); + let longest_allowed_vec = shortest_disallowed_vec; + let longest_allowed_serialized = longest_allowed_vec + .zcash_serialize_to_vec() + .expect("serialization to vec must succed"); + + // Check that our largest_allowed_vec contains the maximum number of items + assert_eq!(longest_allowed_vec.len(), MAX_U8_ALLOCATION); + // Check that our largest_allowed_vec is the size of a maximal protocol message + assert_eq!(longest_allowed_serialized.len(), MAX_PROTOCOL_MESSAGE_LEN); +} diff --git a/zebra-chain/src/serialization/proptests.rs b/zebra-chain/src/serialization/tests/prop.rs similarity index 91% rename from zebra-chain/src/serialization/proptests.rs rename to zebra-chain/src/serialization/tests/prop.rs index 99f2ec16617..259695ed9c0 100644 --- a/zebra-chain/src/serialization/proptests.rs +++ b/zebra-chain/src/serialization/tests/prop.rs @@ -1,14 +1,12 @@ //! Property-based tests for basic serialization primitives. -use super::*; use proptest::prelude::*; use std::io::Cursor; -proptest! { - // The tests below are cheap so we can run them a lot. - #![proptest_config(ProptestConfig::with_cases(100_000))] +use crate::serialization::{ReadZcashExt, WriteZcashExt}; +proptest! { #[test] fn compactsize_write_then_read_round_trip(s in 0u64..0x2_0000u64) { zebra_test::init(); diff --git a/zebra-chain/src/serialization/zcash_deserialize.rs b/zebra-chain/src/serialization/zcash_deserialize.rs index 5885be08c66..5b0abe5ec6f 100644 --- a/zebra-chain/src/serialization/zcash_deserialize.rs +++ b/zebra-chain/src/serialization/zcash_deserialize.rs @@ -150,120 +150,4 @@ pub trait TrustedPreallocate { /// It takes 5 bytes to encode a compactsize representing any number netween 2^16 and (2^32 - 1) /// MAX_PROTOCOL_MESSAGE_LEN is ~2^21, so the largest Vec that can be received from an honest peer is /// (MAX_PROTOCOL_MESSAGE_LEN - 5); -const MAX_U8_ALLOCATION: usize = MAX_PROTOCOL_MESSAGE_LEN - 5; - -/// Implement ZcashDeserialize for Vec directly instead of using the blanket Vec implementation -/// -/// This allows us to optimize the inner loop into a single call to `read_exact()` -/// Note thate we don't implement TrustedPreallocate for u8. -/// This allows the optimization without relying on specialization. -impl ZcashDeserialize for Vec { - fn zcash_deserialize(mut reader: R) -> Result { - let len = reader.read_compactsize()?.try_into()?; - if len > MAX_U8_ALLOCATION { - return Err(SerializationError::Parse( - "Vector longer than max_allocation", - )); - } - let mut vec = vec![0u8; len]; - reader.read_exact(&mut vec)?; - Ok(vec) - } -} - -#[cfg(test)] -mod test_u8_deserialize { - use super::MAX_U8_ALLOCATION; - use crate::serialization::MAX_PROTOCOL_MESSAGE_LEN; - use crate::serialization::{SerializationError, ZcashDeserialize, ZcashSerialize}; - use proptest::{collection::size_range, prelude::*}; - use std::matches; - - // Allow direct serialization of Vec for these tests. We don't usuall allow this because some types have - // specific rules for about serialization of their inner Vec. This method could be easily misused if it applied - // more generally. - impl ZcashSerialize for u8 { - fn zcash_serialize(&self, mut writer: W) -> Result<(), std::io::Error> { - writer.write_all(&[*self]) - } - } - - proptest! { - #![proptest_config(ProptestConfig::with_cases(3))] - #[test] - /// Confirm that deserialize yields the expected result for any vec smaller than `MAX_U8_ALLOCATION` - fn u8_ser_deser_roundtrip(input in any_with::>(size_range(MAX_U8_ALLOCATION).lift()) ) { - let serialized = input.zcash_serialize_to_vec().expect("Serialization to vec must succeed"); - let cursor = std::io::Cursor::new(serialized); - let deserialized = >::zcash_deserialize(cursor).expect("deserialization from vec must succeed"); - prop_assert_eq!(deserialized, input) - } - } - - #[test] - /// Confirm that deserialize allows vectors with length up to and including `MAX_U8_ALLOCATION` - fn u8_deser_accepts_max_valid_input() { - let serialized = vec![0u8; MAX_U8_ALLOCATION] - .zcash_serialize_to_vec() - .expect("Serialization to vec must succeed"); - let cursor = std::io::Cursor::new(serialized); - let deserialized = >::zcash_deserialize(cursor); - assert!(deserialized.is_ok()) - } - #[test] - /// Confirm that rejects vectors longer than `MAX_U8_ALLOCATION` - fn u8_deser_throws_when_input_too_large() { - let serialized = vec![0u8; MAX_U8_ALLOCATION + 1] - .zcash_serialize_to_vec() - .expect("Serialization to vec must succeed"); - let cursor = std::io::Cursor::new(serialized); - let deserialized = >::zcash_deserialize(cursor); - - assert!(matches!( - deserialized, - Err(SerializationError::Parse( - "Vector longer than max_allocation" - )) - )) - } - - #[test] - /// Confirm that every u8 takes exactly 1 byte when serialized. - /// This verifies that our calculated `MAX_U8_ALLOCATION` is indeed an upper bound. - fn u8_size_is_correct() { - for byte in std::u8::MIN..=std::u8::MAX { - let serialized = byte - .zcash_serialize_to_vec() - .expect("Serialization to vec must succeed"); - assert!(serialized.len() == 1) - } - } - - #[test] - /// Verify that... - /// 1. The smallest disallowed `Vec` is too big to include in a Zcash Wire Protocol message - /// 2. The largest allowed `Vec`is exactly the size of a maximal Zcash Wire Protocol message - fn u8_max_allocation_is_correct() { - let mut shortest_disallowed_vec = vec![0u8; MAX_U8_ALLOCATION + 1]; - let shortest_disallowed_serialized = shortest_disallowed_vec - .zcash_serialize_to_vec() - .expect("Serialization to vec must succeed"); - - // Confirm that shortest_disallowed_vec is only one item larger than the limit - assert_eq!((shortest_disallowed_vec.len() - 1), MAX_U8_ALLOCATION); - // Confirm that shortest_disallowed_vec is too large to be included in a valid zcash message - assert!(shortest_disallowed_serialized.len() > MAX_PROTOCOL_MESSAGE_LEN); - - // Create largest_allowed_vec by removing one element from smallest_disallowed_vec without copying (for efficiency) - shortest_disallowed_vec.pop(); - let longest_allowed_vec = shortest_disallowed_vec; - let longest_allowed_serialized = longest_allowed_vec - .zcash_serialize_to_vec() - .expect("serialization to vec must succed"); - - // Check that our largest_allowed_vec contains the maximum number of items - assert_eq!(longest_allowed_vec.len(), MAX_U8_ALLOCATION); - // Check that our largest_allowed_vec is the size of a maximal protocol message - assert_eq!(longest_allowed_serialized.len(), MAX_PROTOCOL_MESSAGE_LEN); - } -} +pub(crate) const MAX_U8_ALLOCATION: usize = MAX_PROTOCOL_MESSAGE_LEN - 5; From 7b4ad4135da533ed086bf38fda660aea28f8eba6 Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 15 Apr 2021 20:54:12 +1000 Subject: [PATCH 4/5] Add a convenience alias zcash_serialize_external_count --- zebra-chain/src/serialization.rs | 3 ++- .../src/serialization/zcash_serialize.rs | 20 ++++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/zebra-chain/src/serialization.rs b/zebra-chain/src/serialization.rs index 18bee6870b0..a4e94d8365c 100644 --- a/zebra-chain/src/serialization.rs +++ b/zebra-chain/src/serialization.rs @@ -24,7 +24,8 @@ pub use zcash_deserialize::{ ZcashDeserialize, ZcashDeserializeInto, }; pub use zcash_serialize::{ - zcash_serialize_external_count, ZcashSerialize, MAX_PROTOCOL_MESSAGE_LEN, + zcash_serialize_bytes_external_count, zcash_serialize_external_count, ZcashSerialize, + MAX_PROTOCOL_MESSAGE_LEN, }; #[cfg(test)] diff --git a/zebra-chain/src/serialization/zcash_serialize.rs b/zebra-chain/src/serialization/zcash_serialize.rs index 14717158f33..b06247df137 100644 --- a/zebra-chain/src/serialization/zcash_serialize.rs +++ b/zebra-chain/src/serialization/zcash_serialize.rs @@ -40,7 +40,8 @@ impl ZcashSerialize for Vec { } } -/// Serialize a `Vec` **without** writing the number of items as a compactsize. +/// Serialize a typed `Vec` **without** writing the number of items as a +/// compactsize. /// /// In Zcash, most arrays are stored as a compactsize, followed by that number /// of items of type `T`. But in `Transaction::V5`, some types are serialized as @@ -50,8 +51,7 @@ impl ZcashSerialize for Vec { /// ## Usage /// /// Use `zcash_serialize_external_count` when the array count is determined by -/// other data, or a consensus rule. (For raw byte vectors and slices, use -/// `writer.write_all(&vec)`.) +/// other data, or a consensus rule. /// /// Use `Vec::zcash_serialize` for data that contains compactsize count, /// followed by the data array. @@ -77,6 +77,20 @@ pub fn zcash_serialize_external_count( Ok(()) } +/// Serialize a raw byte `Vec` **without** writing the number of items as a +/// compactsize. +/// +/// This is a convenience alias for `writer.write_all(&vec)`. +// +// we specifically want to serialize `Vec`s here, rather than generic slices +#[allow(clippy::ptr_arg)] +pub fn zcash_serialize_bytes_external_count( + vec: &Vec, + mut writer: W, +) -> Result<(), io::Error> { + writer.write_all(&vec) +} + /// The maximum length of a Zcash message, in bytes. /// /// This value is used to calculate safe preallocation limits for some types From 818bd32329a2c355f8b88b4517a5d8acfb6577b2 Mon Sep 17 00:00:00 2001 From: teor Date: Fri, 16 Apr 2021 08:21:26 +1000 Subject: [PATCH 5/5] Explain why u64::MAX items will never be reached --- zebra-chain/src/serialization/zcash_deserialize.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/zebra-chain/src/serialization/zcash_deserialize.rs b/zebra-chain/src/serialization/zcash_deserialize.rs index 5b0abe5ec6f..1a9be47d9a6 100644 --- a/zebra-chain/src/serialization/zcash_deserialize.rs +++ b/zebra-chain/src/serialization/zcash_deserialize.rs @@ -78,6 +78,9 @@ pub fn zcash_deserialize_external_count {} + // As of 2021, usize is less than or equal to 64 bits on all (or almost all?) supported Rust platforms. + // So in practice this error is impossible. (But the check is required, because Rust is future-proof + // for 128 bit memory spaces.) Err(_) => return Err(SerializationError::Parse("Vector longer than u64::MAX")), } let mut vec = Vec::with_capacity(external_count);