feat(scale): add range checks to decodeUint function

[edwardmack]

Changes

• Refactor decodeUint to add range checks that check to confirm decoded value is within range for given prefix mode.
  This replicates behavior of parity scale codec decode functions.
• Add tests to confirm updated functionality is working as expected.

Tests

go test github.com/ChainSafe/gossamer/pkg/scale/

Issues

fixes #2629, which was causing out of memory errors when trying to decode very long bytes encodings.

Primary Reviewer

@timwu20

[qdm12]

Thinking about this, 1GB per malicious message is still risky.
I'm wondering how quickly a peer would be banned if it sends a few 1GB messages at the same time (before program crash).

[codecov]

Codecov Report

Merging #2683 (dd4c99a) into development (f635c59) will increase coverage by 0.36%.
The diff coverage is 84.61%.

Additional details and impacted files

@@               Coverage Diff               @@
##           development    #2683      +/-   ##
===============================================
+ Coverage        62.88%   63.25%   +0.36%     
===============================================
  Files              213      213              
  Lines            27021    26987      -34     
===============================================
+ Hits             16992    17070      +78     
+ Misses            8476     8367     -109     
+ Partials          1553     1550       -3     

[qdm12]

Aren't we just reading 0 bytes from that buffer? 🤔 Or the ds.Read is badly named and should be Write instead??

[edwardmack]

Note: this was changed because after refactoring decodeUint it was getting errors, which is consistent with parity scale codec behavior, confirm when testing with:

#[test]
	fn decode_vec() {
		let encoded = vec![0x10, 0x07, 0x00, 0x00, 0x00, 0x00, 0x01, 0x08, 0x0c, 0x07, 0x00, 0x00, 0x00, 0x00, 0x01];
		let result = <Vec<u8>>::decode_all(&mut encoded.as_slice());
		println!("result: {:?}", result);
	}

Returns error: Err(Error { cause: None, desc: "Input buffer has still data left after decoding!" })

While our code ignores remaining bytes on buffer. Let me know any ideas how to check if bytes remain.

Correct encoding determined by parity scale codec:

#[test]
	fn vec_and_slice() {
		let slice: &[i64] = &[1 << 32, 2, 3, 1 << 32];
		let data: Vec<i64> = slice.iter().copied().collect();

		let data_encoded = data.encode();
		println!("result: {:?}", data_encoded);
	}

[timwu20]

Since we use the same function for a target uint in a struct, and to a uint directly, it will be difficult to error out correctly without refactoring things significantly. I was trying to figure out how substrate deals with this, but I kinda got lost navigating around. I would assume this error only comes as a result of using decode_all, which implies that all the bytes should be read.

Looking at the test code in decode_all.rs, for struct types you need to handle each attribute specifically when implementing the Decode trait. This differs from our scale package, since it decodes optimistically across the struct attributes.

I tried adding a test with the Vec<u8> as an attribute, and I passed in the bytes that produced that error using decode_all and It didn't produce that error when it's a struct attribute. See TestStruct2

mod tests {
	use super::*;
	use crate::{Compact, Encode, EncodeLike, Input};

	macro_rules! test_decode_all {
		(
			$( $type:ty => $value:expr; )*
		) => {
			$(
				{
					let mut encoded = <$type as Encode>::encode(&$value);
					<$type>::decode_all(&mut encoded.as_slice()).expect(
						&format!("`{} => {}` decodes all!", stringify!($type), stringify!($value)),
					);

					encoded.extend(&[1, 2, 3, 4, 5, 6]);
					assert_eq!(
						<$type>::decode_all(&mut encoded.as_slice()).unwrap_err().to_string(),
						"Input buffer has still data left after decoding!",
					);
				}
			)*
		};
	}

	...

	#[derive(Debug)]
	struct TestStruct2 {
		data: Vec<u8>,
		other: u8,
		compact: Compact<u128>,
	}

	impl EncodeLike for TestStruct2 {}

	impl Encode for TestStruct2 {
		fn encode(&self) -> Vec<u8> {
			let mut res = Vec::new();
			self.data.encode_to(&mut res);
			self.other.encode_to(&mut res);
			self.compact.encode_to(&mut res);
			res
		}
	}

	impl Decode for TestStruct2 {
		fn decode<I: Input>(input: &mut I) -> Result<Self, Error> {
			Ok(Self {
				data: Vec::<u8>::decode(input)?,
				other: u8::decode(input)?,
				compact: Compact::<u128>::decode(input)?,
			})
		}
	}

	#[test]
	fn decode_all_works() {
		test_decode_all! {
			u8 => 120;
			u16 => 30;
			u32 => 1;
			u64 => 2343545;
			u128 => 34358394245459854;
			Vec<u8> => vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
			Vec<u32> => vec![1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
			Compact<u32> => Compact(32445);
			Compact<u128> => Compact(34353454453545);
			TestStruct => TestStruct { data: vec![1, 2, 4, 5, 6], other: 45, compact: Compact(123234545) };
			TestStruct2 => TestStruct2 {
				data: vec![
					0x10, 0x07, 0x00, 0x00, 0x00, 0x00, 0x01, 0x08, 0x0c, 0x07, 0x00, 0x00, 0x00, 0x00,
					0x01,
				],
				other: 45,
				compact: Compact(123234545),
			};
		}
	}
}

[edwardmack]

This makes sense, thanks.

[edwardmack]

Note: this was changed because after refactoring decodeUint it was getting errors, which is consistent with parity scale codec behavior, confirm when testing with:

#[test]
	fn decode_vec() {
		let encoded = vec![0x07, 0x00, 0x00, 0x00, 0x00, 0x01, 0x08, 0x0c, 0x07, 0x00, 0x00, 0x00, 0x00, 0x01];
		let result = <Vec<u8>>::decode_all(&mut encoded.as_slice());
		println!("result: {:?}", result);
	}

[timwu20]

Great work @edwardmack!

[qdm12]

Just a few minor nit commit suggestions 😉

[qdm12]

This blocks a unit test case in #2673 and a unit test case in #2747 (since the error code path cannot be triggered without this PR!)

[github-actions]

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀