From 0b76f875c7e180b36af40f3ad3e9fe1e04e4b0e7 Mon Sep 17 00:00:00 2001 From: Evgenii Stratonikov Date: Mon, 5 Oct 2020 16:12:08 +0300 Subject: [PATCH] core: allow Null in `System.Contract.Update` Null means absense of script or manifest, empty byte-slice is an error. Related #1459. --- pkg/core/interop_neo.go | 33 ++++++++++++++++++---------- pkg/core/interop_system_test.go | 39 ++++++++++++++++++++++----------- pkg/vm/stack.go | 13 +++++++++++ 3 files changed, 61 insertions(+), 24 deletions(-) diff --git a/pkg/core/interop_neo.go b/pkg/core/interop_neo.go index 244ccccf3..686bb8d7b 100644 --- a/pkg/core/interop_neo.go +++ b/pkg/core/interop_neo.go @@ -112,6 +112,17 @@ func contractCreate(ic *interop.Context) error { return nil } +func checkNonEmpty(b []byte, max int) error { + if b != nil { + if l := len(b); l == 0 { + return errors.New("empty") + } else if l > max { + return fmt.Errorf("len is %d (max %d)", l, max) + } + } + return nil +} + // contractUpdate migrates a contract. This method assumes that Manifest and Script // of the contract can be updated independently. func contractUpdate(ic *interop.Context) error { @@ -119,22 +130,22 @@ func contractUpdate(ic *interop.Context) error { if contract == nil { return errors.New("contract doesn't exist") } - script := ic.VM.Estack().Pop().Bytes() - if len(script) > MaxContractScriptSize { - return errors.New("the script is too big") + script := ic.VM.Estack().Pop().BytesOrNil() + manifestBytes := ic.VM.Estack().Pop().BytesOrNil() + if script == nil && manifestBytes == nil { + return errors.New("both script and manifest are nil") } - manifestBytes := ic.VM.Estack().Pop().Bytes() - if len(manifestBytes) > manifest.MaxManifestSize { - return errors.New("manifest is too big") + if err := checkNonEmpty(script, MaxContractScriptSize); err != nil { + return fmt.Errorf("invalid script size: %w", err) + } + if err := checkNonEmpty(manifestBytes, manifest.MaxManifestSize); err != nil { + return fmt.Errorf("invalid manifest size: %w", err) } if !ic.VM.AddGas(int64(StoragePrice * (len(script) + len(manifestBytes)))) { return errGasLimitExceeded } // if script was provided, update the old contract script and Manifest.ABI hash - if l := len(script); l > 0 { - if l > MaxContractScriptSize { - return errors.New("invalid script len") - } + if script != nil { newHash := hash.Hash160(script) if newHash.Equals(contract.ScriptHash()) { return errors.New("the script is the same") @@ -158,7 +169,7 @@ func contractUpdate(ic *interop.Context) error { } // if manifest was provided, update the old contract manifest and check associated // storage items if needed - if len(manifestBytes) > 0 { + if manifestBytes != nil { var newManifest manifest.Manifest err := newManifest.UnmarshalJSON(manifestBytes) if err != nil { diff --git a/pkg/core/interop_system_test.go b/pkg/core/interop_system_test.go index 365e650b4..9f67cfc93 100644 --- a/pkg/core/interop_system_test.go +++ b/pkg/core/interop_system_test.go @@ -627,7 +627,7 @@ func TestContractUpdate(t *testing.T) { defer bc.Close() v.GasLimit = -1 - putArgsOnStack := func(script, manifest []byte) { + putArgsOnStack := func(script, manifest interface{}) { v.Estack().PushVal(manifest) v.Estack().PushVal(script) } @@ -635,8 +635,8 @@ func TestContractUpdate(t *testing.T) { t.Run("no args", func(t *testing.T) { require.NoError(t, ic.DAO.PutContractState(cs)) v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All) - putArgsOnStack(nil, nil) - require.NoError(t, contractUpdate(ic)) + putArgsOnStack(stackitem.Null{}, stackitem.Null{}) + require.Error(t, contractUpdate(ic)) }) t.Run("no contract", func(t *testing.T) { @@ -648,14 +648,14 @@ func TestContractUpdate(t *testing.T) { t.Run("too large script", func(t *testing.T) { require.NoError(t, ic.DAO.PutContractState(cs)) v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All) - putArgsOnStack(make([]byte, MaxContractScriptSize+1), nil) + putArgsOnStack(make([]byte, MaxContractScriptSize+1), stackitem.Null{}) require.Error(t, contractUpdate(ic)) }) t.Run("too large manifest", func(t *testing.T) { require.NoError(t, ic.DAO.PutContractState(cs)) v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All) - putArgsOnStack(nil, make([]byte, manifest.MaxManifestSize+1)) + putArgsOnStack(stackitem.Null{}, make([]byte, manifest.MaxManifestSize+1)) require.Error(t, contractUpdate(ic)) }) @@ -671,7 +671,7 @@ func TestContractUpdate(t *testing.T) { require.NoError(t, ic.DAO.PutContractState(cs)) v.GasLimit = -1 v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All) - putArgsOnStack(cs.Script, nil) + putArgsOnStack(cs.Script, stackitem.Null{}) require.Error(t, contractUpdate(ic)) }) @@ -689,16 +689,23 @@ func TestContractUpdate(t *testing.T) { }, })) v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All) - putArgsOnStack(duplicateScript, nil) + putArgsOnStack(duplicateScript, stackitem.Null{}) require.Error(t, contractUpdate(ic)) }) t.Run("update script, positive", func(t *testing.T) { require.NoError(t, ic.DAO.PutContractState(cs)) + t.Run("empty manifest", func(t *testing.T) { + v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All) + newScript := []byte{9, 8, 7, 6, 5} + putArgsOnStack(newScript, []byte{}) + require.Error(t, contractUpdate(ic)) + }) + v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All) newScript := []byte{9, 8, 7, 6, 5} - putArgsOnStack(newScript, nil) + putArgsOnStack(newScript, stackitem.Null{}) require.NoError(t, contractUpdate(ic)) @@ -722,7 +729,7 @@ func TestContractUpdate(t *testing.T) { t.Run("update manifest, bad manifest", func(t *testing.T) { require.NoError(t, ic.DAO.PutContractState(cs)) v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All) - putArgsOnStack(nil, []byte{1, 2, 3}) + putArgsOnStack(stackitem.Null{}, []byte{1, 2, 3}) require.Error(t, contractUpdate(ic)) }) @@ -737,7 +744,7 @@ func TestContractUpdate(t *testing.T) { } manifestBytes, err := manifest.MarshalJSON() require.NoError(t, err) - putArgsOnStack(nil, manifestBytes) + putArgsOnStack(stackitem.Null{}, manifestBytes) require.Error(t, contractUpdate(ic)) }) @@ -757,7 +764,7 @@ func TestContractUpdate(t *testing.T) { } manifestBytes, err := manifest.MarshalJSON() require.NoError(t, err) - putArgsOnStack(nil, manifestBytes) + putArgsOnStack(stackitem.Null{}, manifestBytes) require.Error(t, contractUpdate(ic)) }) @@ -765,7 +772,6 @@ func TestContractUpdate(t *testing.T) { t.Run("update manifest, positive", func(t *testing.T) { cs.Manifest.Features = smartcontract.NoProperties require.NoError(t, ic.DAO.PutContractState(cs)) - v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All) manifest := &manifest.Manifest{ ABI: manifest.ABI{ Hash: cs.ScriptHash(), @@ -774,8 +780,15 @@ func TestContractUpdate(t *testing.T) { } manifestBytes, err := manifest.MarshalJSON() require.NoError(t, err) - putArgsOnStack(nil, manifestBytes) + t.Run("empty script", func(t *testing.T) { + v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All) + putArgsOnStack([]byte{}, manifestBytes) + require.Error(t, contractUpdate(ic)) + }) + + v.LoadScriptWithHash([]byte{byte(opcode.RET)}, cs.ScriptHash(), smartcontract.All) + putArgsOnStack(stackitem.Null{}, manifestBytes) require.NoError(t, contractUpdate(ic)) // updated contract should have new scripthash diff --git a/pkg/vm/stack.go b/pkg/vm/stack.go index dd2a9656a..62f45022a 100644 --- a/pkg/vm/stack.go +++ b/pkg/vm/stack.go @@ -100,6 +100,19 @@ func (e *Element) Bytes() []byte { return bs } +// BytesOrNil attempts to get the underlying value of the element as a byte array or nil. +// Will panic if the assertion failed which will be caught by the VM. +func (e *Element) BytesOrNil() []byte { + if _, ok := e.value.(stackitem.Null); ok { + return nil + } + bs, err := e.value.TryBytes() + if err != nil { + panic(err) + } + return bs +} + // String attempts to get string from the element value. // It is assumed to be use in interops and panics if string is not a valid UTF-8 byte sequence. func (e *Element) String() string {