Merge PR #3435: Assert store implementations never store nil values

* Check non-nil values on IAVL store
* Reuse assertValidValue

PENDING.md

@@ -47,6 +47,7 @@ IMPROVEMENTS
   genesis validation checks to `GaiaValidateGenesisState`.
 
 * SDK
+  * \#3435 Test that store implementations do not allow nil values
 
 * Tendermint
 

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{

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)

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)

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)

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)
 }
 

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)

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)
 }
 

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}

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")
+	}
+}