diff --git a/PENDING.md b/PENDING.md index 1ce08770d09f..7814facca7cc 100644 --- a/PENDING.md +++ b/PENDING.md @@ -47,6 +47,7 @@ IMPROVEMENTS genesis validation checks to `GaiaValidateGenesisState`. * SDK + * \#3435 Test that store implementations do not allow nil values * Tendermint diff --git a/store/cachekvstore.go b/store/cachekvstore.go index 4860566c4b6d..28c344fd0b5b 100644 --- a/store/cachekvstore.go +++ b/store/cachekvstore.go @@ -44,7 +44,7 @@ func (ci *cacheKVStore) GetStoreType() StoreType { func (ci *cacheKVStore) Get(key []byte) (value []byte) { ci.mtx.Lock() defer ci.mtx.Unlock() - ci.assertValidKey(key) + assertValidKey(key) cacheValue, ok := ci.cache[string(key)] if !ok { @@ -61,8 +61,8 @@ func (ci *cacheKVStore) Get(key []byte) (value []byte) { func (ci *cacheKVStore) Set(key []byte, value []byte) { ci.mtx.Lock() defer ci.mtx.Unlock() - ci.assertValidKey(key) - ci.assertValidValue(value) + assertValidKey(key) + assertValidValue(value) ci.setCacheValue(key, value, false, true) } @@ -77,7 +77,7 @@ func (ci *cacheKVStore) Has(key []byte) bool { func (ci *cacheKVStore) Delete(key []byte) { ci.mtx.Lock() defer ci.mtx.Unlock() - ci.assertValidKey(key) + assertValidKey(key) ci.setCacheValue(key, nil, true, true) } @@ -189,21 +189,6 @@ func (ci *cacheKVStore) dirtyItems(start, end []byte, ascending bool) []cmn.KVPa return items } -//---------------------------------------- -// etc - -func (ci *cacheKVStore) assertValidKey(key []byte) { - if key == nil { - panic("key is nil") - } -} - -func (ci *cacheKVStore) assertValidValue(value []byte) { - if value == nil { - panic("value is nil") - } -} - // Only entrypoint to mutate ci.cache. func (ci *cacheKVStore) setCacheValue(key, value []byte, deleted bool, dirty bool) { ci.cache[string(key)] = cValue{ diff --git a/store/cachekvstore_test.go b/store/cachekvstore_test.go index 37e0364fb5aa..8f43b0b3bb5e 100644 --- a/store/cachekvstore_test.go +++ b/store/cachekvstore_test.go @@ -60,6 +60,12 @@ func TestCacheKVStore(t *testing.T) { require.Empty(t, mem.Get(keyFmt(1)), "Expected `key1` to be empty") } +func TestCacheKVStoreNoNilSet(t *testing.T) { + mem := dbStoreAdapter{dbm.NewMemDB()} + st := NewCacheKVStore(mem) + require.Panics(t, func() { st.Set([]byte("key"), nil) }, "setting a nil value should panic") +} + func TestCacheKVStoreNested(t *testing.T) { mem := dbStoreAdapter{dbm.NewMemDB()} st := NewCacheKVStore(mem) diff --git a/store/gaskvstore.go b/store/gaskvstore.go index 1100f8433979..790ae612fd54 100644 --- a/store/gaskvstore.go +++ b/store/gaskvstore.go @@ -45,6 +45,7 @@ func (gs *gasKVStore) Get(key []byte) (value []byte) { // Implements KVStore. func (gs *gasKVStore) Set(key []byte, value []byte) { + assertValidValue(value) gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostFlat, sdk.GasWriteCostFlatDesc) // TODO overflow-safe math? gs.gasMeter.ConsumeGas(gs.gasConfig.WriteCostPerByte*sdk.Gas(len(value)), sdk.GasWritePerByteDesc) diff --git a/store/gaskvstore_test.go b/store/gaskvstore_test.go index 2426cf334727..c3a3e3928d1f 100644 --- a/store/gaskvstore_test.go +++ b/store/gaskvstore_test.go @@ -29,6 +29,11 @@ func TestGasKVStoreBasic(t *testing.T) { require.Equal(t, meter.GasConsumed(), sdk.Gas(6429)) } +func TestGasKVStoreNoNilSet(t *testing.T) { + st := newGasKVStore() + require.Panics(t, func() { st.Set([]byte("key"), nil) }, "setting a nil value should panic") +} + func TestGasKVStoreIterator(t *testing.T) { mem := dbStoreAdapter{dbm.NewMemDB()} meter := sdk.NewGasMeter(10000) diff --git a/store/iavlstore.go b/store/iavlstore.go index 26c739da3bf3..9bb6a8cb1413 100644 --- a/store/iavlstore.go +++ b/store/iavlstore.go @@ -128,6 +128,7 @@ func (st *iavlStore) CacheWrapWithTrace(w io.Writer, tc TraceContext) CacheWrap // Implements KVStore. func (st *iavlStore) Set(key, value []byte) { + assertValidValue(value) st.tree.Set(key, value) } diff --git a/store/iavlstore_test.go b/store/iavlstore_test.go index 4d8605e4e198..78140ba327f0 100644 --- a/store/iavlstore_test.go +++ b/store/iavlstore_test.go @@ -69,6 +69,13 @@ func TestIAVLStoreGetSetHasDelete(t *testing.T) { require.False(t, exists) } +func TestIAVLStoreNoNilSet(t *testing.T) { + db := dbm.NewMemDB() + tree, _ := newAlohaTree(t, db) + iavlStore := newIAVLStore(tree, numRecent, storeEvery) + require.Panics(t, func() { iavlStore.Set([]byte("key"), nil) }, "setting a nil value should panic") +} + func TestIAVLIterator(t *testing.T) { db := dbm.NewMemDB() tree, _ := newAlohaTree(t, db) diff --git a/store/prefixstore.go b/store/prefixstore.go index 1f52a99fec48..66a08f0eb707 100644 --- a/store/prefixstore.go +++ b/store/prefixstore.go @@ -61,6 +61,7 @@ func (s prefixStore) Has(key []byte) bool { // Implements KVStore func (s prefixStore) Set(key, value []byte) { + assertValidValue(value) s.parent.Set(s.key(key), value) } diff --git a/store/prefixstore_test.go b/store/prefixstore_test.go index 48e6cd17f7b9..4a4253af461e 100644 --- a/store/prefixstore_test.go +++ b/store/prefixstore_test.go @@ -95,6 +95,13 @@ func TestGasKVStorePrefix(t *testing.T) { testPrefixStore(t, gasStore, []byte("test")) } +func TestPrefixKVStoreNoNilSet(t *testing.T) { + meter := sdk.NewGasMeter(100000000) + mem := dbStoreAdapter{dbm.NewMemDB()} + gasStore := NewGasKVStore(meter, sdk.KVGasConfig(), mem) + require.Panics(t, func() { gasStore.Set([]byte("key"), nil) }, "setting a nil value should panic") +} + func TestPrefixStoreIterate(t *testing.T) { db := dbm.NewMemDB() baseStore := dbStoreAdapter{db} diff --git a/store/validity.go b/store/validity.go new file mode 100644 index 000000000000..5a20f1fb3a21 --- /dev/null +++ b/store/validity.go @@ -0,0 +1,13 @@ +package store + +func assertValidKey(key []byte) { + if key == nil { + panic("key is nil") + } +} + +func assertValidValue(value []byte) { + if value == nil { + panic("value is nil") + } +}