From 8ba244071e51d658b9f0e7d0eeef84f2aed7f756 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Fri, 10 Jul 2020 10:58:11 +0100 Subject: [PATCH] fix types.ChainAnteDecorators() panic (#5742) (#6669) ChainAnteDecorators() panics when no arguments are supplied. This change its behaviour and the function now returns a nil AnteHandler in case no AnteDecorator instances are supplied. Closes: #5741 --- CHANGELOG.md | 1 + Makefile | 1 + tests/mocks/account_retriever.go | 13 +++++++-- tests/mocks/types_handler.go | 49 ++++++++++++++++++++++++++++++++ types/handler.go | 12 ++++---- types/handler_test.go | 28 ++++++++++++++++++ 6 files changed, 96 insertions(+), 8 deletions(-) create mode 100644 tests/mocks/types_handler.go create mode 100644 types/handler_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 8df0d6a1b0a5..e43e38377f39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,7 @@ options `pruning-keep-recent`, `pruning-keep-every`, and `pruning-interval`. The dictate how many recent versions are kept on disk and the offset of what versions are kept after that respectively, and the latter defines the height interval in which versions are deleted in a batch. **Note, there are some client-facing API breaking changes with regard to IAVL, stores, and pruning settings.** +* (types) [\#5741](https://github.com/cosmos/cosmos-sdk/issues/5741) Prevent `ChainAnteDecorators()` from panicking when empty `AnteDecorator` slice is supplied. ## [v0.38.4] - 2020-05-21 diff --git a/Makefile b/Makefile index 8d947640f75a..9d857bc8ca7c 100644 --- a/Makefile +++ b/Makefile @@ -35,6 +35,7 @@ update-swagger-docs: statik mocks: $(MOCKS_DIR) mockgen -source=x/auth/types/account_retriever.go -package mocks -destination tests/mocks/account_retriever.go + mockgen -source=types/handler.go -package mocks -destination tests/mocks/types_handler.go .PHONY: mocks $(MOCKS_DIR): diff --git a/tests/mocks/account_retriever.go b/tests/mocks/account_retriever.go index 9f8ee69282a2..a3086c77bdca 100644 --- a/tests/mocks/account_retriever.go +++ b/tests/mocks/account_retriever.go @@ -1,30 +1,38 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: x/auth/types/account_retriever.go + +// Package mocks is a generated GoMock package. package mocks import ( - reflect "reflect" - gomock "github.com/golang/mock/gomock" + reflect "reflect" ) +// MockNodeQuerier is a mock of NodeQuerier interface type MockNodeQuerier struct { ctrl *gomock.Controller recorder *MockNodeQuerierMockRecorder } +// MockNodeQuerierMockRecorder is the mock recorder for MockNodeQuerier type MockNodeQuerierMockRecorder struct { mock *MockNodeQuerier } +// NewMockNodeQuerier creates a new mock instance func NewMockNodeQuerier(ctrl *gomock.Controller) *MockNodeQuerier { mock := &MockNodeQuerier{ctrl: ctrl} mock.recorder = &MockNodeQuerierMockRecorder{mock} return mock } +// EXPECT returns an object that allows the caller to indicate expected use func (m *MockNodeQuerier) EXPECT() *MockNodeQuerierMockRecorder { return m.recorder } +// QueryWithData mocks base method func (m *MockNodeQuerier) QueryWithData(path string, data []byte) ([]byte, int64, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "QueryWithData", path, data) @@ -34,6 +42,7 @@ func (m *MockNodeQuerier) QueryWithData(path string, data []byte) ([]byte, int64 return ret0, ret1, ret2 } +// QueryWithData indicates an expected call of QueryWithData func (mr *MockNodeQuerierMockRecorder) QueryWithData(path, data interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "QueryWithData", reflect.TypeOf((*MockNodeQuerier)(nil).QueryWithData), path, data) diff --git a/tests/mocks/types_handler.go b/tests/mocks/types_handler.go new file mode 100644 index 000000000000..da80614ae884 --- /dev/null +++ b/tests/mocks/types_handler.go @@ -0,0 +1,49 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: types/handler.go + +// Package mocks is a generated GoMock package. +package mocks + +import ( + types "github.com/cosmos/cosmos-sdk/types" + gomock "github.com/golang/mock/gomock" + reflect "reflect" +) + +// MockAnteDecorator is a mock of AnteDecorator interface +type MockAnteDecorator struct { + ctrl *gomock.Controller + recorder *MockAnteDecoratorMockRecorder +} + +// MockAnteDecoratorMockRecorder is the mock recorder for MockAnteDecorator +type MockAnteDecoratorMockRecorder struct { + mock *MockAnteDecorator +} + +// NewMockAnteDecorator creates a new mock instance +func NewMockAnteDecorator(ctrl *gomock.Controller) *MockAnteDecorator { + mock := &MockAnteDecorator{ctrl: ctrl} + mock.recorder = &MockAnteDecoratorMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockAnteDecorator) EXPECT() *MockAnteDecoratorMockRecorder { + return m.recorder +} + +// AnteHandle mocks base method +func (m *MockAnteDecorator) AnteHandle(ctx types.Context, tx types.Tx, simulate bool, next types.AnteHandler) (types.Context, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AnteHandle", ctx, tx, simulate, next) + ret0, _ := ret[0].(types.Context) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// AnteHandle indicates an expected call of AnteHandle +func (mr *MockAnteDecoratorMockRecorder) AnteHandle(ctx, tx, simulate, next interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AnteHandle", reflect.TypeOf((*MockAnteDecorator)(nil).AnteHandle), ctx, tx, simulate, next) +} diff --git a/types/handler.go b/types/handler.go index 6815230a861f..03d1f02d5806 100644 --- a/types/handler.go +++ b/types/handler.go @@ -25,15 +25,15 @@ type AnteDecorator interface { // MUST set GasMeter with the FIRST AnteDecorator. Failing to do so will cause // transactions to be processed with an infinite gasmeter and open a DOS attack vector. // Use `ante.SetUpContextDecorator` or a custom Decorator with similar functionality. +// Returns nil when no AnteDecorator are supplied. func ChainAnteDecorators(chain ...AnteDecorator) AnteHandler { - if (chain[len(chain)-1] != Terminator{}) { - chain = append(chain, Terminator{}) + if len(chain) == 0 { + return nil } - if len(chain) == 1 { - return func(ctx Context, tx Tx, simulate bool) (Context, error) { - return chain[0].AnteHandle(ctx, tx, simulate, nil) - } + // handle non-terminated decorators chain + if (chain[len(chain)-1] != Terminator{}) { + chain = append(chain, Terminator{}) } return func(ctx Context, tx Tx, simulate bool) (Context, error) { diff --git a/types/handler_test.go b/types/handler_test.go new file mode 100644 index 000000000000..4847a113137a --- /dev/null +++ b/types/handler_test.go @@ -0,0 +1,28 @@ +package types_test + +import ( + "testing" + + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" + + "github.com/cosmos/cosmos-sdk/tests/mocks" + sdk "github.com/cosmos/cosmos-sdk/types" +) + +func TestChainAnteDecorators(t *testing.T) { + t.Parallel() + // test panic + require.Nil(t, sdk.ChainAnteDecorators([]sdk.AnteDecorator{}...)) + + ctx, tx := sdk.Context{}, sdk.Tx(nil) + mockCtrl := gomock.NewController(t) + mockAnteDecorator1 := mocks.NewMockAnteDecorator(mockCtrl) + mockAnteDecorator1.EXPECT().AnteHandle(gomock.Eq(ctx), gomock.Eq(tx), true, gomock.Any()).Times(1) + sdk.ChainAnteDecorators(mockAnteDecorator1)(ctx, tx, true) + + mockAnteDecorator2 := mocks.NewMockAnteDecorator(mockCtrl) + mockAnteDecorator1.EXPECT().AnteHandle(gomock.Eq(ctx), gomock.Eq(tx), true, mockAnteDecorator2).Times(1) + mockAnteDecorator2.EXPECT().AnteHandle(gomock.Eq(ctx), gomock.Eq(tx), true, nil).Times(1) + sdk.ChainAnteDecorators(mockAnteDecorator1, mockAnteDecorator2) +}