Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(scale): add range checks to decodeUint function #2683

Merged
merged 14 commits into from
Sep 8, 2022
Merged

feat(scale): add range checks to decodeUint function #2683

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ceb073c
introduce decodeCompactUint32 function
edwardmack Jul 19, 2022
1ea546d
replace use of int64 with uint32
edwardmack Jul 20, 2022
3c79993
update node decode_test
edwardmack Jul 20, 2022
ea5836a
refactor decodeUint to conform with parity scale codec
edwardmack Jul 22, 2022
a289ece
add test for changed []int encodings
edwardmack Jul 22, 2022
8b77f71
add error
edwardmack Jul 25, 2022
9f53087
update error checking and messages
edwardmack Jul 27, 2022
9787bd3
update decodeLength to return uint
edwardmack Jul 27, 2022
6a542d4
address PR comments
edwardmack Aug 22, 2022
6494fe2
add to unit tests
edwardmack Aug 22, 2022
f11c61b
fix unit tests to match error messages
edwardmack Aug 22, 2022
3d5f687
update error messages to match tests
edwardmack Aug 22, 2022
8ae1960
implement PR comments
edwardmack Sep 7, 2022
dd4c99a
run goimports
edwardmack Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions internal/trie/node/decode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func Test_decodeBranch(t *testing.T) {
variant: branchVariant.bits,
partialKeyLength: 1,
errWrapped: ErrDecodeChildHash,
errMessage: "cannot decode child hash: at index 10: EOF",
errMessage: "cannot decode child hash: at index 10: reading byte: EOF",
},
"success for branch variant": {
reader: bytes.NewBuffer(
Expand Down Expand Up @@ -203,7 +203,7 @@ func Test_decodeBranch(t *testing.T) {
variant: branchWithValueVariant.bits,
partialKeyLength: 1,
errWrapped: ErrDecodeValue,
errMessage: "cannot decode value: EOF",
errMessage: "cannot decode value: reading byte: EOF",
},
"success for branch with value": {
reader: bytes.NewBuffer(concatByteSlices([][]byte{
Expand Down Expand Up @@ -333,7 +333,7 @@ func Test_decodeLeaf(t *testing.T) {
variant: leafVariant.bits,
partialKeyLength: 1,
errWrapped: ErrDecodeValue,
errMessage: "cannot decode value: could not decode invalid integer",
errMessage: "cannot decode value: unknown prefix for compact uint: 255",
},
"zero value": {
reader: bytes.NewBuffer([]byte{
Expand Down
2 changes: 1 addition & 1 deletion lib/runtime/version_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func Test_DecodeVersion(t *testing.T) {
{255, 255}, // error
}),
errWrapped: ErrDecodingVersionField,
errMessage: "decoding version field impl name: could not decode invalid integer",
errMessage: "decoding version field impl name: unknown prefix for compact uint: 255",
},
// TODO add transaction version decode error once
// https://github.com/ChainSafe/gossamer/pull/2683
Expand Down
91 changes: 61 additions & 30 deletions pkg/scale/decode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ func (ds *decodeState) decodeVaryingDataTypeSlice(dstv reflect.Value) (err error
if err != nil {
return
}
for i := 0; i < l; i++ {
for i := uint(0); i < l; i++ {
vdt := vdts.VaryingDataType
vdtv := reflect.New(reflect.TypeOf(vdt))
vdtv.Elem().Set(reflect.ValueOf(vdt))
Expand Down Expand Up @@ -397,7 +397,7 @@ func (ds *decodeState) decodeSlice(dstv reflect.Value) (err error) {
}
in := dstv.Interface()
temp := reflect.New(reflect.ValueOf(in).Type())
for i := 0; i < l; i++ {
for i := uint(0); i < l; i++ {
tempElemType := reflect.TypeOf(in).Elem()
tempElem := reflect.New(tempElemType).Elem()

Expand Down Expand Up @@ -478,59 +478,90 @@ func (ds *decodeState) decodeBool(dstv reflect.Value) (err error) {

// decodeUint will decode unsigned integer
func (ds *decodeState) decodeUint(dstv reflect.Value) (err error) {
b, err := ds.ReadByte()
const maxUint32 = ^uint32(0)
const maxUint64 = ^uint64(0)
prefix, err := ds.ReadByte()
if err != nil {
return
return fmt.Errorf("reading byte: %w", err)
}

in := dstv.Interface()
temp := reflect.New(reflect.TypeOf(in))
// check mode of encoding, stored at 2 least significant bits
mode := b & 3
switch {
case mode <= 2:
var val int64
val, err = ds.decodeSmallInt(b, mode)
mode := prefix % 4
var value uint64
switch mode {
case 0:
value = uint64(prefix >> 2)
case 1:
buf, err := ds.ReadByte()
if err != nil {
return
return fmt.Errorf("reading byte: %w", err)
}
temp.Elem().Set(reflect.ValueOf(val).Convert(reflect.TypeOf(in)))
dstv.Set(temp.Elem())
default:
// >4 byte mode
topSixBits := b >> 2
byteLen := uint(topSixBits) + 4

value = uint64(binary.LittleEndian.Uint16([]byte{prefix, buf}) >> 2)
if value <= 0b0011_1111 || value > 0b0111_1111_1111_1111 {
return fmt.Errorf("%w: %d (%b)", ErrU16OutOfRange, value, value)
}
edwardmack marked this conversation as resolved.
Show resolved Hide resolved
case 2:
buf := make([]byte, 3)
_, err = ds.Read(buf)
if err != nil {
return fmt.Errorf("reading bytes: %w", err)
}
value = uint64(binary.LittleEndian.Uint32(append([]byte{prefix}, buf...)) >> 2)
if value <= 0b0011_1111_1111_1111 || value > uint64(maxUint32>>2) {
return fmt.Errorf("%w: %d (%b)", ErrU32OutOfRange, value, value)
}
case 3:
byteLen := (prefix >> 2) + 4
buf := make([]byte, byteLen)
_, err = ds.Read(buf)
if err != nil {
return
return fmt.Errorf("reading bytes: %w", err)
}

var o uint64
if byteLen == 4 {
o = uint64(binary.LittleEndian.Uint32(buf))
} else if byteLen > 4 && byteLen <= 8 {
switch byteLen {
case 4:
value = uint64(binary.LittleEndian.Uint32(buf))
if value <= uint64(maxUint32>>2) {
return fmt.Errorf("%w: %d (%b)", ErrU32OutOfRange, value, value)
}
case 8:
const uintSize = 32 << (^uint(0) >> 32 & 1)
if uintSize == 32 {
return ErrU64NotSupported
}
tmp := make([]byte, 8)
copy(tmp, buf)
o = binary.LittleEndian.Uint64(tmp)
} else {
err = errors.New("could not decode invalid integer")
return
value = binary.LittleEndian.Uint64(tmp)
if value <= maxUint64>>8 {
return fmt.Errorf("%w: %d (%b)", ErrU64OutOfRange, value, value)
}
default:
return fmt.Errorf("%w: %d", ErrCompactUintPrefixUnknown, prefix)

}
dstv.Set(reflect.ValueOf(o).Convert(reflect.TypeOf(in)))
}
temp.Elem().Set(reflect.ValueOf(value).Convert(reflect.TypeOf(in)))
dstv.Set(temp.Elem())
return
}

var (
ErrU16OutOfRange = errors.New("uint16 out of range")
ErrU32OutOfRange = errors.New("uint32 out of range")
ErrU64OutOfRange = errors.New("uint64 out of range")
ErrU64NotSupported = errors.New("uint64 is not supported")
ErrCompactUintPrefixUnknown = errors.New("unknown prefix for compact uint")
)

// decodeLength is helper method which calls decodeUint and casts to int
func (ds *decodeState) decodeLength() (l int, err error) {
func (ds *decodeState) decodeLength() (l uint, err error) {
dstv := reflect.New(reflect.TypeOf(l))
err = ds.decodeUint(dstv.Elem())
if err != nil {
return
}
l = dstv.Elem().Interface().(int)
l = dstv.Elem().Interface().(uint)
return
}

Expand Down
101 changes: 101 additions & 0 deletions pkg/scale/decode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/stretchr/testify/assert"
edwardmack marked this conversation as resolved.
Show resolved Hide resolved
)

func Test_decodeState_decodeFixedWidthInt(t *testing.T) {
Expand Down Expand Up @@ -302,3 +303,103 @@ func Test_Decoder_Decode_MultipleCalls(t *testing.T) {
})
}
}

func Test_decodeState_decodeUint(t *testing.T) {
t.Parallel()
decodeUint32Tests := tests{
{
name: "int(1) mode 0",
in: uint32(1),
want: []byte{0x04},
},
{
name: "int(16383) mode 1",
in: int(16383),
want: []byte{0xfd, 0xff},
},
{
name: "int(1073741823) mode 2",
in: int(1073741823),
want: []byte{0xfe, 0xff, 0xff, 0xff},
},
{
name: "int(4294967295) mode 3",
in: int(4294967295),
want: []byte{0x3, 0xff, 0xff, 0xff, 0xff},
},
{
name: "myCustomInt(9223372036854775807) mode 3, 64bit",
in: myCustomInt(9223372036854775807),
want: []byte{19, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f},
},
{
name: "uint(overload)",
in: int(0),
want: []byte{0x07, 0x08, 0x09, 0x10, 0x0, 0x40},
wantErr: true,
},
{
name: "uint(16384) mode 2",
in: int(16384),
want: []byte{0x02, 0x00, 0x01, 0x0},
},
{
name: "uint(0) mode 1, error",
in: int(0),
want: []byte{0x01, 0x00},
wantErr: true,
},
{
name: "uint(0) mode 2, error",
in: int(0),
want: []byte{0x02, 0x00, 0x00, 0x0},
wantErr: true,
},
{
name: "uint(0) mode 3, error",
in: int(0),
want: []byte{0x03, 0x00, 0x00, 0x0},
wantErr: true,
},
{
name: "mode 3, 64bit, error",
in: int(0),
want: []byte{19, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0},
wantErr: true,
},
{
name: "[]int{1 << 32, 2, 3, 1 << 32}",
in: uint(4),
want: []byte{0x10, 0x07, 0x00, 0x00, 0x00, 0x00, 0x01, 0x08, 0x0c, 0x07, 0x00, 0x00, 0x00, 0x00, 0x01},
},
{
name: "[4]int{1 << 32, 2, 3, 1 << 32}",
in: [4]int{0, 0, 0, 0},
want: []byte{0x07, 0x00, 0x00, 0x00, 0x00, 0x01, 0x08, 0x0c, 0x07, 0x00, 0x00, 0x00, 0x00, 0x01},
wantErr: true,
},
}

for _, tt := range decodeUint32Tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
dst := reflect.New(reflect.TypeOf(tt.in)).Elem().Interface()
dstv := reflect.ValueOf(&dst)
elem := indirect(dstv)

buf := &bytes.Buffer{}
ds := decodeState{}
_, err := buf.Write(tt.want)
assert.NoError(t, err)
ds.Reader = buf
edwardmack marked this conversation as resolved.
Show resolved Hide resolved
err = ds.decodeUint(elem)
if (err != nil) != tt.wantErr {
t.Errorf("decodeState.decodeUint error = %v, wantErr %v", err, tt.wantErr)
}
if !reflect.DeepEqual(dst, tt.in) {
t.Errorf("decodeState.decodeUint = %v, want %v", dst, tt.in)
}
edwardmack marked this conversation as resolved.
Show resolved Hide resolved
})
}
}
21 changes: 15 additions & 6 deletions pkg/scale/encode_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,11 @@ var (
in: int(1),
want: []byte{0x04},
},
{
name: "int(42)",
in: int(42),
want: []byte{0xa8},
},
{
name: "int(16383)",
in: int(16383),
Expand Down Expand Up @@ -820,9 +825,11 @@ var (
want: []byte{0x10, 0x03, 0x00, 0x00, 0x00, 0x40, 0x08, 0x0c, 0x10},
},
{
name: "[]int{1 << 32, 2, 3, 1 << 32}",
in: []int{1 << 32, 2, 3, 1 << 32},
want: []byte{0x10, 0x07, 0x00, 0x00, 0x00, 0x00, 0x01, 0x08, 0x0c, 0x07, 0x00, 0x00, 0x00, 0x00, 0x01},
name: "[]int64{1 << 32, 2, 3, 1 << 32}",
in: []int64{1 << 32, 2, 3, 1 << 32},
want: []byte{0x10, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,
0x00},
Comment on lines +828 to +832
Copy link
Member Author

@edwardmack edwardmack Jul 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
	}

Copy link
Contributor

@timwu20 timwu20 Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
			};
		}
	}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, thanks.

},
{
name: "[]bool{true, false, true}",
Expand Down Expand Up @@ -863,9 +870,11 @@ var (
want: []byte{0x03, 0x00, 0x00, 0x00, 0x40, 0x08, 0x0c, 0x10},
},
{
name: "[4]int{1 << 32, 2, 3, 1 << 32}",
in: [4]int{1 << 32, 2, 3, 1 << 32},
want: []byte{0x07, 0x00, 0x00, 0x00, 0x00, 0x01, 0x08, 0x0c, 0x07, 0x00, 0x00, 0x00, 0x00, 0x01},
name: "[4]int64{1 << 32, 2, 3, 1 << 32}",
in: [4]int64{1 << 32, 2, 3, 1 << 32},
want: []byte{0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,
0x00},
Comment on lines +873 to +877
Copy link
Member Author

@edwardmack edwardmack Jul 22, 2022 •
edited by qdm12
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
	}

},
{
name: "[3]bool{true, false, true}",
Expand Down