feat(scale): implement max length constraint for decoding bytes #2676

edwardmack · 2022-07-15T18:30:25Z

Changes

Add maxLength constraint to decodeState where error is raised if decodeState.decodeLength is greater than decodeState.maxLength to avoid trying to decode very long bytes values.
Add constant defaultMaxLength of 16 GB to use as default maxLength value, so that decoder will not attempt to decode bytes encoded with a length of greater than 16 GB.
Updated function NewDecoder and Unmarshal to accept variadic option, following pattern explained in https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis
Added function OptionMaxLength for overriding default maxLength
Added unit tests for testing use of OptionMaxLength with NewDecoder and Unmarshal
Added unit tests for testing decoding bytes with length greater than defaultMaxLength

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

codecov · 2022-07-15T18:54:50Z

Codecov Report

Merging #2676 (8bc1697) into development (3a471d9) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           development    #2676      +/-   ##
===============================================
+ Coverage        62.98%   62.99%   +0.01%     
===============================================
  Files              211      211              
  Lines            27136    27155      +19     
===============================================
+ Hits             17091    17107      +16     
- Misses            8482     8484       +2     
- Partials          1563     1564       +1

Flag	Coverage Δ
unit-tests	`62.99% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/scale/decode.go	`66.55% <100.00%> (+1.14%)`	⬆️
dot/network/inbound.go	`92.98% <0.00%> (-7.02%)`	⬇️
dot/network/block_announce.go	`58.40% <0.00%> (-4.81%)`	⬇️
lib/runtime/wasmer/imports.go	`55.26% <0.00%> (+0.05%)`	⬆️
dot/network/message.go	`68.39% <0.00%> (+2.59%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a471d9...8bc1697. Read the comment docs.

qdm12 · 2022-07-15T19:17:04Z

pkg/scale/decode.go

@@ -56,8 +56,10 @@ func indirect(dstv reflect.Value) (elem reflect.Value) {
 	return
 }

+const defaultMaxLength = 16 * 1024 * 1024 * 1024 // 16GB


I would suggest we don't limit the decoding by default. I think that's also why the scale spec doesn't specify a limit, since 20GB of scale data is still, well, valid.

Also if the writer is a file, 16GB+ is fine. The problem is with memory, depending on the environment.

qdm12 · 2022-07-15T19:18:45Z

pkg/scale/decode.go

 // Unmarshal takes data and a destination pointer to unmarshal the data to.
-func Unmarshal(data []byte, dst interface{}) (err error) {
+func Unmarshal(data []byte, dst interface{}, options ...func(*Decoder)) (err error) {


You should define a type type Option func(decoder *Decoder) at global scope

Actually that option function should return an error in case of future conflicting options for example (since it's meant to be a stable package go API). The error should be checked in the for _, option := range options { loop

qdm12 · 2022-07-15T19:20:42Z

pkg/scale/decode.go

 	}
 	return
 }

+// OptionMaxLength sets the maximum length of the data to be decoded
+func OptionMaxLength(maxLength int) func(*Decoder) {


make this argument a uint, to ensure the user cannot plug in negative values.

qdm12 · 2022-07-15T19:22:14Z

pkg/scale/decode.go

 type decodeState struct {
 	io.Reader
+	maxLength int


That's gonna be a niche nit...

getting the size through file.Stat() in Go returns a int64;
maybe we should have this maxLength as int64 if i.e. the writer is a file, and the OS is 32 bit (then int = int32)

qdm12 · 2022-07-15T19:22:57Z

pkg/scale/decode.go

@@ -540,6 +562,9 @@ func (ds *decodeState) decodeBytes(dstv reflect.Value) (err error) {
 	if err != nil {
 		return
 	}
+	if length > ds.maxLength {
+		return fmt.Errorf("could not decode length %v greater than max length %v", length, ds.maxLength)


make this a sentinel error. I understand this package doesn't use sentinel errors, but that would help one more step for #2631

qdm12 · 2022-07-15T19:23:29Z

pkg/scale/decode.go

+	return func(options *Decoder) {
+		options.maxLength = maxLength
+	}


an error would be useful here to prevent the user from settings a limit of 0 for example here.

qdm12 · 2022-07-15T19:24:40Z

pkg/scale/decode_test.go

+			if !reflect.DeepEqual(dst, tt.in) {
+				t.Errorf("decodeState.unmarshal() = %#v, want %#v", dst, tt.in)
+			}


Use assert.Equal

(also the t.Errorf message is wrong here)

qdm12 · 2022-07-15T19:26:14Z

pkg/scale/decode_test.go

@@ -70,6 +71,103 @@ func Test_decodeState_decodeBytes(t *testing.T) {
 	}
 }

+var maxLengthDecodeTests = tests{


make this a function returning a tests, so we can be (more) sure there is no funny mutation between each test using it.

qdm12 · 2022-07-15T19:26:32Z

pkg/scale/decode_test.go

+}
+
+func Test_decodeState_decodeBytes_maxLength(t *testing.T) {
+


Suggested change

t.Parallel()

kishansagathiya

Do we have places where Unmarshal with options is getting used?
I like the idea of option function. It is helpful if we have a bunch of options.

kishansagathiya · 2022-07-18T06:39:08Z

pkg/scale/decode.go

-func NewDecoder(r io.Reader) (d *Decoder) {
-	d = &Decoder{
-		decodeState{r},
+func NewDecoder(r io.Reader, options ...func(*Decoder)) (d *Decoder) {


What is the option here for exactly?
Are they like helper functions for decoding? In which case, may be they could be passed only here and be saved as property of decoder and use them in the Decode function. If that makes sense, decode function would not need to be passed options.

That makes sense. Although you would still have to pass options to scale.Unmarshal which creates its decoder within AFAIK.

edwardmack · 2022-07-20T14:17:27Z

Closed, replaced by PR #2683

implement max length constraint for decoding

8bc1697

edwardmack requested a review from noot as a code owner July 15, 2022 18:30

edwardmack self-assigned this Jul 15, 2022

edwardmack requested review from timwu20, EclesioMeloJunior, jimjbrettj, kishansagathiya and qdm12 as code owners July 15, 2022 18:30

qdm12 reviewed Jul 15, 2022

View reviewed changes

kishansagathiya reviewed Jul 18, 2022

View reviewed changes

edwardmack closed this Jul 20, 2022

edwardmack deleted the ed/scale_memory_bug branch November 11, 2022 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(scale): implement max length constraint for decoding bytes #2676

feat(scale): implement max length constraint for decoding bytes #2676

edwardmack commented Jul 15, 2022

codecov bot commented Jul 15, 2022

qdm12 Jul 15, 2022 •

edited

Loading

qdm12 Jul 15, 2022

qdm12 Jul 15, 2022

qdm12 Jul 15, 2022

qdm12 Jul 15, 2022

qdm12 Jul 15, 2022

qdm12 Jul 15, 2022

qdm12 Jul 15, 2022

qdm12 Jul 15, 2022

kishansagathiya left a comment

kishansagathiya Jul 18, 2022

qdm12 Jul 18, 2022

edwardmack commented Jul 20, 2022

		}

		func Test_decodeState_decodeBytes_maxLength(t *testing.T) {

feat(scale): implement max length constraint for decoding bytes #2676

feat(scale): implement max length constraint for decoding bytes #2676

Conversation

edwardmack commented Jul 15, 2022

Changes

Tests

Issues

Primary Reviewer

codecov bot commented Jul 15, 2022

Codecov Report

qdm12 Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishansagathiya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardmack commented Jul 20, 2022

qdm12 Jul 15, 2022 •

edited

Loading