From dff88b63b2b46300c2efbf5dc44f271fba1a4ef1 Mon Sep 17 00:00:00 2001 From: Aayush Rajasekaran Date: Sun, 8 Nov 2020 19:58:10 -0500 Subject: [PATCH 1/4] fix: guard against self beneficiary when deleting actor --- chain/vm/runtime.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/chain/vm/runtime.go b/chain/vm/runtime.go index 5dc17572644..c537441e80e 100644 --- a/chain/vm/runtime.go +++ b/chain/vm/runtime.go @@ -266,7 +266,7 @@ func (rt *Runtime) CreateActor(codeID cid.Cid, address address.Address) { // DeleteActor deletes the executing actor from the state tree, transferring // any balance to beneficiary. -// Aborts if the beneficiary does not exist. +// Aborts if the beneficiary does not exist or is the calling actor. // May only be called by the actor itself. func (rt *Runtime) DeleteActor(beneficiary address.Address) { rt.chargeGas(rt.Pricelist().OnDeleteActor()) @@ -278,6 +278,9 @@ func (rt *Runtime) DeleteActor(beneficiary address.Address) { panic(aerrors.Fatalf("failed to get actor: %s", err)) } if !act.Balance.IsZero() { + if beneficiary == rt.Receiver() && rt.NetworkVersion() >= network.Version7 { + rt.Abortf(exitcode.SysErrorIllegalArgument, "benefactor cannot be beneficiary") + } // Transfer the executing actor's balance to the beneficiary if err := rt.vm.transfer(rt.Receiver(), beneficiary, act.Balance); err != nil { panic(aerrors.Fatalf("failed to transfer balance to beneficiary actor: %s", err)) From ce9146916a25c14e0b5cf0e0abc7a88704ede481 Mon Sep 17 00:00:00 2001 From: Aayush Rajasekaran Date: Sun, 8 Nov 2020 20:48:05 -0500 Subject: [PATCH 2/4] fix: actor method params deserialization error exit code --- chain/vm/invoker.go | 10 ++++++++-- chain/vm/invoker_test.go | 28 ++++++++++++++++++++++++---- chain/vm/runtime.go | 9 ++++++--- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/chain/vm/invoker.go b/chain/vm/invoker.go index 661e31178ee..e22d6965312 100644 --- a/chain/vm/invoker.go +++ b/chain/vm/invoker.go @@ -6,6 +6,8 @@ import ( "fmt" "reflect" + "github.com/filecoin-project/go-state-types/network" + "github.com/filecoin-project/lotus/chain/actors/builtin" "github.com/ipfs/go-cid" @@ -173,9 +175,14 @@ func (*ActorRegistry) transform(instance invokee) (nativeCode, error) { paramT := meth.Type().In(1).Elem() param := reflect.New(paramT) + rt := in[0].Interface().(*Runtime) inBytes := in[1].Interface().([]byte) if err := DecodeParams(inBytes, param.Interface()); err != nil { - aerr := aerrors.Absorb(err, 1, "failed to decode parameters") + ec := exitcode.ErrSerialization + if rt.NetworkVersion() < network.Version7 { + ec = 1 + } + aerr := aerrors.Absorb(err, ec, "failed to decode parameters") return []reflect.Value{ reflect.ValueOf([]byte{}), // Below is a hack, fixed in Go 1.13 @@ -183,7 +190,6 @@ func (*ActorRegistry) transform(instance invokee) (nativeCode, error) { reflect.ValueOf(&aerr).Elem(), } } - rt := in[0].Interface().(*Runtime) rval, aerror := rt.shimCall(func() interface{} { ret := meth.Call([]reflect.Value{ reflect.ValueOf(rt), diff --git a/chain/vm/invoker_test.go b/chain/vm/invoker_test.go index bce385b02ba..6822e2371f5 100644 --- a/chain/vm/invoker_test.go +++ b/chain/vm/invoker_test.go @@ -1,10 +1,13 @@ package vm import ( + "context" "fmt" "io" "testing" + "github.com/filecoin-project/go-state-types/network" + cbor "github.com/ipfs/go-ipld-cbor" "github.com/stretchr/testify/assert" cbg "github.com/whyrusleeping/cbor-gen" @@ -105,10 +108,27 @@ func TestInvokerBasic(t *testing.T) { } } - _, aerr := code[1](&Runtime{}, []byte{99}) - if aerrors.IsFatal(aerr) { - t.Fatal("err should not be fatal") + { + _, aerr := code[1](&Runtime{ + vm: &VM{ntwkVersion: func(ctx context.Context, epoch abi.ChainEpoch) network.Version { + return network.Version0 + }}, + }, []byte{99}) + if aerrors.IsFatal(aerr) { + t.Fatal("err should not be fatal") + } + assert.Equal(t, exitcode.ExitCode(1), aerrors.RetCode(aerr), "return code should be 1") } - assert.Equal(t, exitcode.ExitCode(1), aerrors.RetCode(aerr), "return code should be 1") + { + _, aerr := code[1](&Runtime{ + vm: &VM{ntwkVersion: func(ctx context.Context, epoch abi.ChainEpoch) network.Version { + return network.Version7 + }}, + }, []byte{99}) + if aerrors.IsFatal(aerr) { + t.Fatal("err should not be fatal") + } + assert.Equal(t, exitcode.ErrSerialization, aerrors.RetCode(aerr), "return code should be %s", 1) + } } diff --git a/chain/vm/runtime.go b/chain/vm/runtime.go index c537441e80e..9107e6d7e1c 100644 --- a/chain/vm/runtime.go +++ b/chain/vm/runtime.go @@ -244,20 +244,23 @@ func (rt *Runtime) NewActorAddress() address.Address { return addr } -func (rt *Runtime) CreateActor(codeID cid.Cid, address address.Address) { +func (rt *Runtime) CreateActor(codeID cid.Cid, addr address.Address) { + if addr == address.Undef && rt.NetworkVersion() >= network.Version7 { + rt.Abortf(exitcode.SysErrorIllegalArgument, "CreateActor with Undef address") + } act, aerr := rt.vm.areg.Create(codeID, rt) if aerr != nil { rt.Abortf(aerr.RetCode(), aerr.Error()) } - _, err := rt.state.GetActor(address) + _, err := rt.state.GetActor(addr) if err == nil { rt.Abortf(exitcode.SysErrorIllegalArgument, "Actor address already exists") } rt.chargeGas(rt.Pricelist().OnCreateActor()) - err = rt.state.SetActor(address, act) + err = rt.state.SetActor(addr, act) if err != nil { panic(aerrors.Fatalf("creating actor entry: %v", err)) } From 5c791cd93bd5336d1e4e89714d85a23e439c0d1e Mon Sep 17 00:00:00 2001 From: Aayush Rajasekaran Date: Sun, 8 Nov 2020 21:13:58 -0500 Subject: [PATCH 3/4] Check parent runtime allowInternal when making a new Runtime --- chain/vm/vm.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/chain/vm/vm.go b/chain/vm/vm.go index fa28ce92847..5f82b966994 100644 --- a/chain/vm/vm.go +++ b/chain/vm/vm.go @@ -138,6 +138,10 @@ func (vm *VM) makeRuntime(ctx context.Context, msg *types.Message, parent *Runti } if parent != nil { + // TODO: The version check here should be unnecessary, but we can wait to take it out + if !parent.allowInternal && rt.NetworkVersion() >= network.Version7 { + rt.Abortf(exitcode.SysErrForbidden, "internal calls currently disabled") + } rt.gasUsed = parent.gasUsed rt.origin = parent.origin rt.originNonce = parent.originNonce From 2219bff1c9faeb7c2a2da44dcdb32e69083f8376 Mon Sep 17 00:00:00 2001 From: Aayush Rajasekaran Date: Tue, 17 Nov 2020 00:28:27 -0500 Subject: [PATCH 4/4] SerializeParams should return ErrSerialization --- chain/actors/params.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/chain/actors/params.go b/chain/actors/params.go index e14dcafc9ff..6dc0b1084db 100644 --- a/chain/actors/params.go +++ b/chain/actors/params.go @@ -3,6 +3,8 @@ package actors import ( "bytes" + "github.com/filecoin-project/go-state-types/exitcode" + "github.com/filecoin-project/lotus/chain/actors/aerrors" cbg "github.com/whyrusleeping/cbor-gen" ) @@ -11,7 +13,7 @@ func SerializeParams(i cbg.CBORMarshaler) ([]byte, aerrors.ActorError) { buf := new(bytes.Buffer) if err := i.MarshalCBOR(buf); err != nil { // TODO: shouldnt this be a fatal error? - return nil, aerrors.Absorb(err, 1, "failed to encode parameter") + return nil, aerrors.Absorb(err, exitcode.ErrSerialization, "failed to encode parameter") } return buf.Bytes(), nil }