Merge pull request #2698 from oasislabs/kostko/feature/mkvs-fuzzing

 go/storage/mkvs: Add MKVS fuzzing

.changelog/2698.bugfix.1.md

@@ -0,0 +1 @@
+go/storage/mkvs: Fix removal crash when key is too short

.changelog/2698.bugfix.2.md

@@ -0,0 +1 @@
+go/storage/mkvs: Fix bug in key.Merge operation with extra bytes

.changelog/2698.internal.md

@@ -0,0 +1 @@
+go/storage/mkvs: Add MKVS fuzzing

go/Makefile

@@ -70,7 +70,11 @@ integrationrunner:
 	@$(GO) test $(GOFLAGS) -c -covermode=atomic -coverpkg=./... -o oasis-node/$@/$@.test ./oasis-node/$@
 
 # Fuzzing binaries.
-build-fuzz: consensus/tendermint/fuzz/fuzz-fuzz.zip storage/fuzz/fuzz-fuzz.zip
+fuzz-targets := consensus/tendermint/fuzz/fuzz-fuzz.zip \
+	storage/fuzz/fuzz-fuzz.zip \
+	storage/mkvs/urkel/fuzz/fuzz-fuzz.zip
+
+build-fuzz: $(fuzz-targets)
 %/fuzz-fuzz.zip: .FORCE
 	@echo "Building $@"
 	@cd "$$(dirname "$@")"; go-fuzz-build
@@ -94,6 +98,8 @@ fuzz-storage: storage/fuzz/ oasis-node/oasis-node
 	@chmod 0700 /tmp/oasis-node-fuzz-storage/identity
 	@oasis-node/oasis-node identity init --datadir /tmp/oasis-node-fuzz-storage/identity
 	$(canned-fuzz-run)
+fuzz-storage-mkvs: storage/mkvs/urkel/fuzz/
+	$(canned-fuzz-run)
 
 # Clean.
 clean:

go/storage/client/tests/tests.go

@@ -6,8 +6,6 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 
 	"github.com/oasislabs/oasis-core/go/common/crypto/hash"
 	"github.com/oasislabs/oasis-core/go/common/identity"
@@ -114,14 +112,15 @@ recvLoop:
 
 	// Try getting path.
 	// TimeOut is expected, as test nodes do not actually start storage worker.
+	ctx, cancel = context.WithTimeout(ctx, 1*time.Second)
+	defer cancel()
 	r, err = client.SyncGet(ctx, &api.GetRequest{
 		Tree: api.TreeID{
 			Root:     root,
 			Position: root.Hash,
 		},
 	})
 	require.Error(err, "storage client should error")
-	require.Equal(codes.Unavailable, status.Code(err), "storage client should timeout")
 	require.Nil(r, "result should be nil")
 
 	rt.Cleanup(t, consensus.Registry(), consensus)

go/storage/mkvs/urkel/fuzz/.gitignore

@@ -0,0 +1,5 @@
+corpus/
+crashers/
+suppressions/
+*.zip
+gencorpus/gencorpus

go/storage/mkvs/urkel/fuzz/fuzz.go

@@ -0,0 +1,203 @@
+// +build gofuzz
+
+package fuzz
+
+import (
+	"bytes"
+	"context"
+	"encoding/hex"
+	"encoding/json"
+	"fmt"
+	"sort"
+
+	commonFuzz "github.com/oasislabs/oasis-core/go/common/fuzz"
+	mkvs "github.com/oasislabs/oasis-core/go/storage/mkvs/urkel"
+	mkvsTests "github.com/oasislabs/oasis-core/go/storage/mkvs/urkel/tests"
+)
+
+var (
+	tree *TreeFuzz
+
+	fuzzer *commonFuzz.InterfaceFuzzer
+)
+
+// TreeFuzz is a wrapper around a mkvs.KeyValueTree for fuzzing purposes.
+type TreeFuzz struct {
+	inner     mkvs.KeyValueTree
+	reference map[string][]byte
+	history   mkvsTests.TestVector
+}
+
+func (t *TreeFuzz) Insert(ctx context.Context, key []byte, value []byte) int {
+	if len(key) == 0 {
+		// Ignore zero-length keys as they are invalid.
+		return -1
+	}
+
+	if err := t.inner.Insert(ctx, key, value); err != nil {
+		t.fail("Insert failed: %s", err)
+	}
+
+	// Make sure the key has been set.
+	if getValue, err := t.inner.Get(ctx, key); err != nil || !bytes.Equal(value, getValue) {
+		t.fail("Insert check failed: %s", err)
+	}
+
+	t.reference[string(key)] = value
+	t.history = append(t.history, &mkvsTests.Op{Op: mkvsTests.OpInsert, Key: key, Value: value})
+	return 0
+}
+
+func (t *TreeFuzz) Get(ctx context.Context, key []byte) int {
+	if len(key) == 0 {
+		// Ignore zero-length keys as they are invalid.
+		return -1
+	}
+
+	value, err := t.inner.Get(ctx, key)
+	if err != nil {
+		t.fail("Get failed: %s", err)
+	}
+
+	t.assertCorrectValue(key, value)
+
+	return 0
+}
+
+func (t *TreeFuzz) RemoveExisting(ctx context.Context, key []byte) int {
+	if len(key) == 0 {
+		// Ignore zero-length keys as they are invalid.
+		return -1
+	}
+
+	value, err := t.inner.RemoveExisting(ctx, key)
+	if err != nil {
+		t.fail("RemoveExisting failed: %s", err)
+	}
+
+	// Make sure the key has been removed.
+	if value, err := t.inner.Get(ctx, key); err != nil || value != nil {
+		t.fail("RemoveExisting check failed: %s", err)
+	}
+
+	t.assertCorrectValue(key, value)
+
+	delete(t.reference, string(key))
+	t.history = append(t.history, &mkvsTests.Op{Op: mkvsTests.OpRemove, Key: key})
+
+	return 0
+}
+
+func (t *TreeFuzz) Remove(ctx context.Context, key []byte) int {
+	if len(key) == 0 {
+		// Ignore zero-length keys as they are invalid.
+		return -1
+	}
+
+	if err := t.inner.Remove(ctx, key); err != nil {
+		t.fail("Remove failed: %s", err)
+	}
+
+	// Make sure the key has been removed.
+	if value, err := t.inner.Get(ctx, key); err != nil || value != nil {
+		t.fail("Remove check failed: %s", err)
+	}
+
+	delete(t.reference, string(key))
+	t.history = append(t.history, &mkvsTests.Op{Op: mkvsTests.OpRemove, Key: key})
+
+	return 0
+}
+
+func (t *TreeFuzz) IteratorSeek(ctx context.Context, key []byte) int {
+	it := t.inner.NewIterator(ctx)
+	defer it.Close()
+
+	it.Seek(key)
+	if it.Err() != nil {
+		t.fail("IteratorSeek failed: %s", it.Err())
+	}
+
+	// Check that the iterator is in the correct position.
+	var ordered []string
+	for k := range t.reference {
+		ordered = append(ordered, k)
+	}
+	sort.Strings(ordered)
+
+	var expectedKey, expectedValue []byte
+	for _, k := range ordered {
+		if k >= string(key) {
+			expectedKey = []byte(k)
+			expectedValue = t.reference[k]
+			break
+		}
+	}
+	if !bytes.Equal(expectedKey, it.Key()) || !bytes.Equal(expectedValue, it.Value()) {
+		// Add the final IteratorSeek operation.
+		t.history = append(t.history, &mkvsTests.Op{
+			Op:          mkvsTests.OpIteratorSeek,
+			Key:         key,
+			Value:       expectedValue,
+			ExpectedKey: expectedKey,
+		})
+
+		t.fail("iterator Seek returned incorrect key/value (expected: %s/%s got: %s/%s)",
+			hex.EncodeToString(expectedKey),
+			hex.EncodeToString(expectedValue),
+			hex.EncodeToString(it.Key()),
+			hex.EncodeToString(it.Value()),
+		)
+	}
+
+	return 0
+}
+
+func (t *TreeFuzz) assertCorrectValue(key, value []byte) {
+	if refValue := t.reference[string(key)]; !bytes.Equal(value, refValue) {
+		// Add the final Get operation.
+		t.history = append(t.history, &mkvsTests.Op{Op: mkvsTests.OpGet, Key: key, Value: refValue})
+
+		t.fail("Get returned incorrect value for key %s (expected: %s got: %s)",
+			hex.EncodeToString(key),
+			hex.EncodeToString(refValue),
+			hex.EncodeToString(value),
+		)
+	}
+}
+
+func (t *TreeFuzz) fail(format string, a ...interface{}) {
+	// In case there is a failure, dump the operation history so it can be used to create a test
+	// vector for unit tests.
+	fmt.Printf("--- FAILURE: Dumping operation history ---\n")
+	history, _ := json.MarshalIndent(t.history, "", "    ")
+	fmt.Printf("%s\n", history)
+	fmt.Printf("------------------------------------------\n")
+
+	panic(fmt.Sprintf(format, a...))
+}
+
+func NewTreeFuzz() *TreeFuzz {
+	return &TreeFuzz{
+		inner:     mkvs.New(nil, nil, mkvs.WithoutWriteLog()),
+		reference: make(map[string][]byte),
+	}
+}
+
+func init() {
+	// Create the in-memory tree.
+	tree = NewTreeFuzz()
+
+	// Create and prepare the fuzzer.
+	fuzzer = commonFuzz.NewInterfaceFuzzer(tree)
+}
+
+func Fuzz(data []byte) int {
+	values, result := fuzzer.DispatchBlob(data)
+	if !result {
+		return -1
+	}
+
+	// Return value is another result.
+	return values[0].Interface().(int)
+}

go/storage/mkvs/urkel/fuzz/gencorpus/main.go

@@ -0,0 +1,30 @@
+// +build gofuzz
+
+// Gencorpus implements a simple utility to generate corpus files for the fuzzer.
+// It has no command-line options and creates the files in the current working directory.
+package main
+
+import (
+	"fmt"
+	"io/ioutil"
+
+	commonFuzz "github.com/oasislabs/oasis-core/go/common/fuzz"
+	mkvsFuzz "github.com/oasislabs/oasis-core/go/storage/mkvs/urkel/fuzz"
+)
+
+const (
+	samplesPerMethod int = 20
+)
+
+func main() {
+	tree := mkvsFuzz.NewTreeFuzz()
+	fuzzer := commonFuzz.NewInterfaceFuzzer(tree)
+
+	for i := 0; i < samplesPerMethod; i++ {
+		blobs := fuzzer.MakeSampleBlobs()
+		for meth := 0; meth < len(blobs); meth++ {
+			fileName := fmt.Sprintf("%s_%02d.bin", fuzzer.Method(meth).Name, i)
+			_ = ioutil.WriteFile(fileName, blobs[meth], 0644)
+		}
+	}
+}

go/storage/mkvs/urkel/insert.go

@@ -10,6 +10,10 @@ import (
 
 // Implements Tree.
 func (t *tree) Insert(ctx context.Context, key []byte, value []byte) error {
+	if value == nil {
+		value = []byte{}
+	}
+
 	t.cache.Lock()
 	defer t.cache.Unlock()
 

go/storage/mkvs/urkel/node/key.go

@@ -137,19 +137,24 @@ func (k Key) Split(splitPoint Depth, keyLen Depth) (prefix Key, suffix Key) {
 // another key in bits.
 // This function is immutable and returns a new instance of Key.
 func (k Key) Merge(keyLen Depth, k2 Key, k2Len Depth) Key {
+	keyLenBytes := int(keyLen) / 8
+	if keyLen%8 != 0 {
+		keyLenBytes++
+	}
+
 	newKey := make(Key, (keyLen + k2Len).ToBytes())
-	copy(newKey[:], k[:])
+	copy(newKey[:], k[:keyLenBytes])
 
 	for i := 0; i < len(k2); i++ {
 		// First set the right chunk of the previous byte
-		if keyLen%8 != 0 && len(k) > 0 {
-			newKey[len(k)+i-1] |= k2[i] >> (keyLen % 8)
+		if keyLen%8 != 0 && keyLenBytes > 0 {
+			newKey[keyLenBytes+i-1] |= k2[i] >> (keyLen % 8)
 		}
 		// ...and the next left chunk, if we haven't reached the end of newKey
 		// yet.
-		if len(k)+i < len(newKey) {
+		if keyLenBytes+i < len(newKey) {
 			// another mod 8 to prevent bit shifting for 8 bits
-			newKey[len(k)+i] |= k2[i] << ((8 - keyLen%8) % 8)
+			newKey[keyLenBytes+i] |= k2[i] << ((8 - keyLen%8) % 8)
 		}
 	}
 

go/storage/mkvs/urkel/node/key_test.go

@@ -70,6 +70,16 @@ func TestKeyAppendSplitMerge(t *testing.T) {
 	newKey = p.Merge(21, s, 8)
 	// Merge doesn't obtain original key, because the split cleaned unused bits!
 	require.Equal(t, Key{0xff, 0xff, 0xff, 0xf8}, newKey)
+
+	// Special case with zero-length key.
+	key = Key{0x80}
+	newKey = key.Merge(0, Key{0xf0}, 4)
+	require.Equal(t, Key{0xf0}, newKey)
+
+	// Special case with extra bytes.
+	key = Key{0x41, 0x6b, 0x00}
+	newKey = key.Merge(16, Key{0x37}, 8)
+	require.Equal(t, Key{0x41, 0x6b, 0x37}, newKey)
 }
 
 func TestKeyCommonPrefixLen(t *testing.T) {

go/storage/mkvs/urkel/remove.go

@@ -72,7 +72,10 @@ func (t *tree) doRemove(
 
 		var changed bool
 		var existing []byte
-		if key.BitLength() == bitLength {
+		if key.BitLength() < bitLength {
+			// Lookup key is too short for the current n.Label, so it doesn't exist.
+			return ptr, false, nil, nil
+		} else if key.BitLength() == bitLength {
 			n.LeafNode, changed, existing, err = t.doRemove(ctx, n.LeafNode, bitLength, key, depth)
 		} else if key.GetBit(bitLength) {
 			n.Right, changed, existing, err = t.doRemove(ctx, n.Right, bitLength, key, depth+1)

go/storage/mkvs/urkel/testdata/case-1.json

@@ -0,0 +1,21 @@
+[
+    {
+        "op": "Insert",
+        "key": "y/XyJC9ZURWPCA==",
+        "value": "gg=="
+    },
+    {
+        "op": "Insert",
+        "key": "y/XyJA==",
+        "value": "Wc0="
+    },
+    {
+        "op": "Remove",
+        "key": "y/Xy"
+    },
+    {
+        "op": "Get",
+        "key": "y/XyJA==",
+        "value": "Wc0="
+    }
+]