From d8b03ea89dfda5520810caa8df55a352bf69b828 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 4 Dec 2023 18:25:02 +0200 Subject: [PATCH] Don't panic during any store operations. (backport #5294) (#5303) * Don't panic during any store operations. (#5294) * Don't panic during any store operations. * Panic when creating wrapped store if substores are nil. * Address Damian's feedback. (cherry picked from commit b7d6a3056df7dc6b305de491f55d40cefc18c8ec) # Conflicts: # modules/light-clients/08-wasm/types/export_test.go # modules/light-clients/08-wasm/types/store_test.go * fix conflicts. --------- Co-authored-by: DimitrisJim --- .../08-wasm/types/export_test.go | 2 +- modules/light-clients/08-wasm/types/store.go | 83 ++++++--- .../light-clients/08-wasm/types/store_test.go | 168 ++++++++++++------ 3 files changed, 171 insertions(+), 82 deletions(-) diff --git a/modules/light-clients/08-wasm/types/export_test.go b/modules/light-clients/08-wasm/types/export_test.go index f9b3e40b176..4d9c477566f 100644 --- a/modules/light-clients/08-wasm/types/export_test.go +++ b/modules/light-clients/08-wasm/types/export_test.go @@ -30,7 +30,7 @@ func NewMigrateProposalWrappedStore(subjectStore, substituteStore sdk.KVStore) m } // GetStore is a wrapper around getStore to allow the function to be directly called in tests. -func (ws migrateClientWrappedStore) GetStore(key []byte) sdk.KVStore { +func (ws migrateClientWrappedStore) GetStore(key []byte) (sdk.KVStore, bool) { return ws.getStore(key) } diff --git a/modules/light-clients/08-wasm/types/store.go b/modules/light-clients/08-wasm/types/store.go index 37da73cb739..a82ed33a1e5 100644 --- a/modules/light-clients/08-wasm/types/store.go +++ b/modules/light-clients/08-wasm/types/store.go @@ -3,7 +3,6 @@ package types import ( "bytes" "errors" - "fmt" "io" "reflect" "strings" @@ -29,13 +28,20 @@ var ( // // Both stores are used for reads, but only the subjectStore is used for writes. For all operations, the key // is checked to determine which store to use and must be prefixed with either "subject/" or "substitute/" accordingly. -// If the key is not prefixed with either "subject/" or "substitute/", a panic is thrown. +// If the key is not prefixed with either "subject/" or "substitute/", a default action is taken (e.g. no-op for Set/Delete). type migrateClientWrappedStore struct { subjectStore storetypes.KVStore substituteStore storetypes.KVStore } func newMigrateClientWrappedStore(subjectStore, substituteStore storetypes.KVStore) migrateClientWrappedStore { + if subjectStore == nil { + panic(errors.New("subjectStore must not be nil")) + } + if substituteStore == nil { + panic(errors.New("substituteStore must not be nil")) + } + return migrateClientWrappedStore{ subjectStore: subjectStore, substituteStore: substituteStore, @@ -44,11 +50,17 @@ func newMigrateClientWrappedStore(subjectStore, substituteStore storetypes.KVSto // Get implements the storetypes.KVStore interface. It allows reads from both the subjectStore and substituteStore. // -// Get will panic if the key is not prefixed with either "subject/" or "substitute/". +// Get will return an empty byte slice if the key is not prefixed with either "subject/" or "substitute/". func (ws migrateClientWrappedStore) Get(key []byte) []byte { prefix, key := splitPrefix(key) - return ws.getStore(prefix).Get(key) + store, found := ws.getStore(prefix) + if !found { + // return a nil byte slice as KVStore.Get() does by default + return []byte(nil) + } + + return store.Get(key) } // Has implements the storetypes.KVStore interface. It allows reads from both the subjectStore and substituteStore. @@ -57,16 +69,22 @@ func (ws migrateClientWrappedStore) Get(key []byte) []byte { func (ws migrateClientWrappedStore) Has(key []byte) bool { prefix, key := splitPrefix(key) - return ws.getStore(prefix).Has(key) + store, found := ws.getStore(prefix) + if !found { + // return false as value when store is not found + return false + } + + return store.Has(key) } // Set implements the storetypes.KVStore interface. It allows writes solely to the subjectStore. // -// Set will panic if the key is not prefixed with "subject/". +// Set will no-op if the key is not prefixed with "subject/". func (ws migrateClientWrappedStore) Set(key, value []byte) { prefix, key := splitPrefix(key) if !bytes.Equal(prefix, subjectPrefix) { - panic(fmt.Errorf("writes only allowed on subject store; key must be prefixed with \"%s\"", subjectPrefix)) + return // no-op } ws.subjectStore.Set(key, value) @@ -74,11 +92,11 @@ func (ws migrateClientWrappedStore) Set(key, value []byte) { // Delete implements the storetypes.KVStore interface. It allows deletions solely to the subjectStore. // -// Delete will panic if the key is not prefixed with "subject/". +// Delete will no-op if the key is not prefixed with "subject/". func (ws migrateClientWrappedStore) Delete(key []byte) { prefix, key := splitPrefix(key) if !bytes.Equal(prefix, subjectPrefix) { - panic(fmt.Errorf("writes only allowed on subject store; key must be prefixed with \"%s\"", subjectPrefix)) + return // no-op } ws.subjectStore.Delete(key) @@ -86,30 +104,40 @@ func (ws migrateClientWrappedStore) Delete(key []byte) { // Iterator implements the storetypes.KVStore interface. It allows iteration over both the subjectStore and substituteStore. // -// Iterator will panic if the start or end keys are not prefixed with either "subject/" or "substitute/". +// Iterator will return a closed iterator if the start or end keys are not prefixed with either "subject/" or "substitute/". func (ws migrateClientWrappedStore) Iterator(start, end []byte) storetypes.Iterator { prefixStart, start := splitPrefix(start) prefixEnd, end := splitPrefix(end) if !bytes.Equal(prefixStart, prefixEnd) { - panic(errors.New("start and end keys must be prefixed with the same prefix")) + return ws.closedIterator() } - return ws.getStore(prefixStart).Iterator(start, end) + store, found := ws.getStore(prefixStart) + if !found { + return ws.closedIterator() + } + + return store.Iterator(start, end) } // ReverseIterator implements the storetypes.KVStore interface. It allows iteration over both the subjectStore and substituteStore. // -// ReverseIterator will panic if the start or end keys are not prefixed with either "subject/" or "substitute/". +// ReverseIterator will return a closed iterator if the start or end keys are not prefixed with either "subject/" or "substitute/". func (ws migrateClientWrappedStore) ReverseIterator(start, end []byte) storetypes.Iterator { prefixStart, start := splitPrefix(start) prefixEnd, end := splitPrefix(end) if !bytes.Equal(prefixStart, prefixEnd) { - panic(errors.New("start and end keys must be prefixed with the same prefix")) + return ws.closedIterator() + } + + store, found := ws.getStore(prefixStart) + if !found { + return ws.closedIterator() } - return ws.getStore(prefixStart).ReverseIterator(start, end) + return store.ReverseIterator(start, end) } // GetStoreType implements the storetypes.KVStore interface, it is implemented solely to satisfy the interface. @@ -127,18 +155,29 @@ func (ws migrateClientWrappedStore) CacheWrapWithTrace(w io.Writer, tc storetype return cachekv.NewStore(tracekv.NewStore(ws, w, tc)) } -// getStore returns the store to be used for the given key. If the key is prefixed with "subject/", the subjectStore -// is returned. If the key is prefixed with "substitute/", the substituteStore is returned. +// getStore returns the store to be used for the given key and a boolean flag indicating if that store was found. +// If the key is prefixed with "subject/", the subjectStore is returned. If the key is prefixed with "substitute/", +// the substituteStore is returned. // -// If the key is not prefixed with either "subject/" or "substitute/", a panic is thrown. -func (ws migrateClientWrappedStore) getStore(prefix []byte) storetypes.KVStore { +// If the key is not prefixed with either "subject/" or "substitute/", a nil store is returned and the boolean flag is false. +func (ws migrateClientWrappedStore) getStore(prefix []byte) (storetypes.KVStore, bool) { if bytes.Equal(prefix, subjectPrefix) { - return ws.subjectStore + return ws.subjectStore, true } else if bytes.Equal(prefix, substitutePrefix) { - return ws.substituteStore + return ws.substituteStore, true } - panic(fmt.Errorf("key must be prefixed with either \"%s\" or \"%s\"", subjectPrefix, substitutePrefix)) + return nil, false +} + +// closedIterator returns an iterator that is always closed, used when Iterator() or ReverseIterator() is called +// with an invalid prefix or start/end key. +func (ws migrateClientWrappedStore) closedIterator() storetypes.Iterator { + // Create a dummy iterator that is always closed right away. + it := ws.subjectStore.Iterator([]byte{0}, []byte{1}) + it.Close() + + return it } // splitPrefix splits the key into the prefix and the key itself, if the key is prefixed with either "subject/" or "substitute/". diff --git a/modules/light-clients/08-wasm/types/store_test.go b/modules/light-clients/08-wasm/types/store_test.go index b98b5fc850d..71af589dd15 100644 --- a/modules/light-clients/08-wasm/types/store_test.go +++ b/modules/light-clients/08-wasm/types/store_test.go @@ -1,9 +1,6 @@ package types_test import ( - "errors" - fmt "fmt" - prefixstore "github.com/cosmos/cosmos-sdk/store/prefix" storetypes "github.com/cosmos/cosmos-sdk/types" @@ -23,31 +20,26 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreGetStore() { name string prefix []byte expStore storetypes.KVStore - expPanic error }{ { "success: subject store", types.SubjectPrefix, subjectStore, - nil, }, { "success: substitute store", types.SubstitutePrefix, substituteStore, - nil, }, { "failure: invalid prefix", invalidPrefix, nil, - fmt.Errorf("key must be prefixed with either \"%s\" or \"%s\"", types.SubjectPrefix, types.SubstitutePrefix), }, { "failure: invalid prefix contains both subject/ and substitute/", append(types.SubjectPrefix, types.SubstitutePrefix...), nil, - fmt.Errorf("key must be prefixed with either \"%s\" or \"%s\"", types.SubjectPrefix, types.SubstitutePrefix), }, } @@ -56,10 +48,15 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreGetStore() { suite.Run(tc.name, func() { wrappedStore := types.NewMigrateProposalWrappedStore(subjectStore, substituteStore) - if tc.expPanic == nil { - suite.Require().Equal(tc.expStore, wrappedStore.GetStore(tc.prefix)) + store, found := wrappedStore.GetStore(tc.prefix) + + storeFound := tc.expStore != nil + if storeFound { + suite.Require().Equal(tc.expStore, store) + suite.Require().True(found) } else { - suite.Require().PanicsWithError(tc.expPanic.Error(), func() { wrappedStore.GetStore(tc.prefix) }) + suite.Require().Nil(store) + suite.Require().False(found) } }) } @@ -117,28 +114,24 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreGet() { prefix []byte key []byte expStore storetypes.KVStore - expPanic error }{ { "success: subject store Get", types.SubjectPrefix, host.ClientStateKey(), subjectStore, - nil, }, { "success: substitute store Get", types.SubstitutePrefix, host.ClientStateKey(), substituteStore, - nil, }, { "failure: key not prefixed with subject/ or substitute/", invalidPrefix, host.ClientStateKey(), nil, - fmt.Errorf("key must be prefixed with either \"%s\" or \"%s\"", types.SubjectPrefix, types.SubstitutePrefix), }, } @@ -150,12 +143,14 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreGet() { prefixedKey := tc.prefix prefixedKey = append(prefixedKey, tc.key...) - if tc.expPanic == nil { + storeFound := tc.expStore != nil + if storeFound { expValue := tc.expStore.Get(tc.key) suite.Require().Equal(expValue, wrappedStore.Get(prefixedKey)) } else { - suite.Require().PanicsWithError(tc.expPanic.Error(), func() { wrappedStore.Get(prefixedKey) }) + // expected value when store is not found is an empty byte slice + suite.Require().Equal([]byte(nil), wrappedStore.Get(prefixedKey)) } }) } @@ -163,48 +158,50 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreGet() { // TestMigrateClientWrappedStoreSet tests the Set method of the migrateClientWrappedStore. func (suite *TypesTestSuite) TestMigrateClientWrappedStoreSet() { - // calls suite.SetupWasmWithMockVM() and creates two clients with their respective stores - subjectStore, substituteStore := suite.GetSubjectAndSubstituteStore() - testCases := []struct { - name string - prefix []byte - key []byte - expStore storetypes.KVStore - expPanic error + name string + prefix []byte + key []byte + expSet bool }{ { "success: subject store Set", types.SubjectPrefix, host.ClientStateKey(), - subjectStore, - nil, + true, }, { "failure: cannot Set on substitute store", types.SubstitutePrefix, host.ClientStateKey(), - nil, - fmt.Errorf("writes only allowed on subject store; key must be prefixed with \"%s\"", types.SubjectPrefix), + false, }, } for _, tc := range testCases { tc := tc suite.Run(tc.name, func() { + // calls suite.SetupWasmWithMockVM() and creates two clients with their respective stores + subjectStore, substituteStore := suite.GetSubjectAndSubstituteStore() wrappedStore := types.NewMigrateProposalWrappedStore(subjectStore, substituteStore) prefixedKey := tc.prefix prefixedKey = append(prefixedKey, tc.key...) - if tc.expPanic == nil { - wrappedStore.Set(prefixedKey, wasmtesting.MockClientStateBz) + wrappedStore.Set(prefixedKey, wasmtesting.MockClientStateBz) - expValue := tc.expStore.Get(tc.key) + if tc.expSet { + store, found := wrappedStore.GetStore(tc.prefix) + suite.Require().True(found) + suite.Require().Equal(subjectStore, store) + + value := store.Get(tc.key) - suite.Require().Equal(expValue, wasmtesting.MockClientStateBz) + suite.Require().Equal(wasmtesting.MockClientStateBz, value) } else { - suite.Require().PanicsWithError(tc.expPanic.Error(), func() { wrappedStore.Set(prefixedKey, wasmtesting.MockClientStateBz) }) + // Assert that no writes happened to subject or substitute store + suite.Require().NotEqual(wasmtesting.MockClientStateBz, subjectStore.Get(tc.key)) + suite.Require().NotEqual(wasmtesting.MockClientStateBz, substituteStore.Get(tc.key)) } }) } @@ -212,46 +209,48 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreSet() { // TestMigrateClientWrappedStoreDelete tests the Delete method of the migrateClientWrappedStore. func (suite *TypesTestSuite) TestMigrateClientWrappedStoreDelete() { - // calls suite.SetupWasmWithMockVM() and creates two clients with their respective stores - subjectStore, substituteStore := suite.GetSubjectAndSubstituteStore() - testCases := []struct { - name string - prefix []byte - key []byte - expStore storetypes.KVStore - expPanic error + name string + prefix []byte + key []byte + expDelete bool }{ { "success: subject store Delete", types.SubjectPrefix, host.ClientStateKey(), - subjectStore, - nil, + true, }, { "failure: cannot Delete on substitute store", types.SubstitutePrefix, host.ClientStateKey(), - nil, - fmt.Errorf("writes only allowed on subject store; key must be prefixed with \"%s\"", types.SubjectPrefix), + false, }, } for _, tc := range testCases { tc := tc suite.Run(tc.name, func() { + // calls suite.SetupWasmWithMockVM() and creates two clients with their respective stores + subjectStore, substituteStore := suite.GetSubjectAndSubstituteStore() wrappedStore := types.NewMigrateProposalWrappedStore(subjectStore, substituteStore) prefixedKey := tc.prefix prefixedKey = append(prefixedKey, tc.key...) - if tc.expPanic == nil { - wrappedStore.Delete(prefixedKey) + wrappedStore.Delete(prefixedKey) - suite.Require().False(tc.expStore.Has(tc.key)) + if tc.expDelete { + store, found := wrappedStore.GetStore(tc.prefix) + suite.Require().True(found) + suite.Require().Equal(subjectStore, store) + + suite.Require().False(store.Has(tc.key)) } else { - suite.Require().PanicsWithError(tc.expPanic.Error(), func() { wrappedStore.Delete(prefixedKey) }) + // Assert that no deletions happened to subject or substitute store + suite.Require().True(subjectStore.Has(tc.key)) + suite.Require().True(substituteStore.Has(tc.key)) } }) } @@ -268,7 +267,7 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreIterators() { prefixEnd []byte start []byte end []byte - expPanic error + expValid bool }{ { "success: subject store Iterate", @@ -276,7 +275,7 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreIterators() { types.SubjectPrefix, []byte("start"), []byte("end"), - nil, + true, }, { "success: substitute store Iterate", @@ -284,7 +283,7 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreIterators() { types.SubstitutePrefix, []byte("start"), []byte("end"), - nil, + true, }, { "failure: key not prefixed", @@ -292,7 +291,7 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreIterators() { invalidPrefix, []byte("start"), []byte("end"), - fmt.Errorf("key must be prefixed with either \"%s\" or \"%s\"", types.SubjectPrefix, types.SubstitutePrefix), + false, }, { "failure: start and end keys not prefixed with same prefix", @@ -300,7 +299,7 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreIterators() { types.SubstitutePrefix, []byte("start"), []byte("end"), - errors.New("start and end keys must be prefixed with the same prefix"), + false, }, } @@ -314,12 +313,62 @@ func (suite *TypesTestSuite) TestMigrateClientWrappedStoreIterators() { prefixedKeyEnd := tc.prefixEnd prefixedKeyEnd = append(prefixedKeyEnd, tc.end...) - if tc.expPanic == nil { + if tc.expValid { suite.Require().NotNil(wrappedStore.Iterator(prefixedKeyStart, prefixedKeyEnd)) suite.Require().NotNil(wrappedStore.ReverseIterator(prefixedKeyStart, prefixedKeyEnd)) } else { - suite.Require().PanicsWithError(tc.expPanic.Error(), func() { wrappedStore.Iterator(prefixedKeyStart, prefixedKeyEnd) }) - suite.Require().PanicsWithError(tc.expPanic.Error(), func() { wrappedStore.ReverseIterator(prefixedKeyStart, prefixedKeyEnd) }) + // Iterator returned should be Closed, calling `Valid` should return false + suite.Require().False(wrappedStore.Iterator(prefixedKeyStart, prefixedKeyEnd).Valid()) + suite.Require().False(wrappedStore.ReverseIterator(prefixedKeyStart, prefixedKeyEnd).Valid()) + } + }) + } +} + +func (suite *TypesTestSuite) TestNewMigrateClientWrappedStore() { + // calls suite.SetupWasmWithMockVM() and creates two clients with their respective stores + subjectStore, substituteStore := suite.GetSubjectAndSubstituteStore() + + testCases := []struct { + name string + malleate func() + expPanic bool + }{ + { + "success", + func() {}, + false, + }, + { + "failure: subject store is nil", + func() { + subjectStore = nil + }, + true, + }, + { + "failure: substitute store is nil", + func() { + substituteStore = nil + }, + true, + }, + } + + for _, tc := range testCases { + tc := tc + suite.Run(tc.name, func() { + tc.malleate() + + expPass := !tc.expPanic + if expPass { + suite.Require().NotPanics(func() { + types.NewMigrateProposalWrappedStore(subjectStore, substituteStore) + }) + } else { + suite.Require().Panics(func() { + types.NewMigrateProposalWrappedStore(subjectStore, substituteStore) + }) } }) } @@ -341,7 +390,8 @@ func (suite *TypesTestSuite) TestGetClientID() { { "success: clientID retrieved from migrateClientWrappedStore", func() { - clientStore = types.NewMigrateProposalWrappedStore(clientStore, nil) + // substituteStore is ignored. + clientStore = types.NewMigrateProposalWrappedStore(clientStore, clientStore) }, nil, },