From 6879cbcdcc6905533d74bf6e29363aba187aa221 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 6 Jul 2021 17:10:31 +0300 Subject: [PATCH 1/9] stackitem: properly detect recursive references during serialization If we're done with element it no longer can lead to recursion error, so fix cases like `[arr, arr]` where we have two copies of `arr` trigger this error for no good reason (there is no recursion there). --- pkg/vm/stackitem/json.go | 2 ++ pkg/vm/stackitem/serialization.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/pkg/vm/stackitem/json.go b/pkg/vm/stackitem/json.go index ae934ba0e..292b6936a 100644 --- a/pkg/vm/stackitem/json.go +++ b/pkg/vm/stackitem/json.go @@ -241,6 +241,7 @@ func toJSONWithTypes(item Item, seen map[Item]bool) (interface{}, error) { arr = append(arr, s) } value = arr + delete(seen, item) case *Bool: value = it.value case *Buffer, *ByteArray: @@ -266,6 +267,7 @@ func toJSONWithTypes(item Item, seen map[Item]bool) (interface{}, error) { }) } value = arr + delete(seen, item) case *Pointer: value = it.pos } diff --git a/pkg/vm/stackitem/serialization.go b/pkg/vm/stackitem/serialization.go index b30cb02c1..bb802dd36 100644 --- a/pkg/vm/stackitem/serialization.go +++ b/pkg/vm/stackitem/serialization.go @@ -82,6 +82,7 @@ func serializeItemTo(item Item, w *io.BinWriter, allowInvalid bool, seen map[Ite for i := range arr { serializeItemTo(arr[i], w, allowInvalid, seen) } + delete(seen, item) case *Map: seen[item] = true @@ -91,6 +92,7 @@ func serializeItemTo(item Item, w *io.BinWriter, allowInvalid bool, seen map[Ite serializeItemTo(t.Value().([]MapElement)[i].Key, w, allowInvalid, seen) serializeItemTo(t.Value().([]MapElement)[i].Value, w, allowInvalid, seen) } + delete(seen, item) case Null: w.WriteB(byte(AnyT)) } From e34fa2e915dc17bab23eba85ee12d8f1674009a0 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 6 Jul 2021 17:33:16 +0300 Subject: [PATCH 2/9] stackitem: limit JSONization nesting level We can have very deep reference types and attempt to JSONize them can easily lead to OOM (even though there are no recursive references inside). Therefore we have to limit them. While regular ToJSON() is buffer size limited to MaxSize, ToJSONWithTypes is not and limiting it to MaxSize output will require substantial rewriting effort while not really providing fair result, MaxSize is more about stack item size while its JSON representation can be much bigger because of various overheads. Initial thought was to limit it by element count based on MaxIteratorResultItems, but the problem here is that we don't always have this limit in contexts using ToJSONWithTypes (like notification event marshaling). Thus we need some generic limit which would be fine for all users. We at the same time have maxJSONDepth used when deserializing from JSON, so it can be used for marshaling as well, it's not often that we have deeper structures in real results. Inspired by neo-project/neo#2521. --- pkg/vm/stackitem/json.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/vm/stackitem/json.go b/pkg/vm/stackitem/json.go index 292b6936a..3d6abe9ed 100644 --- a/pkg/vm/stackitem/json.go +++ b/pkg/vm/stackitem/json.go @@ -221,6 +221,9 @@ func ToJSONWithTypes(item Item) ([]byte, error) { } func toJSONWithTypes(item Item, seen map[Item]bool) (interface{}, error) { + if len(seen) > maxJSONDepth { + return "", errors.New("too deep structure") + } typ := item.Type() result := map[string]interface{}{ "type": typ.String(), From 1b7b7e4bec003e61d28a4f3a3b1b77ee8edaff44 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 6 Jul 2021 17:51:46 +0300 Subject: [PATCH 3/9] stackitem: don't overwrite error when serializing Item We must get out as quickly as possible. --- pkg/vm/stackitem/serialization.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/vm/stackitem/serialization.go b/pkg/vm/stackitem/serialization.go index bb802dd36..960cb4252 100644 --- a/pkg/vm/stackitem/serialization.go +++ b/pkg/vm/stackitem/serialization.go @@ -39,6 +39,9 @@ func EncodeBinaryStackItemAppExec(item Item, w *io.BinWriter) { } func serializeItemTo(item Item, w *io.BinWriter, allowInvalid bool, seen map[Item]bool) { + if w.Err != nil { + return + } if seen[item] { w.Err = errors.New("recursive structures can't be serialized") return From 8472064bbc30ecd6583646ec301a82caead2bd42 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 6 Jul 2021 19:32:52 +0300 Subject: [PATCH 4/9] stackitem: refactor serialization code, add explicit context --- pkg/vm/stackitem/serialization.go | 54 ++++++++++++++++++++++--------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/pkg/vm/stackitem/serialization.go b/pkg/vm/stackitem/serialization.go index 960cb4252..93b625095 100644 --- a/pkg/vm/stackitem/serialization.go +++ b/pkg/vm/stackitem/serialization.go @@ -9,10 +9,22 @@ import ( "github.com/nspcc-dev/neo-go/pkg/io" ) +// serContext is an internal serialization context. +type serContext struct { + *io.BinWriter + allowInvalid bool + seen map[Item]bool +} + // SerializeItem encodes given Item into the byte slice. func SerializeItem(item Item) ([]byte, error) { w := io.NewBufBinWriter() - EncodeBinaryStackItem(item, w.BinWriter) + sc := serContext{ + BinWriter: w.BinWriter, + allowInvalid: false, + seen: make(map[Item]bool), + } + sc.serialize(item) if w.Err != nil { return nil, w.Err } @@ -23,14 +35,24 @@ func SerializeItem(item Item) ([]byte, error) { // similar to io.Serializable's EncodeBinary, but works with Item // interface. func EncodeBinaryStackItem(item Item, w *io.BinWriter) { - serializeItemTo(item, w, false, make(map[Item]bool)) + sc := serContext{ + BinWriter: w, + allowInvalid: false, + seen: make(map[Item]bool), + } + sc.serialize(item) } // EncodeBinaryStackItemAppExec encodes given Item into the given BinWriter. It's // similar to EncodeBinaryStackItem but allows to encode interop (only type, value is lost). func EncodeBinaryStackItemAppExec(item Item, w *io.BinWriter) { bw := io.NewBufBinWriter() - serializeItemTo(item, bw.BinWriter, true, make(map[Item]bool)) + sc := serContext{ + BinWriter: bw.BinWriter, + allowInvalid: true, + seen: make(map[Item]bool), + } + sc.serialize(item) if bw.Err != nil { w.WriteBytes([]byte{byte(InvalidT)}) return @@ -38,15 +60,15 @@ func EncodeBinaryStackItemAppExec(item Item, w *io.BinWriter) { w.WriteBytes(bw.Bytes()) } -func serializeItemTo(item Item, w *io.BinWriter, allowInvalid bool, seen map[Item]bool) { +func (w *serContext) serialize(item Item) { if w.Err != nil { return } - if seen[item] { + if w.seen[item] { w.Err = errors.New("recursive structures can't be serialized") return } - if item == nil && allowInvalid { + if item == nil && w.allowInvalid { w.WriteBytes([]byte{byte(InvalidT)}) return } @@ -65,13 +87,13 @@ func serializeItemTo(item Item, w *io.BinWriter, allowInvalid bool, seen map[Ite w.WriteBytes([]byte{byte(IntegerT)}) w.WriteVarBytes(bigint.ToBytes(t.Value().(*big.Int))) case *Interop: - if allowInvalid { + if w.allowInvalid { w.WriteBytes([]byte{byte(InteropT)}) - return + } else { + w.Err = errors.New("interop item can't be serialized") } - w.Err = errors.New("interop item can't be serialized") case *Array, *Struct: - seen[item] = true + w.seen[item] = true _, isArray := t.(*Array) if isArray { @@ -83,19 +105,19 @@ func serializeItemTo(item Item, w *io.BinWriter, allowInvalid bool, seen map[Ite arr := t.Value().([]Item) w.WriteVarUint(uint64(len(arr))) for i := range arr { - serializeItemTo(arr[i], w, allowInvalid, seen) + w.serialize(arr[i]) } - delete(seen, item) + delete(w.seen, item) case *Map: - seen[item] = true + w.seen[item] = true w.WriteBytes([]byte{byte(MapT)}) w.WriteVarUint(uint64(len(t.Value().([]MapElement)))) for i := range t.Value().([]MapElement) { - serializeItemTo(t.Value().([]MapElement)[i].Key, w, allowInvalid, seen) - serializeItemTo(t.Value().([]MapElement)[i].Value, w, allowInvalid, seen) + w.serialize(t.Value().([]MapElement)[i].Key) + w.serialize(t.Value().([]MapElement)[i].Value) } - delete(seen, item) + delete(w.seen, item) case Null: w.WriteB(byte(AnyT)) } From b9ff07f32c18b125d140c9abbba66157a5d78acf Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 6 Jul 2021 19:34:02 +0300 Subject: [PATCH 5/9] stackitem: add limit to serialized items Standard binary serialization/deserialization is mostly used in VM to put/get elements into/from storage, so they never should exceed MaxSize (otherwise one won't be able to deserialize these items). This patch leaves EncodeBinaryStackItem unprotected, but that's a streaming interface, so it's up to the user of it to ensure its appropriate use (and our uses are mostly for native contract's data, so they're fine). --- pkg/vm/stackitem/serialization.go | 17 +++++++++++++---- pkg/vm/stackitem/serialization_test.go | 21 +++++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 pkg/vm/stackitem/serialization_test.go diff --git a/pkg/vm/stackitem/serialization.go b/pkg/vm/stackitem/serialization.go index 93b625095..f121a3659 100644 --- a/pkg/vm/stackitem/serialization.go +++ b/pkg/vm/stackitem/serialization.go @@ -12,6 +12,7 @@ import ( // serContext is an internal serialization context. type serContext struct { *io.BinWriter + buf *io.BufBinWriter allowInvalid bool seen map[Item]bool } @@ -21,6 +22,7 @@ func SerializeItem(item Item) ([]byte, error) { w := io.NewBufBinWriter() sc := serContext{ BinWriter: w.BinWriter, + buf: w, allowInvalid: false, seen: make(map[Item]bool), } @@ -49,6 +51,7 @@ func EncodeBinaryStackItemAppExec(item Item, w *io.BinWriter) { bw := io.NewBufBinWriter() sc := serContext{ BinWriter: bw.BinWriter, + buf: bw, allowInvalid: true, seen: make(map[Item]bool), } @@ -68,10 +71,6 @@ func (w *serContext) serialize(item Item) { w.Err = errors.New("recursive structures can't be serialized") return } - if item == nil && w.allowInvalid { - w.WriteBytes([]byte{byte(InvalidT)}) - return - } switch t := item.(type) { case *ByteArray: @@ -120,6 +119,16 @@ func (w *serContext) serialize(item Item) { delete(w.seen, item) case Null: w.WriteB(byte(AnyT)) + case nil: + if w.allowInvalid { + w.WriteBytes([]byte{byte(InvalidT)}) + } else { + w.Err = errors.New("invalid stack item") + } + } + + if w.Err == nil && w.buf != nil && w.buf.Len() > MaxSize { + w.Err = errors.New("too big item") } } diff --git a/pkg/vm/stackitem/serialization_test.go b/pkg/vm/stackitem/serialization_test.go new file mode 100644 index 000000000..26000a94a --- /dev/null +++ b/pkg/vm/stackitem/serialization_test.go @@ -0,0 +1,21 @@ +package stackitem + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestSerializationMaxErr(t *testing.T) { + base := make([]byte, MaxSize/2+1) + item := Make(base) + + arr := []Item{item, item.Dup()} + aitem := Make(arr) + + _, err := SerializeItem(item) + require.NoError(t, err) + + _, err = SerializeItem(aitem) + require.Error(t, err) +} From 0de949b575afe3d81f5dfe5aaaef31baf645ed42 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 6 Jul 2021 19:56:23 +0300 Subject: [PATCH 6/9] stackitem: remove Item/StackItem from function names They're useless in a package named 'stackitem', make this package a bit more user-friendly. --- pkg/core/interop/runtime/engine.go | 2 +- pkg/core/interop/storage/find.go | 2 +- pkg/core/interop_system_test.go | 6 +-- pkg/core/native/native_neo.go | 2 +- pkg/core/native/native_neo_candidate.go | 4 +- pkg/core/native/neo_types.go | 4 +- pkg/core/native/oracle.go | 4 +- pkg/core/native/oracle_types.go | 8 ++-- pkg/core/native/oracle_types_test.go | 2 +- pkg/core/native/std.go | 4 +- pkg/core/native/std_test.go | 4 +- pkg/core/native_management_test.go | 8 ++-- pkg/core/native_oracle_test.go | 2 +- pkg/core/state/contract.go | 4 +- pkg/core/state/native_state.go | 8 ++-- pkg/core/state/notification_event.go | 8 ++-- pkg/core/state/notification_event_test.go | 2 +- pkg/core/state/oracle.go | 4 +- pkg/core/state/oracle_test.go | 4 +- pkg/vm/stackitem/serialization.go | 45 ++++++++++++----------- pkg/vm/stackitem/serialization_test.go | 4 +- 21 files changed, 67 insertions(+), 64 deletions(-) diff --git a/pkg/core/interop/runtime/engine.go b/pkg/core/interop/runtime/engine.go index 8bb3f8e19..fe25c8b77 100644 --- a/pkg/core/interop/runtime/engine.go +++ b/pkg/core/interop/runtime/engine.go @@ -61,7 +61,7 @@ func Notify(ic *interop.Context) error { // But it has to be serializable, otherwise we either have some broken // (recursive) structure inside or an interop item that can't be used // outside of the interop subsystem anyway. - bytes, err := stackitem.SerializeItem(elem.Item()) + bytes, err := stackitem.Serialize(elem.Item()) if err != nil { return fmt.Errorf("bad notification: %w", err) } diff --git a/pkg/core/interop/storage/find.go b/pkg/core/interop/storage/find.go index a969e1077..045c9290f 100644 --- a/pkg/core/interop/storage/find.go +++ b/pkg/core/interop/storage/find.go @@ -57,7 +57,7 @@ func (s *Iterator) Value() stackitem.Item { if s.opts&FindDeserialize != 0 { bs := s.m[s.index].Value.Value().([]byte) var err error - value, err = stackitem.DeserializeItem(bs) + value, err = stackitem.Deserialize(bs) if err != nil { panic(err) } diff --git a/pkg/core/interop_system_test.go b/pkg/core/interop_system_test.go index 298bf29e9..a64b45aa1 100644 --- a/pkg/core/interop_system_test.go +++ b/pkg/core/interop_system_test.go @@ -266,11 +266,11 @@ func TestStorageFind(t *testing.T) { stackitem.NewByteArray([]byte("second")), stackitem.Null{}, } - rawArr, err := stackitem.SerializeItem(stackitem.NewArray(arr)) + rawArr, err := stackitem.Serialize(stackitem.NewArray(arr)) require.NoError(t, err) - rawArr0, err := stackitem.SerializeItem(stackitem.NewArray(arr[:0])) + rawArr0, err := stackitem.Serialize(stackitem.NewArray(arr[:0])) require.NoError(t, err) - rawArr1, err := stackitem.SerializeItem(stackitem.NewArray(arr[:1])) + rawArr1, err := stackitem.Serialize(stackitem.NewArray(arr[:1])) require.NoError(t, err) skeys := [][]byte{{0x01, 0x02}, {0x02, 0x01}, {0x01, 0x01}, diff --git a/pkg/core/native/native_neo.go b/pkg/core/native/native_neo.go index b3ae87b85..dadeaee35 100644 --- a/pkg/core/native/native_neo.go +++ b/pkg/core/native/native_neo.go @@ -904,7 +904,7 @@ func (n *NEO) getAccountState(ic *interop.Context, args []stackitem.Item) stacki } r := io.NewBinReaderFromBuf(si) - item := stackitem.DecodeBinaryStackItem(r) + item := stackitem.DecodeBinary(r) if r.Err != nil { panic(r.Err) // no errors are expected but we better be sure } diff --git a/pkg/core/native/native_neo_candidate.go b/pkg/core/native/native_neo_candidate.go index 0fa014eb6..a1a7bb44f 100644 --- a/pkg/core/native/native_neo_candidate.go +++ b/pkg/core/native/native_neo_candidate.go @@ -15,14 +15,14 @@ type candidate struct { // Bytes marshals c to byte array. func (c *candidate) Bytes() []byte { w := io.NewBufBinWriter() - stackitem.EncodeBinaryStackItem(c.toStackItem(), w.BinWriter) + stackitem.EncodeBinary(c.toStackItem(), w.BinWriter) return w.Bytes() } // FromBytes unmarshals candidate from byte array. func (c *candidate) FromBytes(data []byte) *candidate { r := io.NewBinReaderFromBuf(data) - item := stackitem.DecodeBinaryStackItem(r) + item := stackitem.DecodeBinary(r) if r.Err != nil { panic(r.Err) } diff --git a/pkg/core/native/neo_types.go b/pkg/core/native/neo_types.go index aeefda5b9..15dcf1e97 100644 --- a/pkg/core/native/neo_types.go +++ b/pkg/core/native/neo_types.go @@ -86,7 +86,7 @@ func (k *keysWithVotes) fromStackItem(item stackitem.Item) error { func (k keysWithVotes) Bytes() []byte { var it = k.toStackItem() var w = io.NewBufBinWriter() - stackitem.EncodeBinaryStackItem(it, w.BinWriter) + stackitem.EncodeBinary(it, w.BinWriter) if w.Err != nil { panic(w.Err) } @@ -96,7 +96,7 @@ func (k keysWithVotes) Bytes() []byte { // DecodeBytes deserializes keys and votes slice. func (k *keysWithVotes) DecodeBytes(data []byte) error { var r = io.NewBinReaderFromBuf(data) - var it = stackitem.DecodeBinaryStackItem(r) + var it = stackitem.DecodeBinary(r) if r.Err != nil { return r.Err } diff --git a/pkg/core/native/oracle.go b/pkg/core/native/oracle.go index b6deb469c..7c500f6cf 100644 --- a/pkg/core/native/oracle.go +++ b/pkg/core/native/oracle.go @@ -278,7 +278,7 @@ func (o *Oracle) FinishInternal(ic *interop.Context) error { }) r := io.NewBinReaderFromBuf(req.UserData) - userData := stackitem.DecodeBinaryStackItem(r) + userData := stackitem.DecodeBinary(r) args := []stackitem.Item{ stackitem.Make(req.URL), stackitem.Make(userData), @@ -358,7 +358,7 @@ func (o *Oracle) RequestInternal(ic *interop.Context, url string, filter *string } w := io.NewBufBinWriter() - stackitem.EncodeBinaryStackItem(userData, w.BinWriter) + stackitem.EncodeBinary(userData, w.BinWriter) if w.Err != nil { return w.Err } diff --git a/pkg/core/native/oracle_types.go b/pkg/core/native/oracle_types.go index fc6c90145..985897e72 100644 --- a/pkg/core/native/oracle_types.go +++ b/pkg/core/native/oracle_types.go @@ -25,12 +25,12 @@ func (l IDList) Bytes() []byte { // EncodeBinary implements io.Serializable. func (l IDList) EncodeBinary(w *io.BinWriter) { - stackitem.EncodeBinaryStackItem(l.toStackItem(), w) + stackitem.EncodeBinary(l.toStackItem(), w) } // DecodeBinary implements io.Serializable. func (l *IDList) DecodeBinary(r *io.BinReader) { - item := stackitem.DecodeBinaryStackItem(r) + item := stackitem.DecodeBinary(r) if r.Err != nil || item == nil { return } @@ -84,12 +84,12 @@ func (l NodeList) Bytes() []byte { // EncodeBinary implements io.Serializable. func (l NodeList) EncodeBinary(w *io.BinWriter) { - stackitem.EncodeBinaryStackItem(l.toStackItem(), w) + stackitem.EncodeBinary(l.toStackItem(), w) } // DecodeBinary implements io.Serializable. func (l *NodeList) DecodeBinary(r *io.BinReader) { - item := stackitem.DecodeBinaryStackItem(r) + item := stackitem.DecodeBinary(r) if r.Err != nil || item == nil { return } diff --git a/pkg/core/native/oracle_types_test.go b/pkg/core/native/oracle_types_test.go index 78acf7bf2..c58323dd9 100644 --- a/pkg/core/native/oracle_types_test.go +++ b/pkg/core/native/oracle_types_test.go @@ -14,7 +14,7 @@ func getInvalidTestFunc(actual io.Serializable, value interface{}) func(t *testi return func(t *testing.T) { w := io.NewBufBinWriter() it := stackitem.Make(value) - stackitem.EncodeBinaryStackItem(it, w.BinWriter) + stackitem.EncodeBinary(it, w.BinWriter) require.NoError(t, w.Err) require.Error(t, testserdes.DecodeBinary(w.Bytes(), actual)) } diff --git a/pkg/core/native/std.go b/pkg/core/native/std.go index d27be752e..f2e614b0a 100644 --- a/pkg/core/native/std.go +++ b/pkg/core/native/std.go @@ -160,7 +160,7 @@ func newStd() *Std { } func (s *Std) serialize(_ *interop.Context, args []stackitem.Item) stackitem.Item { - data, err := stackitem.SerializeItem(args[0]) + data, err := stackitem.Serialize(args[0]) if err != nil { panic(err) } @@ -177,7 +177,7 @@ func (s *Std) deserialize(_ *interop.Context, args []stackitem.Item) stackitem.I panic(err) } - item, err := stackitem.DeserializeItem(data) + item, err := stackitem.Deserialize(data) if err != nil { panic(err) } diff --git a/pkg/core/native/std_test.go b/pkg/core/native/std_test.go index ce16eb01d..2558b5f2d 100644 --- a/pkg/core/native/std_test.go +++ b/pkg/core/native/std_test.go @@ -276,7 +276,7 @@ func TestStdLibSerialize(t *testing.T) { }) w := io.NewBufBinWriter() - stackitem.EncodeBinaryStackItem(stackitem.Make(42), w.BinWriter) + stackitem.EncodeBinary(stackitem.Make(42), w.BinWriter) require.NoError(t, w.Err) encoded := w.Bytes() @@ -368,7 +368,7 @@ func TestStdLibSerializeDeserialize(t *testing.T) { }) }) t.Run("Deserialize unknown", func(t *testing.T) { - data, err := stackitem.SerializeItem(stackitem.NewBigInteger(big.NewInt(123))) + data, err := stackitem.Serialize(stackitem.NewBigInteger(big.NewInt(123))) require.NoError(t, err) data[0] = 0xFF diff --git a/pkg/core/native_management_test.go b/pkg/core/native_management_test.go index 355a57e37..57ec4c54e 100644 --- a/pkg/core/native_management_test.go +++ b/pkg/core/native_management_test.go @@ -94,7 +94,7 @@ func TestContractDeployAndUpdateWithParameter(t *testing.T) { res, err := invokeContractMethod(bc, 1_00000000, cs1.Hash, "getValue") require.NoError(t, err) require.Equal(t, 1, len(res.Stack)) - item, err := stackitem.DeserializeItem(res.Stack[0].Value().([]byte)) + item, err := stackitem.Deserialize(res.Stack[0].Value().([]byte)) require.NoError(t, err) expected := []stackitem.Item{stackitem.Make("create"), stackitem.Make(42)} require.Equal(t, stackitem.NewArray(expected), item) @@ -114,7 +114,7 @@ func TestContractDeployAndUpdateWithParameter(t *testing.T) { res, err := invokeContractMethod(bc, 1_00000000, cs1.Hash, "getValue") require.NoError(t, err) require.Equal(t, 1, len(res.Stack)) - item, err := stackitem.DeserializeItem(res.Stack[0].Value().([]byte)) + item, err := stackitem.Deserialize(res.Stack[0].Value().([]byte)) require.NoError(t, err) expected := []stackitem.Item{stackitem.Make("update"), stackitem.Make("new data")} require.Equal(t, stackitem.NewArray(expected), item) @@ -259,7 +259,7 @@ func TestContractDeploy(t *testing.T) { res, err := invokeContractMethod(bc, 1_00000000, cs1.Hash, "getValue") require.NoError(t, err) require.Equal(t, 1, len(res.Stack)) - item, err := stackitem.DeserializeItem(res.Stack[0].Value().([]byte)) + item, err := stackitem.Deserialize(res.Stack[0].Value().([]byte)) require.NoError(t, err) expected := []stackitem.Item{stackitem.Make("create"), stackitem.Null{}} require.Equal(t, stackitem.NewArray(expected), item) @@ -466,7 +466,7 @@ func TestContractUpdate(t *testing.T) { res, err := invokeContractMethod(bc, 1_00000000, cs1.Hash, "getValue") require.NoError(t, err) require.Equal(t, 1, len(res.Stack)) - item, err := stackitem.DeserializeItem(res.Stack[0].Value().([]byte)) + item, err := stackitem.Deserialize(res.Stack[0].Value().([]byte)) require.NoError(t, err) expected := []stackitem.Item{stackitem.Make("update"), stackitem.Null{}} require.Equal(t, stackitem.NewArray(expected), item) diff --git a/pkg/core/native_oracle_test.go b/pkg/core/native_oracle_test.go index 3c56fb00a..660eb1ba7 100644 --- a/pkg/core/native_oracle_test.go +++ b/pkg/core/native_oracle_test.go @@ -183,7 +183,7 @@ func TestOracle_Request(t *testing.T) { si := ic.DAO.GetStorageItem(cs.ID, []byte("lastOracleResponse")) require.NotNil(t, si) - item, err := stackitem.DeserializeItem(si) + item, err := stackitem.Deserialize(si) require.NoError(t, err) arr, ok := item.Value().([]stackitem.Item) require.True(t, ok) diff --git a/pkg/core/state/contract.go b/pkg/core/state/contract.go index 3c6da196b..e6ee4e873 100644 --- a/pkg/core/state/contract.go +++ b/pkg/core/state/contract.go @@ -37,7 +37,7 @@ type NativeContract struct { // DecodeBinary implements Serializable interface. func (c *Contract) DecodeBinary(r *io.BinReader) { - si := stackitem.DecodeBinaryStackItem(r) + si := stackitem.DecodeBinary(r) if r.Err != nil { return } @@ -51,7 +51,7 @@ func (c *Contract) EncodeBinary(w *io.BinWriter) { w.Err = err return } - stackitem.EncodeBinaryStackItem(si, w) + stackitem.EncodeBinary(si, w) } // ToStackItem converts state.Contract to stackitem.Item. diff --git a/pkg/core/state/native_state.go b/pkg/core/state/native_state.go index 4b45239ab..75baee89d 100644 --- a/pkg/core/state/native_state.go +++ b/pkg/core/state/native_state.go @@ -58,12 +58,12 @@ func (s *NEP17BalanceState) fromStackItem(item stackitem.Item) { // EncodeBinary implements io.Serializable interface. func (s *NEP17BalanceState) EncodeBinary(w *io.BinWriter) { si := s.toStackItem() - stackitem.EncodeBinaryStackItem(si, w) + stackitem.EncodeBinary(si, w) } // DecodeBinary implements io.Serializable interface. func (s *NEP17BalanceState) DecodeBinary(r *io.BinReader) { - si := stackitem.DecodeBinaryStackItem(r) + si := stackitem.DecodeBinary(r) if r.Err != nil { return } @@ -98,12 +98,12 @@ func (s *NEOBalanceState) Bytes() []byte { // EncodeBinary implements io.Serializable interface. func (s *NEOBalanceState) EncodeBinary(w *io.BinWriter) { si := s.toStackItem() - stackitem.EncodeBinaryStackItem(si, w) + stackitem.EncodeBinary(si, w) } // DecodeBinary implements io.Serializable interface. func (s *NEOBalanceState) DecodeBinary(r *io.BinReader) { - si := stackitem.DecodeBinaryStackItem(r) + si := stackitem.DecodeBinary(r) if r.Err != nil { return } diff --git a/pkg/core/state/notification_event.go b/pkg/core/state/notification_event.go index a7c032b76..193fc8667 100644 --- a/pkg/core/state/notification_event.go +++ b/pkg/core/state/notification_event.go @@ -31,14 +31,14 @@ type AppExecResult struct { func (ne *NotificationEvent) EncodeBinary(w *io.BinWriter) { ne.ScriptHash.EncodeBinary(w) w.WriteString(ne.Name) - stackitem.EncodeBinaryStackItem(ne.Item, w) + stackitem.EncodeBinary(ne.Item, w) } // DecodeBinary implements the Serializable interface. func (ne *NotificationEvent) DecodeBinary(r *io.BinReader) { ne.ScriptHash.DecodeBinary(r) ne.Name = r.ReadString() - item := stackitem.DecodeBinaryStackItem(r) + item := stackitem.DecodeBinary(r) if r.Err != nil { return } @@ -59,7 +59,7 @@ func (aer *AppExecResult) EncodeBinary(w *io.BinWriter) { // Stack items are expected to be marshaled one by one. w.WriteVarUint(uint64(len(aer.Stack))) for _, it := range aer.Stack { - stackitem.EncodeBinaryStackItemAppExec(it, w) + stackitem.EncodeBinaryProtected(it, w) } w.WriteArray(aer.Events) w.WriteVarBytes([]byte(aer.FaultException)) @@ -80,7 +80,7 @@ func (aer *AppExecResult) DecodeBinary(r *io.BinReader) { } arr := make([]stackitem.Item, sz) for i := 0; i < int(sz); i++ { - arr[i] = stackitem.DecodeBinaryStackItemAppExec(r) + arr[i] = stackitem.DecodeBinaryProtected(r) if r.Err != nil { return } diff --git a/pkg/core/state/notification_event_test.go b/pkg/core/state/notification_event_test.go index 2429b7c81..55e36f3aa 100644 --- a/pkg/core/state/notification_event_test.go +++ b/pkg/core/state/notification_event_test.go @@ -77,7 +77,7 @@ func TestEncodeDecodeAppExecResult(t *testing.T) { w.WriteB(byte(aer.Trigger)) w.WriteB(byte(aer.VMState)) w.WriteU64LE(uint64(aer.GasConsumed)) - stackitem.EncodeBinaryStackItem(stackitem.NewBool(true), w.BinWriter) + stackitem.EncodeBinary(stackitem.NewBool(true), w.BinWriter) require.NoError(t, w.Err) require.Error(t, testserdes.DecodeBinary(w.Bytes(), new(AppExecResult))) }) diff --git a/pkg/core/state/oracle.go b/pkg/core/state/oracle.go index ac8fa114d..96efde901 100644 --- a/pkg/core/state/oracle.go +++ b/pkg/core/state/oracle.go @@ -30,12 +30,12 @@ func (o *OracleRequest) Bytes() []byte { // EncodeBinary implements io.Serializable. func (o *OracleRequest) EncodeBinary(w *io.BinWriter) { - stackitem.EncodeBinaryStackItem(o.toStackItem(), w) + stackitem.EncodeBinary(o.toStackItem(), w) } // DecodeBinary implements io.Serializable. func (o *OracleRequest) DecodeBinary(r *io.BinReader) { - item := stackitem.DecodeBinaryStackItem(r) + item := stackitem.DecodeBinary(r) if r.Err != nil || item == nil { return } diff --git a/pkg/core/state/oracle_test.go b/pkg/core/state/oracle_test.go index dc0a9ed12..bb17ee6bc 100644 --- a/pkg/core/state/oracle_test.go +++ b/pkg/core/state/oracle_test.go @@ -34,7 +34,7 @@ func TestOracleRequest_EncodeBinary(t *testing.T) { t.Run("NotArray", func(t *testing.T) { w.Reset() it := stackitem.NewByteArray([]byte{}) - stackitem.EncodeBinaryStackItem(it, w.BinWriter) + stackitem.EncodeBinary(it, w.BinWriter) require.Error(t, testserdes.DecodeBinary(w.Bytes(), new(OracleRequest))) }) t.Run("NotStackItem", func(t *testing.T) { @@ -57,7 +57,7 @@ func TestOracleRequest_EncodeBinary(t *testing.T) { w.Reset() before := items[i] items[i] = elem - stackitem.EncodeBinaryStackItem(arrItem, w.BinWriter) + stackitem.EncodeBinary(arrItem, w.BinWriter) items[i] = before require.Error(t, testserdes.DecodeBinary(w.Bytes(), new(OracleRequest))) } diff --git a/pkg/vm/stackitem/serialization.go b/pkg/vm/stackitem/serialization.go index f121a3659..f668522a6 100644 --- a/pkg/vm/stackitem/serialization.go +++ b/pkg/vm/stackitem/serialization.go @@ -17,8 +17,8 @@ type serContext struct { seen map[Item]bool } -// SerializeItem encodes given Item into the byte slice. -func SerializeItem(item Item) ([]byte, error) { +// Serialize encodes given Item into the byte slice. +func Serialize(item Item) ([]byte, error) { w := io.NewBufBinWriter() sc := serContext{ BinWriter: w.BinWriter, @@ -33,10 +33,10 @@ func SerializeItem(item Item) ([]byte, error) { return w.Bytes(), nil } -// EncodeBinaryStackItem encodes given Item into the given BinWriter. It's +// EncodeBinary encodes given Item into the given BinWriter. It's // similar to io.Serializable's EncodeBinary, but works with Item // interface. -func EncodeBinaryStackItem(item Item, w *io.BinWriter) { +func EncodeBinary(item Item, w *io.BinWriter) { sc := serContext{ BinWriter: w, allowInvalid: false, @@ -45,9 +45,12 @@ func EncodeBinaryStackItem(item Item, w *io.BinWriter) { sc.serialize(item) } -// EncodeBinaryStackItemAppExec encodes given Item into the given BinWriter. It's -// similar to EncodeBinaryStackItem but allows to encode interop (only type, value is lost). -func EncodeBinaryStackItemAppExec(item Item, w *io.BinWriter) { +// EncodeBinaryProtected encodes given Item into the given BinWriter. It's +// similar to EncodeBinary but allows to encode interop items (only type, +// value is lost) and doesn't return any errors in w, instead if error +// (like recursive array) is encountered it just writes special InvalidT +// type of element to w. +func EncodeBinaryProtected(item Item, w *io.BinWriter) { bw := io.NewBufBinWriter() sc := serContext{ BinWriter: bw.BinWriter, @@ -132,31 +135,31 @@ func (w *serContext) serialize(item Item) { } } -// DeserializeItem decodes Item from the given byte slice. -func DeserializeItem(data []byte) (Item, error) { +// Deserialize decodes Item from the given byte slice. +func Deserialize(data []byte) (Item, error) { r := io.NewBinReaderFromBuf(data) - item := DecodeBinaryStackItem(r) + item := DecodeBinary(r) if r.Err != nil { return nil, r.Err } return item, nil } -// DecodeBinaryStackItem decodes previously serialized Item from the given +// DecodeBinary decodes previously serialized Item from the given // reader. It's similar to the io.Serializable's DecodeBinary(), but implemented // as a function because Item itself is an interface. Caveat: always check // reader's error value before using the returned Item. -func DecodeBinaryStackItem(r *io.BinReader) Item { - return decodeBinaryStackItem(r, false) +func DecodeBinary(r *io.BinReader) Item { + return decodeBinary(r, false) } -// DecodeBinaryStackItemAppExec is similar to DecodeBinaryStackItem -// but allows Interop values to be present. -func DecodeBinaryStackItemAppExec(r *io.BinReader) Item { - return decodeBinaryStackItem(r, true) +// DecodeBinaryProtected is similar to DecodeBinary but allows Interop and +// Invalid values to be present (making it symmetric to EncodeBinaryProtected). +func DecodeBinaryProtected(r *io.BinReader) Item { + return decodeBinary(r, true) } -func decodeBinaryStackItem(r *io.BinReader, allowInvalid bool) Item { +func decodeBinary(r *io.BinReader, allowInvalid bool) Item { var t = Type(r.ReadB()) if r.Err != nil { return nil @@ -180,7 +183,7 @@ func decodeBinaryStackItem(r *io.BinReader, allowInvalid bool) Item { size := int(r.ReadVarUint()) arr := make([]Item, size) for i := 0; i < size; i++ { - arr[i] = DecodeBinaryStackItem(r) + arr[i] = DecodeBinary(r) } if t == ArrayT { @@ -191,8 +194,8 @@ func decodeBinaryStackItem(r *io.BinReader, allowInvalid bool) Item { size := int(r.ReadVarUint()) m := NewMap() for i := 0; i < size; i++ { - key := DecodeBinaryStackItem(r) - value := DecodeBinaryStackItem(r) + key := DecodeBinary(r) + value := DecodeBinary(r) if r.Err != nil { break } diff --git a/pkg/vm/stackitem/serialization_test.go b/pkg/vm/stackitem/serialization_test.go index 26000a94a..f3d58aa96 100644 --- a/pkg/vm/stackitem/serialization_test.go +++ b/pkg/vm/stackitem/serialization_test.go @@ -13,9 +13,9 @@ func TestSerializationMaxErr(t *testing.T) { arr := []Item{item, item.Dup()} aitem := Make(arr) - _, err := SerializeItem(item) + _, err := Serialize(item) require.NoError(t, err) - _, err = SerializeItem(aitem) + _, err = Serialize(aitem) require.Error(t, err) } From 5a9efcc6543dec421404e5c2aa0b9ff5f8723815 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Wed, 7 Jul 2021 00:18:00 +0300 Subject: [PATCH 7/9] stackitem: rework error handling Return good named errors everywhere, export appropriate constants, make errors.Is() work. --- pkg/vm/stackitem/item.go | 86 +++++++++++++++----------- pkg/vm/stackitem/json.go | 60 +++++++++++------- pkg/vm/stackitem/serialization.go | 18 ++++-- pkg/vm/stackitem/serialization_test.go | 6 +- pkg/vm/stackitem/type.go | 5 +- 5 files changed, 108 insertions(+), 67 deletions(-) diff --git a/pkg/vm/stackitem/item.go b/pkg/vm/stackitem/item.go index a8af99573..db2c1cac4 100644 --- a/pkg/vm/stackitem/item.go +++ b/pkg/vm/stackitem/item.go @@ -53,10 +53,28 @@ type Item interface { } var ( - errInvalidConversion = errors.New("invalid conversion type") - errExceedingMaxComparableSize = errors.New("the operand exceeds the maximum comparable size") + // ErrInvalidConversion is returned on attempt to make an incorrect + // conversion between item types. + ErrInvalidConversion = errors.New("invalid conversion") + + // ErrTooBig is returned when item exceeds some size constraints like + // maximum allowed integer value of number of elements in array. It + // can also be returned by serialization functions if resulting + // value exceeds MaxSize. + ErrTooBig = errors.New("too big") + + errTooBigComparable = fmt.Errorf("%w: uncomparable", ErrTooBig) + errTooBigInteger = fmt.Errorf("%w: integer", ErrTooBig) + errTooBigKey = fmt.Errorf("%w: map key", ErrTooBig) + errTooBigSize = fmt.Errorf("%w: size", ErrTooBig) ) +// mkInvConversion creates conversion error with additional metadata (from and +// to types). +func mkInvConversion(from Item, to Type) error { + return fmt.Errorf("%w: %s/%s", ErrInvalidConversion, from, to) +} + // Make tries to make appropriate stack item from provided value. // It will panic if it's not possible. func Make(v interface{}) Item { @@ -138,7 +156,7 @@ func ToString(item Item) (string, error) { return "", err } if !utf8.Valid(bs) { - return "", errors.New("not a valid UTF-8") + return "", fmt.Errorf("%w: not UTF-8", ErrInvalidValue) } return string(bs), nil } @@ -174,7 +192,7 @@ func convertPrimitive(item Item, typ Type) (Item, error) { } return NewBool(b), nil default: - return nil, errInvalidConversion + return nil, mkInvConversion(item, typ) } } @@ -232,12 +250,12 @@ func (i *Struct) TryBool() (bool, error) { return true, nil } // TryBytes implements Item interface. func (i *Struct) TryBytes() ([]byte, error) { - return nil, errors.New("can't convert Struct to ByteString") + return nil, mkInvConversion(i, ByteArrayT) } // TryInteger implements Item interface. func (i *Struct) TryInteger() (*big.Int, error) { - return nil, errors.New("can't convert Struct to Integer") + return nil, mkInvConversion(i, IntegerT) } // Equals implements Item interface. @@ -274,7 +292,7 @@ func (i *Struct) Convert(typ Type) (Item, error) { case BooleanT: return NewBool(true), nil default: - return nil, errInvalidConversion + return nil, mkInvConversion(i, typ) } } @@ -318,12 +336,12 @@ func (i Null) TryBool() (bool, error) { return false, nil } // TryBytes implements Item interface. func (i Null) TryBytes() ([]byte, error) { - return nil, errors.New("can't convert Null to ByteString") + return nil, mkInvConversion(i, ByteArrayT) } // TryInteger implements Item interface. func (i Null) TryInteger() (*big.Int, error) { - return nil, errors.New("can't convert Null to Integer") + return nil, mkInvConversion(i, IntegerT) } // Equals implements Item interface. @@ -338,7 +356,7 @@ func (i Null) Type() Type { return AnyT } // Convert implements Item interface. func (i Null) Convert(typ Type) (Item, error) { if typ == AnyT || !typ.IsValid() { - return nil, errInvalidConversion + return nil, mkInvConversion(i, typ) } return i, nil } @@ -350,18 +368,16 @@ type BigInteger struct { // NewBigInteger returns an new BigInteger object. func NewBigInteger(value *big.Int) *BigInteger { - const tooBigErrMsg = "integer is too big" - // There are 2 cases, when `BitLen` differs from actual size: // 1. Positive integer with highest bit on byte boundary = 1. // 2. Negative integer with highest bit on byte boundary = 1 // minus some value. (-0x80 -> 0x80, -0x7F -> 0x81, -0x81 -> 0x7FFF). sz := value.BitLen() if sz > MaxBigIntegerSizeBits { - panic(tooBigErrMsg) + panic(errTooBigInteger) } else if sz == MaxBigIntegerSizeBits { if value.Sign() == 1 || value.TrailingZeroBits() != MaxBigIntegerSizeBits-1 { - panic(tooBigErrMsg) + panic(errTooBigInteger) } } return &BigInteger{ @@ -531,7 +547,7 @@ func (i *ByteArray) String() string { // TryBool implements Item interface. func (i *ByteArray) TryBool() (bool, error) { if len(i.value) > MaxBigIntegerSizeBits/8 { - return false, errors.New("too big byte string") + return false, errTooBigInteger } for _, b := range i.value { if b != 0 { @@ -549,7 +565,7 @@ func (i *ByteArray) TryBytes() ([]byte, error) { // TryInteger implements Item interface. func (i *ByteArray) TryInteger() (*big.Int, error) { if len(i.value) > MaxBigIntegerSizeBits/8 { - return nil, errors.New("integer is too big") + return nil, errTooBigInteger } return bigint.FromBytes(i.value), nil } @@ -557,7 +573,7 @@ func (i *ByteArray) TryInteger() (*big.Int, error) { // Equals implements Item interface. func (i *ByteArray) Equals(s Item) bool { if len(i.value) > MaxByteArrayComparableSize { - panic(errExceedingMaxComparableSize) + panic(errTooBigComparable) } if i == s { return true @@ -569,7 +585,7 @@ func (i *ByteArray) Equals(s Item) bool { return false } if len(val.value) > MaxByteArrayComparableSize { - panic(errExceedingMaxComparableSize) + panic(errTooBigComparable) } return bytes.Equal(i.value, val.value) } @@ -641,12 +657,12 @@ func (i *Array) TryBool() (bool, error) { return true, nil } // TryBytes implements Item interface. func (i *Array) TryBytes() ([]byte, error) { - return nil, errors.New("can't convert Array to ByteString") + return nil, mkInvConversion(i, ByteArrayT) } // TryInteger implements Item interface. func (i *Array) TryInteger() (*big.Int, error) { - return nil, errors.New("can't convert Array to Integer") + return nil, mkInvConversion(i, IntegerT) } // Equals implements Item interface. @@ -675,7 +691,7 @@ func (i *Array) Convert(typ Type) (Item, error) { case BooleanT: return NewBool(true), nil default: - return nil, errInvalidConversion + return nil, mkInvConversion(i, typ) } } @@ -731,12 +747,12 @@ func (i *Map) TryBool() (bool, error) { return true, nil } // TryBytes implements Item interface. func (i *Map) TryBytes() ([]byte, error) { - return nil, errors.New("can't convert Map to ByteString") + return nil, mkInvConversion(i, ByteArrayT) } // TryInteger implements Item interface. func (i *Map) TryInteger() (*big.Int, error) { - return nil, errors.New("can't convert Map to Integer") + return nil, mkInvConversion(i, IntegerT) } // Equals implements Item interface. @@ -780,7 +796,7 @@ func (i *Map) Convert(typ Type) (Item, error) { case BooleanT: return NewBool(true), nil default: - return nil, errInvalidConversion + return nil, mkInvConversion(i, typ) } } @@ -812,11 +828,11 @@ func IsValidMapKey(key Item) error { case *ByteArray: size := len(key.Value().([]byte)) if size > MaxKeySize { - return fmt.Errorf("invalid map key size: %d", size) + return errTooBigKey } return nil default: - return fmt.Errorf("invalid map key of type %s", key.Type()) + return fmt.Errorf("%w: %s map key", ErrInvalidType, key.Type()) } } @@ -853,12 +869,12 @@ func (i *Interop) TryBool() (bool, error) { return true, nil } // TryBytes implements Item interface. func (i *Interop) TryBytes() ([]byte, error) { - return nil, errors.New("can't convert Interop to ByteString") + return nil, mkInvConversion(i, ByteArrayT) } // TryInteger implements Item interface. func (i *Interop) TryInteger() (*big.Int, error) { - return nil, errors.New("can't convert Interop to Integer") + return nil, mkInvConversion(i, IntegerT) } // Equals implements Item interface. @@ -883,7 +899,7 @@ func (i *Interop) Convert(typ Type) (Item, error) { case BooleanT: return NewBool(true), nil default: - return nil, errInvalidConversion + return nil, mkInvConversion(i, typ) } } @@ -946,12 +962,12 @@ func (p *Pointer) TryBool() (bool, error) { // TryBytes implements Item interface. func (p *Pointer) TryBytes() ([]byte, error) { - return nil, errors.New("can't convert Pointer to ByteString") + return nil, mkInvConversion(p, ByteArrayT) } // TryInteger implements Item interface. func (p *Pointer) TryInteger() (*big.Int, error) { - return nil, errors.New("can't convert Pointer to Integer") + return nil, mkInvConversion(p, IntegerT) } // Equals implements Item interface. @@ -976,7 +992,7 @@ func (p *Pointer) Convert(typ Type) (Item, error) { case BooleanT: return NewBool(true), nil default: - return nil, errInvalidConversion + return nil, mkInvConversion(p, typ) } } @@ -1024,7 +1040,7 @@ func (i *Buffer) TryBytes() ([]byte, error) { // TryInteger implements Item interface. func (i *Buffer) TryInteger() (*big.Int, error) { - return nil, errors.New("can't convert Buffer to Integer") + return nil, mkInvConversion(i, IntegerT) } // Equals implements Item interface. @@ -1058,11 +1074,11 @@ func (i *Buffer) Convert(typ Type) (Item, error) { return NewByteArray(val), nil case IntegerT: if len(i.value) > MaxBigIntegerSizeBits/8 { - return nil, errInvalidConversion + return nil, errTooBigInteger } return NewBigInteger(bigint.FromBytes(i.value)), nil default: - return nil, errInvalidConversion + return nil, mkInvConversion(i, typ) } } diff --git a/pkg/vm/stackitem/json.go b/pkg/vm/stackitem/json.go index 3d6abe9ed..a431d73b9 100644 --- a/pkg/vm/stackitem/json.go +++ b/pkg/vm/stackitem/json.go @@ -23,8 +23,16 @@ type decoder struct { // MaxAllowedInteger is the maximum integer allowed to be encoded. const MaxAllowedInteger = 2<<53 - 1 -// maxJSONDepth is a maximum allowed depth-level of decoded JSON. -const maxJSONDepth = 10 +// MaxJSONDepth is the maximum allowed nesting level of encoded/decoded JSON. +const MaxJSONDepth = 10 + +// ErrInvalidValue is returned when item value doesn't fit some constraints +// during serialization or deserialization. +var ErrInvalidValue = errors.New("invalid value") + +// ErrTooDeep is returned when JSON encoder/decoder goes beyond MaxJSONDepth in +// its processing. +var ErrTooDeep = errors.New("too deep") // ToJSON encodes Item to JSON. // It behaves as following: @@ -48,7 +56,7 @@ func toJSON(buf *io.BufBinWriter, item Item) { if w.Err != nil { return } else if buf.Len() > MaxSize { - w.Err = errors.New("item is too big") + w.Err = errTooBigSize } switch it := item.(type) { case *Array, *Struct: @@ -76,7 +84,7 @@ func toJSON(buf *io.BufBinWriter, item Item) { w.WriteB('}') case *BigInteger: if it.value.CmpAbs(big.NewInt(MaxAllowedInteger)) == 1 { - w.Err = errors.New("too big integer") + w.Err = fmt.Errorf("%w (MaxAllowedInteger)", ErrInvalidValue) return } w.WriteBytes([]byte(it.value.String())) @@ -91,11 +99,11 @@ func toJSON(buf *io.BufBinWriter, item Item) { case Null: w.WriteBytes([]byte("null")) default: - w.Err = fmt.Errorf("invalid item: %s", it.String()) + w.Err = fmt.Errorf("%w: %s", ErrUnserializable, it.String()) return } if w.Err == nil && buf.Len() > MaxSize { - w.Err = errors.New("item is too big") + w.Err = errTooBigSize } } @@ -131,7 +139,7 @@ func FromJSON(data []byte) (Item, error) { if item, err := d.decode(); err != nil { return nil, err } else if _, err := d.Token(); err != gio.EOF { - return nil, errors.New("unexpected items") + return nil, fmt.Errorf("%w: unexpected items", ErrInvalidValue) } else { return item, nil } @@ -146,8 +154,8 @@ func (d *decoder) decode() (Item, error) { case json.Delim: switch t { case json.Delim('{'), json.Delim('['): - if d.depth == maxJSONDepth { - return nil, errors.New("JSON depth limit exceeded") + if d.depth == MaxJSONDepth { + return nil, ErrTooDeep } d.depth++ var item Item @@ -167,7 +175,7 @@ func (d *decoder) decode() (Item, error) { return NewByteArray([]byte(t)), nil case float64: if math.Floor(t) != t { - return nil, fmt.Errorf("real value is not allowed: %v", t) + return nil, fmt.Errorf("%w (real value for int)", ErrInvalidValue) } return NewBigInteger(big.NewInt(int64(t))), nil case bool: @@ -221,8 +229,8 @@ func ToJSONWithTypes(item Item) ([]byte, error) { } func toJSONWithTypes(item Item, seen map[Item]bool) (interface{}, error) { - if len(seen) > maxJSONDepth { - return "", errors.New("too deep structure") + if len(seen) > MaxJSONDepth { + return "", ErrTooDeep } typ := item.Type() result := map[string]interface{}{ @@ -232,7 +240,7 @@ func toJSONWithTypes(item Item, seen map[Item]bool) (interface{}, error) { switch it := item.(type) { case *Array, *Struct: if seen[item] { - return "", errors.New("recursive structures can't be serialized to json") + return "", ErrRecursive } seen[item] = true arr := []interface{}{} @@ -253,7 +261,7 @@ func toJSONWithTypes(item Item, seen map[Item]bool) (interface{}, error) { value = it.value.String() case *Map: if seen[item] { - return "", errors.New("recursive structures can't be serialized to json") + return "", ErrRecursive } seen[item] = true arr := []interface{}{} @@ -292,6 +300,10 @@ type ( } ) +func mkErrValue(err error) error { + return fmt.Errorf("%w: %v", ErrInvalidValue, err) +} + // FromJSONWithTypes deserializes an item from typed-json representation. func FromJSONWithTypes(data []byte) (Item, error) { raw := new(rawItem) @@ -300,7 +312,7 @@ func FromJSONWithTypes(data []byte) (Item, error) { } typ, err := FromString(raw.Type) if err != nil { - return nil, errors.New("invalid type") + return nil, fmt.Errorf("%w: %v", ErrInvalidType, raw.Type) } switch typ { case AnyT: @@ -308,33 +320,33 @@ func FromJSONWithTypes(data []byte) (Item, error) { case PointerT: var pos int if err := json.Unmarshal(raw.Value, &pos); err != nil { - return nil, err + return nil, mkErrValue(err) } return NewPointer(pos, nil), nil case BooleanT: var b bool if err := json.Unmarshal(raw.Value, &b); err != nil { - return nil, err + return nil, mkErrValue(err) } return NewBool(b), nil case IntegerT: var s string if err := json.Unmarshal(raw.Value, &s); err != nil { - return nil, err + return nil, mkErrValue(err) } val, ok := new(big.Int).SetString(s, 10) if !ok { - return nil, errors.New("invalid integer") + return nil, mkErrValue(errors.New("not an integer")) } return NewBigInteger(val), nil case ByteArrayT, BufferT: var s string if err := json.Unmarshal(raw.Value, &s); err != nil { - return nil, err + return nil, mkErrValue(err) } val, err := base64.StdEncoding.DecodeString(s) if err != nil { - return nil, err + return nil, mkErrValue(err) } if typ == ByteArrayT { return NewByteArray(val), nil @@ -343,7 +355,7 @@ func FromJSONWithTypes(data []byte) (Item, error) { case ArrayT, StructT: var arr []json.RawMessage if err := json.Unmarshal(raw.Value, &arr); err != nil { - return nil, err + return nil, mkErrValue(err) } items := make([]Item, len(arr)) for i := range arr { @@ -360,7 +372,7 @@ func FromJSONWithTypes(data []byte) (Item, error) { case MapT: var arr []rawMapElement if err := json.Unmarshal(raw.Value, &arr); err != nil { - return nil, err + return nil, mkErrValue(err) } m := NewMap() for i := range arr { @@ -380,6 +392,6 @@ func FromJSONWithTypes(data []byte) (Item, error) { case InteropT: return NewInterop(nil), nil default: - return nil, errors.New("unexpected type") + return nil, fmt.Errorf("%w: %v", ErrInvalidType, typ) } } diff --git a/pkg/vm/stackitem/serialization.go b/pkg/vm/stackitem/serialization.go index f668522a6..cbc54b4b0 100644 --- a/pkg/vm/stackitem/serialization.go +++ b/pkg/vm/stackitem/serialization.go @@ -9,6 +9,14 @@ import ( "github.com/nspcc-dev/neo-go/pkg/io" ) +// ErrRecursive is returned on attempts to serialize some recursive stack item +// (like array including an item with reference to the same array). +var ErrRecursive = errors.New("recursive item") + +// ErrUnserializable is returned on attempt to serialize some item that can't +// be serialized (like Interop item or Pointer). +var ErrUnserializable = errors.New("unserializable") + // serContext is an internal serialization context. type serContext struct { *io.BinWriter @@ -71,7 +79,7 @@ func (w *serContext) serialize(item Item) { return } if w.seen[item] { - w.Err = errors.New("recursive structures can't be serialized") + w.Err = ErrRecursive return } @@ -92,7 +100,7 @@ func (w *serContext) serialize(item Item) { if w.allowInvalid { w.WriteBytes([]byte{byte(InteropT)}) } else { - w.Err = errors.New("interop item can't be serialized") + w.Err = fmt.Errorf("%w: Interop", ErrUnserializable) } case *Array, *Struct: w.seen[item] = true @@ -126,12 +134,12 @@ func (w *serContext) serialize(item Item) { if w.allowInvalid { w.WriteBytes([]byte{byte(InvalidT)}) } else { - w.Err = errors.New("invalid stack item") + w.Err = fmt.Errorf("%w: nil", ErrUnserializable) } } if w.Err == nil && w.buf != nil && w.buf.Len() > MaxSize { - w.Err = errors.New("too big item") + w.Err = errTooBigSize } } @@ -213,7 +221,7 @@ func decodeBinary(r *io.BinReader, allowInvalid bool) Item { if t == InvalidT && allowInvalid { return nil } - r.Err = fmt.Errorf("unknown type: %v", t) + r.Err = fmt.Errorf("%w: %v", ErrInvalidType, t) return nil } } diff --git a/pkg/vm/stackitem/serialization_test.go b/pkg/vm/stackitem/serialization_test.go index f3d58aa96..02b07cb24 100644 --- a/pkg/vm/stackitem/serialization_test.go +++ b/pkg/vm/stackitem/serialization_test.go @@ -1,6 +1,7 @@ package stackitem import ( + "errors" "testing" "github.com/stretchr/testify/require" @@ -10,12 +11,13 @@ func TestSerializationMaxErr(t *testing.T) { base := make([]byte, MaxSize/2+1) item := Make(base) - arr := []Item{item, item.Dup()} + // Pointer is unserializable, but we specifically want to catch ErrTooBig. + arr := []Item{item, item.Dup(), NewPointer(0, []byte{})} aitem := Make(arr) _, err := Serialize(item) require.NoError(t, err) _, err = Serialize(aitem) - require.Error(t, err) + require.True(t, errors.Is(err, ErrTooBig), err) } diff --git a/pkg/vm/stackitem/type.go b/pkg/vm/stackitem/type.go index b28df9ebe..30dee93fd 100644 --- a/pkg/vm/stackitem/type.go +++ b/pkg/vm/stackitem/type.go @@ -2,6 +2,9 @@ package stackitem import "errors" +// ErrInvalidType is returned on attempts to deserialize some unknown item type. +var ErrInvalidType = errors.New("invalid type") + // Type represents type of the stack item. type Type byte @@ -82,6 +85,6 @@ func FromString(s string) (Type, error) { case "Interop": return InteropT, nil default: - return 0xFF, errors.New("invalid type") + return 0xFF, ErrInvalidType } } From e62a766058516974f41f003eae9655e873506d43 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Wed, 7 Jul 2021 00:38:19 +0300 Subject: [PATCH 8/9] state: move nil check down to stackitem JSON processing We want to return real errors, not some generic thing for any kind of thing happening. --- pkg/core/state/notification_event.go | 4 ---- pkg/core/state/notification_event_test.go | 8 +++++++- pkg/vm/stackitem/json.go | 9 +++++---- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/pkg/core/state/notification_event.go b/pkg/core/state/notification_event.go index 193fc8667..06e77184a 100644 --- a/pkg/core/state/notification_event.go +++ b/pkg/core/state/notification_event.go @@ -194,10 +194,6 @@ func (e Execution) MarshalJSON() ([]byte, error) { var errRecursive = []byte(`"error: recursive reference"`) arr := make([]json.RawMessage, len(e.Stack)) for i := range arr { - if e.Stack[i] == nil { - arr[i] = errRecursive - continue - } data, err := stackitem.ToJSONWithTypes(e.Stack[i]) if err != nil { data = errRecursive diff --git a/pkg/core/state/notification_event_test.go b/pkg/core/state/notification_event_test.go index 55e36f3aa..2f6667f61 100644 --- a/pkg/core/state/notification_event_test.go +++ b/pkg/core/state/notification_event_test.go @@ -184,7 +184,13 @@ func TestMarshalUnmarshalJSONAppExecResult(t *testing.T) { bs1, err := json.Marshal(actual) require.NoError(t, err) - require.Equal(t, bs, bs1) + require.NotEqual(t, bs, bs1) // recursive ref error vs. unserializable nil + + actual2 := new(AppExecResult) + require.NoError(t, json.Unmarshal(bs, actual2)) + bs2, err := json.Marshal(actual2) + require.NoError(t, err) + require.Equal(t, bs1, bs2) // unserializable nil in both cases }) t.Run("UnmarshalJSON error", func(t *testing.T) { diff --git a/pkg/vm/stackitem/json.go b/pkg/vm/stackitem/json.go index a431d73b9..b930344ba 100644 --- a/pkg/vm/stackitem/json.go +++ b/pkg/vm/stackitem/json.go @@ -232,10 +232,6 @@ func toJSONWithTypes(item Item, seen map[Item]bool) (interface{}, error) { if len(seen) > MaxJSONDepth { return "", ErrTooDeep } - typ := item.Type() - result := map[string]interface{}{ - "type": typ.String(), - } var value interface{} switch it := item.(type) { case *Array, *Struct: @@ -281,6 +277,11 @@ func toJSONWithTypes(item Item, seen map[Item]bool) (interface{}, error) { delete(seen, item) case *Pointer: value = it.pos + case nil: + return "", fmt.Errorf("%w: nil", ErrUnserializable) + } + result := map[string]interface{}{ + "type": item.Type().String(), } if value != nil { result["value"] = value From 0cd9cd0c80dda2641f57a1b5e932cb72b4e2027b Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Wed, 7 Jul 2021 00:42:36 +0300 Subject: [PATCH 9/9] state/result: save/return real JSONization errors Don't hide/obfuscate real problems. --- pkg/core/state/notification_event.go | 5 ++--- pkg/rpc/response/result/invoke.go | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/core/state/notification_event.go b/pkg/core/state/notification_event.go index 06e77184a..87ea9dbb1 100644 --- a/pkg/core/state/notification_event.go +++ b/pkg/core/state/notification_event.go @@ -101,7 +101,7 @@ type notificationEventAux struct { func (ne NotificationEvent) MarshalJSON() ([]byte, error) { item, err := stackitem.ToJSONWithTypes(ne.Item) if err != nil { - item = []byte(`"error: recursive reference"`) + item = []byte(fmt.Sprintf(`"error: %v"`, err)) } return json.Marshal(¬ificationEventAux{ ScriptHash: ne.ScriptHash, @@ -191,12 +191,11 @@ type executionAux struct { // MarshalJSON implements implements json.Marshaler interface. func (e Execution) MarshalJSON() ([]byte, error) { - var errRecursive = []byte(`"error: recursive reference"`) arr := make([]json.RawMessage, len(e.Stack)) for i := range arr { data, err := stackitem.ToJSONWithTypes(e.Stack[i]) if err != nil { - data = errRecursive + data = []byte(fmt.Sprintf(`"error: %v"`, err)) } arr[i] = data } diff --git a/pkg/rpc/response/result/invoke.go b/pkg/rpc/response/result/invoke.go index cbd2740f0..83685a7ac 100644 --- a/pkg/rpc/response/result/invoke.go +++ b/pkg/rpc/response/result/invoke.go @@ -70,7 +70,7 @@ func (r Invoke) MarshalJSON() ([]byte, error) { for j := range iteratorValues { value[j], err = stackitem.ToJSONWithTypes(iteratorValues[j]) if err != nil { - st = []byte(`"error: recursive reference"`) + st = []byte(fmt.Sprintf(`"error: %v"`, err)) break } } @@ -85,7 +85,7 @@ func (r Invoke) MarshalJSON() ([]byte, error) { } else { data, err = stackitem.ToJSONWithTypes(r.Stack[i]) if err != nil { - st = []byte(`"error: recursive reference"`) + st = []byte(fmt.Sprintf(`"error: %v"`, err)) break } }