From 80ef3792020d8c79a93f3add1ab3f40054cd97e7 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 3 May 2019 15:43:10 +1000 Subject: [PATCH] Improve docs, cover a bunch of invalid inputs --- eth2/utils/ssz2/src/decode.rs | 14 ++- eth2/utils/ssz2/src/decode/impls.rs | 163 ++++++++++++++++++++++++---- 2 files changed, 151 insertions(+), 26 deletions(-) diff --git a/eth2/utils/ssz2/src/decode.rs b/eth2/utils/ssz2/src/decode.rs index 7481565f1..db7c68d3c 100644 --- a/eth2/utils/ssz2/src/decode.rs +++ b/eth2/utils/ssz2/src/decode.rs @@ -4,10 +4,20 @@ mod impls; #[derive(Debug, PartialEq)] pub enum DecodeError { - // BytesTooShort { given: usize, expected: usize }, - // BytesTooLong { given: usize, expected: usize }, + /// The bytes supplied were too short to be decoded into the specified type. InvalidByteLength { len: usize, expected: usize }, + /// The given bytes were too short to be read as a length prefix. InvalidLengthPrefix { len: usize, expected: usize }, + /// A length offset pointed to a byte that was out-of-bounds (OOB). + /// + /// A bytes may be OOB for the following reasons: + /// + /// - It is `>= bytes.len()`. + /// - When decoding variable length items, the 1st offset points "backwards" into the fixed + /// length items (i.e., `length[0] < BYTES_PER_LENGTH_OFFSET`). + /// - When decoding variable-length items, the `n`'th offset was less than the `n-1`'th offset. + OutOfBoundsByte { i: usize }, + /// The given bytes were invalid for some application-level reason. BytesInvalid(String), } diff --git a/eth2/utils/ssz2/src/decode/impls.rs b/eth2/utils/ssz2/src/decode/impls.rs index e795ad847..a00de33aa 100644 --- a/eth2/utils/ssz2/src/decode/impls.rs +++ b/eth2/utils/ssz2/src/decode/impls.rs @@ -39,36 +39,53 @@ impl Decodable for Vec { } fn from_ssz_bytes(bytes: &[u8]) -> Result { - // Zero-bytes indicates an empty list. if bytes.len() == 0 { - return Ok(vec![]); - } - - // Decode a non-empty list differently, depending if the items in the vec are fixed-length - // or variable-length. - if T::is_ssz_fixed_len() { - Ok(bytes + Ok(vec![]) + } else if T::is_ssz_fixed_len() { + bytes .chunks(T::ssz_fixed_len()) .map(|chunk| T::from_ssz_bytes(chunk)) - .collect::>()?) + .collect() } else { let mut next_variable_byte = read_offset(bytes)?; - let mut values = Vec::with_capacity(next_variable_byte / BYTES_PER_LENGTH_OFFSET); + // The value of the first offset must not point back into the same bytes that defined + // it. + if next_variable_byte < BYTES_PER_LENGTH_OFFSET { + return Err(DecodeError::OutOfBoundsByte { + i: next_variable_byte, + }); + } - for i in 1..=values.capacity() { - let slice = if i == values.capacity() { - &bytes[next_variable_byte..] + let num_items = next_variable_byte / BYTES_PER_LENGTH_OFFSET; + + // The fixed-length section must be a clean multiple of `BYTES_PER_LENGTH_OFFSET`. + if next_variable_byte != num_items * BYTES_PER_LENGTH_OFFSET { + return Err(DecodeError::InvalidByteLength { + len: next_variable_byte, + expected: num_items * BYTES_PER_LENGTH_OFFSET, + }); + } + + let mut values = Vec::with_capacity(num_items); + for i in 1..=num_items { + let slice_option = if i == num_items { + bytes.get(next_variable_byte..) } else { - let offset = decode_offset( - &bytes[i * BYTES_PER_LENGTH_OFFSET..(i + 1) * BYTES_PER_LENGTH_OFFSET], - )?; + let offset = read_offset(&bytes[(i * BYTES_PER_LENGTH_OFFSET)..])?; + let start = next_variable_byte; next_variable_byte = offset; - &bytes[start..next_variable_byte] + // Note: the condition where `start > next_variable_byte` returns `None` which + // raises an error later in the program. + bytes.get(start..next_variable_byte) }; + let slice = slice_option.ok_or_else(|| DecodeError::OutOfBoundsByte { + i: next_variable_byte, + })?; + values.push(T::from_ssz_bytes(slice)?); } @@ -242,14 +259,76 @@ mod tests { assert_eq!(res, Err(DecodeError::TooShort)); } */ + #[test] - fn from_ssz_bytes_vec_u16() { - assert_eq!(>::from_ssz_bytes(&[0, 0, 0, 0]), Ok(vec![0, 0])); + fn first_length_points_backwards() { + assert_eq!( + >>::from_ssz_bytes(&[0, 0, 0, 0]), + Err(DecodeError::OutOfBoundsByte { i: 0 }) + ); + + assert_eq!( + >>::from_ssz_bytes(&[1, 0, 0, 0]), + Err(DecodeError::OutOfBoundsByte { i: 1 }) + ); + + assert_eq!( + >>::from_ssz_bytes(&[2, 0, 0, 0]), + Err(DecodeError::OutOfBoundsByte { i: 2 }) + ); + + assert_eq!( + >>::from_ssz_bytes(&[3, 0, 0, 0]), + Err(DecodeError::OutOfBoundsByte { i: 3 }) + ); + } + + #[test] + fn lengths_are_decreasing() { + assert_eq!( + >>::from_ssz_bytes(&[12, 0, 0, 0, 14, 0, 0, 0, 12, 0, 0, 0, 1, 0, 1, 0]), + Err(DecodeError::OutOfBoundsByte { i: 12 }) + ); + } + + #[test] + fn awkward_fixed_lenth_portion() { + assert_eq!( + >>::from_ssz_bytes(&[10, 0, 0, 0, 10, 0, 0, 0, 0, 0]), + Err(DecodeError::InvalidByteLength { + len: 10, + expected: 8 + }) + ); + } + + #[test] + fn length_out_of_bounds() { + assert_eq!( + >>::from_ssz_bytes(&[5, 0, 0, 0]), + Err(DecodeError::InvalidByteLength { + len: 5, + expected: 4 + }) + ); + assert_eq!( + >>::from_ssz_bytes(&[8, 0, 0, 0, 9, 0, 0, 0]), + Err(DecodeError::OutOfBoundsByte { i: 9 }) + ); + } + + #[test] + fn vec_of_vec_of_u16() { + assert_eq!( + >>::from_ssz_bytes(&[4, 0, 0, 0]), + Ok(vec![vec![]]) + ); + + /* assert_eq!( >::from_ssz_bytes(&[0, 0, 1, 0, 2, 0, 3, 0]), Ok(vec![0, 1, 2, 3]) ); - /* assert_eq!(::from_ssz_bytes(&[16, 0]), Ok(16)); assert_eq!(::from_ssz_bytes(&[0, 1]), Ok(256)); assert_eq!(::from_ssz_bytes(&[255, 255]), Ok(65535)); @@ -257,7 +336,7 @@ mod tests { assert_eq!( ::from_ssz_bytes(&[255]), Err(DecodeError::InvalidByteLength { - given: 1, + len: 1, expected: 2 }) ); @@ -265,7 +344,7 @@ mod tests { assert_eq!( ::from_ssz_bytes(&[]), Err(DecodeError::InvalidByteLength { - given: 0, + len: 0, expected: 2 }) ); @@ -273,7 +352,7 @@ mod tests { assert_eq!( ::from_ssz_bytes(&[0, 1, 2]), Err(DecodeError::InvalidByteLength { - given: 3, + len: 3, expected: 2 }) ); @@ -281,7 +360,43 @@ mod tests { } #[test] - fn from_ssz_bytes_u16() { + fn vec_of_u16() { + assert_eq!(>::from_ssz_bytes(&[0, 0, 0, 0]), Ok(vec![0, 0])); + assert_eq!( + >::from_ssz_bytes(&[0, 0, 1, 0, 2, 0, 3, 0]), + Ok(vec![0, 1, 2, 3]) + ); + assert_eq!(::from_ssz_bytes(&[16, 0]), Ok(16)); + assert_eq!(::from_ssz_bytes(&[0, 1]), Ok(256)); + assert_eq!(::from_ssz_bytes(&[255, 255]), Ok(65535)); + + assert_eq!( + ::from_ssz_bytes(&[255]), + Err(DecodeError::InvalidByteLength { + len: 1, + expected: 2 + }) + ); + + assert_eq!( + ::from_ssz_bytes(&[]), + Err(DecodeError::InvalidByteLength { + len: 0, + expected: 2 + }) + ); + + assert_eq!( + ::from_ssz_bytes(&[0, 1, 2]), + Err(DecodeError::InvalidByteLength { + len: 3, + expected: 2 + }) + ); + } + + #[test] + fn u16() { assert_eq!(::from_ssz_bytes(&[0, 0]), Ok(0)); assert_eq!(::from_ssz_bytes(&[16, 0]), Ok(16)); assert_eq!(::from_ssz_bytes(&[0, 1]), Ok(256));