diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c071f7b1..b046b0248 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -313,6 +313,7 @@ pagination. * (types) [\#6195](https://github.com/cosmos/cosmos-sdk/pull/6195) Add codespace to broadcast(sync/async) response. * (baseapp) [\#6053](https://github.com/cosmos/cosmos-sdk/pull/6053) Customizable panic recovery handling added for `app.runTx()` method (as proposed in the [ADR 22](https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-022-custom-panic-handling.md)). Adds ability for developers to register custom panic handlers extending standard ones. * (store) [\#6481](https://github.com/cosmos/cosmos-sdk/pull/6481) Move `SimpleProofsFromMap` from Tendermint into the SDK. +* (store) [\#6719](https://github.com/cosmos/cosmos-sdk/6754) Add validity checks to stores for nil and empty keys. ## [v0.38.5] - 2020-07-02 diff --git a/client/testutil/suite.go b/client/testutil/suite.go index c6d35c181..5aa8a5e3e 100644 --- a/client/testutil/suite.go +++ b/client/testutil/suite.go @@ -3,11 +3,12 @@ package testutil import ( "bytes" + "github.com/stretchr/testify/suite" + "github.com/tendermint/tendermint/crypto" + "github.com/cosmos/cosmos-sdk/crypto/types/multisig" "github.com/cosmos/cosmos-sdk/testutil/testdata" "github.com/cosmos/cosmos-sdk/x/auth/signing" - "github.com/stretchr/testify/suite" - "github.com/tendermint/tendermint/crypto" "github.com/cosmos/cosmos-sdk/client" diff --git a/client/utils.go b/client/utils.go index 53b76788e..19f4b3049 100644 --- a/client/utils.go +++ b/client/utils.go @@ -1,9 +1,10 @@ package client import ( + "github.com/spf13/pflag" + "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/types/query" - "github.com/spf13/pflag" ) // Paginate returns the correct starting and ending index for a paginated query, diff --git a/server/mock/store.go b/server/mock/store.go index bf8e66a4d..d8e9cee1c 100644 --- a/server/mock/store.go +++ b/server/mock/store.go @@ -131,6 +131,7 @@ func (kv kvStore) Has(key []byte) bool { } func (kv kvStore) Set(key, value []byte) { + store.AssertValidKey(key) kv.store[string(key)] = value } diff --git a/server/mock/store_test.go b/server/mock/store_test.go index 28959f03b..b945cd305 100644 --- a/server/mock/store_test.go +++ b/server/mock/store_test.go @@ -30,4 +30,6 @@ func TestStore(t *testing.T) { require.Equal(t, v, store.Get(k)) store.Delete(k) require.False(t, store.Has(k)) + require.Panics(t, func() { store.Set([]byte(""), v) }, "setting an empty key should panic") + require.Panics(t, func() { store.Set(nil, v) }, "setting a nil key should panic") } diff --git a/store/cachekv/store_test.go b/store/cachekv/store_test.go index 4a115d3cb..e3b33341b 100644 --- a/store/cachekv/store_test.go +++ b/store/cachekv/store_test.go @@ -68,6 +68,8 @@ func TestCacheKVStoreNoNilSet(t *testing.T) { mem := dbadapter.Store{DB: dbm.NewMemDB()} st := cachekv.NewStore(mem) require.Panics(t, func() { st.Set([]byte("key"), nil) }, "setting a nil value should panic") + require.Panics(t, func() { st.Set(nil, []byte("value")) }, "setting a nil key should panic") + require.Panics(t, func() { st.Set([]byte(""), []byte("value")) }, "setting an empty key should panic") } func TestCacheKVStoreNested(t *testing.T) { diff --git a/store/dbadapter/store.go b/store/dbadapter/store.go index 0c281c272..fcaeac15a 100644 --- a/store/dbadapter/store.go +++ b/store/dbadapter/store.go @@ -37,6 +37,7 @@ func (dsa Store) Has(key []byte) bool { // Set wraps the underlying DB's Set method panicing on error. func (dsa Store) Set(key, value []byte) { + types.AssertValidKey(key) if err := dsa.DB.Set(key, value); err != nil { panic(err) } diff --git a/store/dbadapter/store_test.go b/store/dbadapter/store_test.go index a29e3f6c8..c09f09331 100644 --- a/store/dbadapter/store_test.go +++ b/store/dbadapter/store_test.go @@ -24,6 +24,9 @@ func TestAccessors(t *testing.T) { key := []byte("test") value := []byte("testvalue") + require.Panics(t, func() { store.Set(nil, []byte("value")) }, "setting a nil key should panic") + require.Panics(t, func() { store.Set([]byte(""), []byte("value")) }, "setting an empty key should panic") + require.Equal(t, types.StoreTypeDB, store.GetStoreType()) store.GetStoreType() diff --git a/store/gaskv/store.go b/store/gaskv/store.go index c53a07af3..b2b4b792a 100644 --- a/store/gaskv/store.go +++ b/store/gaskv/store.go @@ -51,6 +51,7 @@ func (gs *Store) Get(key []byte) (value []byte) { func (gs *Store) Set(key []byte, value []byte) { defer telemetry.MeasureSince(time.Now(), "store", "gaskv", "set") + types.AssertValidKey(key) types.AssertValidValue(value) gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostFlat, types.GasWriteCostFlatDesc) // TODO overflow-safe math? diff --git a/store/gaskv/store_test.go b/store/gaskv/store_test.go index ec0d42be8..55378666c 100644 --- a/store/gaskv/store_test.go +++ b/store/gaskv/store_test.go @@ -27,6 +27,9 @@ func TestGasKVStoreBasic(t *testing.T) { require.Panics(t, func() { st.CacheWrap() }) require.Panics(t, func() { st.CacheWrapWithTrace(nil, nil) }) + require.Panics(t, func() { st.Set(nil, []byte("value")) }, "setting a nil key should panic") + require.Panics(t, func() { st.Set([]byte(""), []byte("value")) }, "setting an empty key should panic") + require.Empty(t, st.Get(keyFmt(1)), "Expected `key1` to be empty") st.Set(keyFmt(1), valFmt(1)) require.Equal(t, valFmt(1), st.Get(keyFmt(1))) diff --git a/store/iavl/store.go b/store/iavl/store.go index 628df8ed7..cf3ecb681 100644 --- a/store/iavl/store.go +++ b/store/iavl/store.go @@ -147,6 +147,7 @@ func (st *Store) CacheWrapWithTrace(w io.Writer, tc types.TraceContext) types.Ca // Implements types.KVStore. func (st *Store) Set(key, value []byte) { defer telemetry.MeasureSince(time.Now(), "store", "iavl", "set") + types.AssertValidKey(key) types.AssertValidValue(value) st.tree.Set(key, value) } diff --git a/store/iavl/store_test.go b/store/iavl/store_test.go index 07d364697..27ebc4dbd 100644 --- a/store/iavl/store_test.go +++ b/store/iavl/store_test.go @@ -131,6 +131,10 @@ func TestIAVLStoreNoNilSet(t *testing.T) { db := dbm.NewMemDB() tree, _ := newAlohaTree(t, db) iavlStore := UnsafeNewStore(tree) + + require.Panics(t, func() { iavlStore.Set(nil, []byte("value")) }, "setting a nil key should panic") + require.Panics(t, func() { iavlStore.Set([]byte(""), []byte("value")) }, "setting an empty key should panic") + require.Panics(t, func() { iavlStore.Set([]byte("key"), nil) }, "setting a nil value should panic") } diff --git a/store/prefix/store_test.go b/store/prefix/store_test.go index b564e6f34..c46aec6f1 100644 --- a/store/prefix/store_test.go +++ b/store/prefix/store_test.go @@ -246,7 +246,6 @@ func mockStoreWithStuff() types.KVStore { store.Set(bz("key2"), bz("value2")) store.Set(bz("key3"), bz("value3")) store.Set(bz("something"), bz("else")) - store.Set(bz(""), bz("")) store.Set(bz("k"), bz(sdk.PrefixValidator)) store.Set(bz("ke"), bz("valu")) store.Set(bz("kee"), bz("valuu")) diff --git a/store/rootmulti/internal/maps/maps.go b/store/rootmulti/internal/maps/maps.go index 1dfdca6ff..feb52bafb 100644 --- a/store/rootmulti/internal/maps/maps.go +++ b/store/rootmulti/internal/maps/maps.go @@ -6,6 +6,8 @@ import ( "github.com/tendermint/tendermint/crypto/merkle" "github.com/tendermint/tendermint/crypto/tmhash" "github.com/tendermint/tendermint/libs/kv" + + "github.com/cosmos/cosmos-sdk/store/types" ) // merkleMap defines a merkle-ized tree from a map. Leave values are treated as @@ -26,6 +28,9 @@ func newMerkleMap() *merkleMap { // to creating a kv.Pair. The created kv.Pair is appended to the MerkleMap's slice // of kv.Pairs. Whenever called, the MerkleMap must be resorted. func (sm *merkleMap) set(key string, value []byte) { + byteKey := []byte(key) + types.AssertValidKey(byteKey) + sm.sorted = false // The value is hashed, so you can check for equality with a cached value (say) @@ -33,7 +38,7 @@ func (sm *merkleMap) set(key string, value []byte) { vhash := tmhash.Sum(value) sm.kvs = append(sm.kvs, kv.Pair{ - Key: []byte(key), + Key: byteKey, Value: vhash, }) } @@ -84,6 +89,8 @@ func newSimpleMap() *simpleMap { // Set creates a kv pair of the key and the hash of the value, // and then appends it to SimpleMap's kv pairs. func (sm *simpleMap) Set(key string, value []byte) { + byteKey := []byte(key) + types.AssertValidKey(byteKey) sm.sorted = false // The value is hashed, so you can @@ -92,7 +99,7 @@ func (sm *simpleMap) Set(key string, value []byte) { vhash := tmhash.Sum(value) sm.Kvs = append(sm.Kvs, kv.Pair{ - Key: []byte(key), + Key: byteKey, Value: vhash, }) } diff --git a/store/rootmulti/internal/maps/maps_test.go b/store/rootmulti/internal/maps/maps_test.go index cb8b30527..51b411310 100644 --- a/store/rootmulti/internal/maps/maps_test.go +++ b/store/rootmulti/internal/maps/maps_test.go @@ -5,8 +5,13 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestEmptyKeyMerkleMap(t *testing.T) { + db := newMerkleMap() + require.Panics(t, func() { db.set("", []byte("value")) }, "setting an empty key should panic") +} func TestMerkleMap(t *testing.T) { tests := []struct { keys []string @@ -49,6 +54,10 @@ func TestMerkleMap(t *testing.T) { } } +func TestEmptyKeySimpleMap(t *testing.T) { + db := newSimpleMap() + require.Panics(t, func() { db.Set("", []byte("value")) }, "setting an empty key should panic") +} func TestSimpleMap(t *testing.T) { tests := []struct { keys []string diff --git a/store/tracekv/store.go b/store/tracekv/store.go index b23a97c3e..ba0bd8bf3 100644 --- a/store/tracekv/store.go +++ b/store/tracekv/store.go @@ -60,6 +60,7 @@ func (tkv *Store) Get(key []byte) []byte { // Set implements the KVStore interface. It traces a write operation and // delegates the Set call to the parent KVStore. func (tkv *Store) Set(key []byte, value []byte) { + types.AssertValidKey(key) writeOperation(tkv.writer, writeOp, tkv.context, key, value) tkv.parent.Set(key, value) } diff --git a/store/tracekv/store_test.go b/store/tracekv/store_test.go index 99b17233e..ec2e95567 100644 --- a/store/tracekv/store_test.go +++ b/store/tracekv/store_test.go @@ -85,16 +85,21 @@ func TestTraceKVStoreSet(t *testing.T) { value []byte expectedOut string }{ - { - key: []byte{}, - value: nil, - expectedOut: "{\"operation\":\"write\",\"key\":\"\",\"value\":\"\",\"metadata\":{\"blockHeight\":64}}\n", - }, { key: kvPairs[0].Key, value: kvPairs[0].Value, expectedOut: "{\"operation\":\"write\",\"key\":\"a2V5MDAwMDAwMDE=\",\"value\":\"dmFsdWUwMDAwMDAwMQ==\",\"metadata\":{\"blockHeight\":64}}\n", }, + { + key: kvPairs[1].Key, + value: kvPairs[1].Value, + expectedOut: "{\"operation\":\"write\",\"key\":\"a2V5MDAwMDAwMDI=\",\"value\":\"dmFsdWUwMDAwMDAwMg==\",\"metadata\":{\"blockHeight\":64}}\n", + }, + { + key: kvPairs[2].Key, + value: kvPairs[2].Value, + expectedOut: "{\"operation\":\"write\",\"key\":\"a2V5MDAwMDAwMDM=\",\"value\":\"dmFsdWUwMDAwMDAwMw==\",\"metadata\":{\"blockHeight\":64}}\n", + }, } for _, tc := range testCases { @@ -106,6 +111,12 @@ func TestTraceKVStoreSet(t *testing.T) { require.Equal(t, tc.expectedOut, buf.String()) } + + var buf bytes.Buffer + store := newEmptyTraceKVStore(&buf) + require.Panics(t, func() { store.Set([]byte(""), []byte("value")) }, "setting an empty key should panic") + require.Panics(t, func() { store.Set(nil, []byte("value")) }, "setting a nil key should panic") + } func TestTraceKVStoreDelete(t *testing.T) { diff --git a/store/types/store.go b/store/types/store.go index a1dfc4adc..fd4e33596 100644 --- a/store/types/store.go +++ b/store/types/store.go @@ -320,6 +320,9 @@ type KVStoreKey struct { // NewKVStoreKey returns a new pointer to a KVStoreKey. // Use a pointer so keys don't collide. func NewKVStoreKey(name string) *KVStoreKey { + if name == "" { + panic("empty key name not allowed") + } return &KVStoreKey{ name: name, } diff --git a/store/types/store_test.go b/store/types/store_test.go index bf70af1bb..f86144af7 100644 --- a/store/types/store_test.go +++ b/store/types/store_test.go @@ -75,6 +75,14 @@ func TestKVStoreKey(t *testing.T) { require.Equal(t, fmt.Sprintf("KVStoreKey{%p, test}", key), key.String()) } +func TestNilKVStoreKey(t *testing.T) { + t.Parallel() + + require.Panics(t, func() { + _ = NewKVStoreKey("") + }, "setting an empty key should panic") +} + func TestTransientStoreKey(t *testing.T) { t.Parallel() key := NewTransientStoreKey("test") diff --git a/store/types/validity.go b/store/types/validity.go index 59dbd0c03..ef2387a69 100644 --- a/store/types/validity.go +++ b/store/types/validity.go @@ -2,7 +2,7 @@ package types // Check if the key is valid(key is not nil) func AssertValidKey(key []byte) { - if key == nil { + if len(key) == 0 { panic("key is nil") } } diff --git a/store/types/validity_test.go b/store/types/validity_test.go index e2adcd19c..0f5d0466a 100644 --- a/store/types/validity_test.go +++ b/store/types/validity_test.go @@ -10,8 +10,8 @@ import ( func TestAssertValidKey(t *testing.T) { t.Parallel() - require.NotPanics(t, func() { types.AssertValidKey([]byte{}) }) require.NotPanics(t, func() { types.AssertValidKey([]byte{0x01}) }) + require.Panics(t, func() { types.AssertValidKey([]byte{}) }) require.Panics(t, func() { types.AssertValidKey(nil) }) } diff --git a/testutil/testdata/test_tx.go b/testutil/testdata/test_tx.go index 4ba9b6066..92972fe41 100644 --- a/testutil/testdata/test_tx.go +++ b/testutil/testdata/test_tx.go @@ -3,9 +3,10 @@ package testdata import ( "encoding/json" - sdk "github.com/cosmos/cosmos-sdk/types" "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/crypto/secp256k1" + + sdk "github.com/cosmos/cosmos-sdk/types" ) // KeyTestPubAddr generates a new secp256k1 keypair. diff --git a/x/auth/ante/basic_test.go b/x/auth/ante/basic_test.go index b29066198..4e8c83e9c 100644 --- a/x/auth/ante/basic_test.go +++ b/x/auth/ante/basic_test.go @@ -5,9 +5,10 @@ import ( "strings" "testing" - "github.com/cosmos/cosmos-sdk/testutil/testdata" "github.com/stretchr/testify/suite" + "github.com/cosmos/cosmos-sdk/testutil/testdata" + "github.com/tendermint/tendermint/crypto" sdk "github.com/cosmos/cosmos-sdk/types" diff --git a/x/auth/ante/fee_test.go b/x/auth/ante/fee_test.go index b8b4d5971..80f200c61 100644 --- a/x/auth/ante/fee_test.go +++ b/x/auth/ante/fee_test.go @@ -3,9 +3,10 @@ package ante_test import ( "testing" - "github.com/cosmos/cosmos-sdk/testutil/testdata" "github.com/stretchr/testify/suite" + "github.com/cosmos/cosmos-sdk/testutil/testdata" + "github.com/tendermint/tendermint/crypto" sdk "github.com/cosmos/cosmos-sdk/types" diff --git a/x/auth/ante/setup_test.go b/x/auth/ante/setup_test.go index b78585ed3..a6e185d14 100644 --- a/x/auth/ante/setup_test.go +++ b/x/auth/ante/setup_test.go @@ -3,9 +3,10 @@ package ante_test import ( "testing" - "github.com/cosmos/cosmos-sdk/testutil/testdata" "github.com/stretchr/testify/suite" + "github.com/cosmos/cosmos-sdk/testutil/testdata" + "github.com/tendermint/tendermint/crypto" sdk "github.com/cosmos/cosmos-sdk/types" diff --git a/x/auth/ante/sigverify_test.go b/x/auth/ante/sigverify_test.go index f9ac32ec6..e128b9e26 100644 --- a/x/auth/ante/sigverify_test.go +++ b/x/auth/ante/sigverify_test.go @@ -4,9 +4,10 @@ import ( "fmt" "testing" - "github.com/cosmos/cosmos-sdk/testutil/testdata" "github.com/stretchr/testify/suite" + "github.com/cosmos/cosmos-sdk/testutil/testdata" + "github.com/cosmos/cosmos-sdk/types/tx/signing" "github.com/cosmos/cosmos-sdk/simapp" diff --git a/x/auth/signing/sig_verifiable_tx.go b/x/auth/signing/sig_verifiable_tx.go index 599a544ea..d4c5c5c26 100644 --- a/x/auth/signing/sig_verifiable_tx.go +++ b/x/auth/signing/sig_verifiable_tx.go @@ -1,9 +1,10 @@ package signing import ( + "github.com/tendermint/tendermint/crypto" + "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/tx/signing" - "github.com/tendermint/tendermint/crypto" ) // SigVerifiableTx defines a Tx interface for all signature verification decorators diff --git a/x/distribution/keeper/grpc_query.go b/x/distribution/keeper/grpc_query.go index 9367d2d6f..70d377d7e 100644 --- a/x/distribution/keeper/grpc_query.go +++ b/x/distribution/keeper/grpc_query.go @@ -3,14 +3,15 @@ package keeper import ( "context" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "github.com/cosmos/cosmos-sdk/store/prefix" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/query" "github.com/cosmos/cosmos-sdk/x/distribution/types" "github.com/cosmos/cosmos-sdk/x/staking/exported" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) var _ types.QueryServer = Keeper{} diff --git a/x/distribution/keeper/grpc_query_test.go b/x/distribution/keeper/grpc_query_test.go index 7902f5c78..b6fda92d4 100644 --- a/x/distribution/keeper/grpc_query_test.go +++ b/x/distribution/keeper/grpc_query_test.go @@ -5,6 +5,9 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/suite" + abci "github.com/tendermint/tendermint/abci/types" + "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/simapp" sdk "github.com/cosmos/cosmos-sdk/types" @@ -12,8 +15,6 @@ import ( "github.com/cosmos/cosmos-sdk/x/distribution/types" "github.com/cosmos/cosmos-sdk/x/staking" stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" - "github.com/stretchr/testify/suite" - abci "github.com/tendermint/tendermint/abci/types" ) type KeeperTestSuite struct { diff --git a/x/gov/keeper/grpc_query.go b/x/gov/keeper/grpc_query.go index 39f9c5f7e..4b50ffe98 100644 --- a/x/gov/keeper/grpc_query.go +++ b/x/gov/keeper/grpc_query.go @@ -3,12 +3,13 @@ package keeper import ( "context" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "github.com/cosmos/cosmos-sdk/store/prefix" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/query" "github.com/cosmos/cosmos-sdk/x/gov/types" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) var _ types.QueryServer = Keeper{} diff --git a/x/ibc/types/query.go b/x/ibc/types/query.go index d529361d0..07db85b70 100644 --- a/x/ibc/types/query.go +++ b/x/ibc/types/query.go @@ -1,11 +1,12 @@ package types import ( + "github.com/gogo/protobuf/grpc" + connection "github.com/cosmos/cosmos-sdk/x/ibc/03-connection" connectiontypes "github.com/cosmos/cosmos-sdk/x/ibc/03-connection/types" channel "github.com/cosmos/cosmos-sdk/x/ibc/04-channel" channeltypes "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/types" - "github.com/gogo/protobuf/grpc" ) // QueryServer defines the IBC interfaces that the gRPC query server must implement diff --git a/x/mint/keeper/grpc_query_test.go b/x/mint/keeper/grpc_query_test.go index bf3d35d1a..f63b248a2 100644 --- a/x/mint/keeper/grpc_query_test.go +++ b/x/mint/keeper/grpc_query_test.go @@ -4,12 +4,13 @@ import ( gocontext "context" "testing" + "github.com/stretchr/testify/suite" + abci "github.com/tendermint/tendermint/abci/types" + "github.com/cosmos/cosmos-sdk/baseapp" "github.com/cosmos/cosmos-sdk/simapp" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/x/mint/types" - "github.com/stretchr/testify/suite" - abci "github.com/tendermint/tendermint/abci/types" ) type MintTestSuite struct { diff --git a/x/staking/keeper/grpc_query_test.go b/x/staking/keeper/grpc_query_test.go index 2cb88f46f..cb2607010 100644 --- a/x/staking/keeper/grpc_query_test.go +++ b/x/staking/keeper/grpc_query_test.go @@ -583,7 +583,7 @@ func (suite *KeeperTestSuite) TestGRPCQueryRedelegation() { expPass bool expErr bool }{ - {"request redelegations for non existant addr", + {"request redelegations for non existent addr", func() { req = &types.QueryRedelegationsRequest{DelegatorAddr: addrAcc} },