decoder: Fix crashes found by fuzzer (kisielk#32)

* decoder: Don't forget to check memo for key not there on read access

If we do not do reading from memo will return memo's zero value (= nil),
which is

a) not correct - many memo keys could be read this way, and
b) nil there on stack breaks invariants of stack containing only good
   values.

Furthermore, it can even lead to crashes on e.g. calling
reflect.TypeOf(stack.top()) in the next patch.

Anyway getting something from memo must be checked for whether it there
or not for correctness.

Noticed while working on fix for kisielk#30.

* decoder: Fix "panic: runtime error: hash of unhashable type ..."

Go specification requires that only comparable types could be used as
map keys:

    https://golang.org/ref/spec#Map_types

For map[interface{}]... this is checked at runtime, and if concrete
value used as a key is not comparable it results in runtime panic, e.g.:

    panic: runtime error: hash of unhashable type ogórek.Call

    goroutine 1 [running]:
    github.com/kisielk/og-rek.(*Decoder).loadDict(0xc420084360, 0x64, 0x0)
            /tmp/go-fuzz-build561441550/gopath/src/github.com/kisielk/og-rek/ogorek.go:655 +0x18c
    github.com/kisielk/og-rek.Decoder.Decode(0xc42001c3c0, 0x5a9300, 0x0, 0x0, 0xc4200164b0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
            /tmp/go-fuzz-build561441550/gopath/src/github.com/kisielk/og-rek/ogorek.go:187 +0x172b
    github.com/kisielk/og-rek.Fuzz(0x7ff901592000, 0xa, 0x200000, 0x3)
            /tmp/go-fuzz-build561441550/gopath/src/github.com/kisielk/og-rek/fuzz.go:12 +0x108
    go-fuzz-dep.Main(0x50d830)
            /tmp/go-fuzz-build561441550/goroot/src/go-fuzz-dep/main.go:49 +0xd9
    main.main()
            /tmp/go-fuzz-build561441550/gopath/src/github.com/kisielk/og-rek/go.fuzz.main/main.go:10 +0x2d
    exit status 2

so when decoding dict and friends - all places where maps are populated - we
have to check whether an object on stack we are going to use as key is suitable.

Issue kisielk#30 contains comparison of 2 ways to do such check - catch
runtime panic in exception style manner or use
reflect.TypeOf(v).Comparable(). Since reflect-way turns out to be faster

kisielk#30 (comment)

and likely will become more faster in the future:

golang/go#19361

it was decided to go the reflect way (which is also a canonical way in
go land).

So audit all places where map items are set and add appropriate checks
before them.

I've verified that if we remove any of the added checks, via so far found
crash vectors, at least one crash case will reappear in tests. This
means that all added checks are actually test covered.

Updates: kisielk#30

* decoder: Don't forget to check for odd #elements in loadDict & friends

e.g. for Dict opcode the expected stack state is

	MARK key1 obj1 key2 obj2 ... keyN objN DICT

so if in between MARK and DICT there is odd number of elements this is
an error in input data. We also used to crash on such cases, e.g.:

        "(\x88d"

        panic: runtime error: index out of range

        goroutine 1 [running]:
        github.com/kisielk/og-rek.(*Decoder).loadDict(0xc420082990, 0xc42000e464, 0x0)
                /tmp/go-fuzz-build403415384/gopath/src/github.com/kisielk/og-rek/ogorek.go:652 +0x21d
        github.com/kisielk/og-rek.Decoder.Decode(0xc42001c7e0, 0x5a9320, 0x0, 0x0, 0xc4200167b0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
                /tmp/go-fuzz-build403415384/gopath/src/github.com/kisielk/og-rek/ogorek.go:188 +0x172d
        github.com/kisielk/og-rek.Fuzz(0x7f6d1f310000, 0x3, 0x200000, 0x3)
                /tmp/go-fuzz-build403415384/gopath/src/github.com/kisielk/og-rek/fuzz.go:12 +0x108
        go-fuzz-dep.Main(0x50d798)
                /tmp/go-fuzz-build403415384/goroot/src/go-fuzz-dep/main.go:49 +0xd9
        main.main()
                /tmp/go-fuzz-build403415384/gopath/src/github.com/kisielk/og-rek/go.fuzz.main/main.go:10 +0x2d

I've audited whole decoder and regarding odd #(elements) there are only 2
places to care: loadDict and loadSetItems and crashers for all of them were
already found by fuzz testing.

Fixes kisielk#30 (for all known cases so far).

ogorek.go

@@ -12,6 +12,7 @@ import (
 	"io"
 	"math"
 	"math/big"
+	"reflect"
 	"strconv"
 )
 
@@ -651,8 +652,15 @@ func (d *Decoder) loadDict() error {
 
 	m := make(map[interface{}]interface{}, 0)
 	items := d.stack[k+1:]
+	if len(items) % 2 != 0 {
+		return fmt.Errorf("pickle: loadDict: odd # of elements")
+	}
 	for i := 0; i < len(items); i += 2 {
-		m[items[i]] = items[i+1]
+		key := items[i]
+		if !reflect.TypeOf(key).Comparable() {
+			return fmt.Errorf("pickle: loadDict: invalid key type %T", key)
+		}
+		m[key] = items[i+1]
 	}
 	d.stack = append(d.stack[:k], m)
 	return nil
@@ -692,7 +700,11 @@ func (d *Decoder) get() error {
 	if err != nil {
 		return err
 	}
-	d.push(d.memo[string(line)])
+	v, ok := d.memo[string(line)]
+	if !ok {
+		return fmt.Errorf("pickle: memo: key error %q", line)
+	}
+	d.push(v)
 	return nil
 }
 
@@ -702,7 +714,11 @@ func (d *Decoder) binGet() error {
 		return err
 	}
 
-	d.push(d.memo[strconv.Itoa(int(b))])
+	v, ok := d.memo[strconv.Itoa(int(b))]
+	if !ok {
+		return fmt.Errorf("pickle: memo: key error %d", b)
+	}
+	d.push(v)
 	return nil
 }
 
@@ -717,7 +733,11 @@ func (d *Decoder) longBinGet() error {
 		return err
 	}
 	v := binary.LittleEndian.Uint32(b[:])
-	d.push(d.memo[strconv.Itoa(int(v))])
+	vv, ok := d.memo[strconv.Itoa(int(v))]
+	if !ok {
+		return fmt.Errorf("pickle: memo: key error %d", v)
+	}
+	d.push(vv)
 	return nil
 }
 
@@ -828,9 +848,11 @@ func (d *Decoder) loadSetItem() error {
 	v := d.xpop()
 	k := d.xpop()
 	m := d.stack[len(d.stack)-1]
-	switch m.(type) {
+	switch m := m.(type) {
 	case map[interface{}]interface{}:
-		m := m.(map[interface{}]interface{})
+		if !reflect.TypeOf(k).Comparable() {
+			return fmt.Errorf("pickle: loadSetItem: invalid key type %T", k)
+		}
 		m[k] = v
 	default:
 		return fmt.Errorf("pickle: loadSetItem: expected a map, got %T", m)
@@ -850,7 +872,14 @@ func (d *Decoder) loadSetItems() error {
 	l := d.stack[k-1]
 	switch m := l.(type) {
 	case map[interface{}]interface{}:
+		if (len(d.stack) - (k + 1)) % 2 != 0 {
+			return fmt.Errorf("pickle: loadSetItems: odd # of elements")
+		}
 		for i := k + 1; i < len(d.stack); i += 2 {
+			key := d.stack[i]
+			if !reflect.TypeOf(key).Comparable() {
+				return fmt.Errorf("pickle: loadSetItems: invalid key type %T", key)
+			}
 			m[d.stack[i]] = d.stack[i+1]
 		}
 		d.stack = append(d.stack[:k-1], m)

ogorek_test.go

@@ -256,10 +256,37 @@ func TestMemoOpCode(t *testing.T) {
 
 }
 
+// verify that decode of erroneous input produces error
+func TestDecodeError(t *testing.T) {
+	testv := []string{
+		// all kinds of opcodes to read memo but key is not there
+		"}g1\n.",
+		"}h\x01.",
+		"}j\x01\x02\x03\x04.",
+	}
+	for _, tt := range testv {
+		buf := bytes.NewBufferString(tt)
+		dec := NewDecoder(buf)
+		v, err := dec.Decode()
+		if !(v == nil && err != nil) {
+			t.Errorf("%q: no decode error  ; got %#v, %#v", tt, v, err)
+		}
+	}
+}
+
 func TestFuzzCrashers(t *testing.T) {
 	crashers := []string{
 		"(dS''\n(lc\n\na2a2a22aasS''\na",
 		"S\n",
+		"((dd",
+		"}}}s",
+		"(((ld",
+		"(dS''\n(lp4\nsg4\n(s",
+		"}((tu",
+		"}((du",
+		"(c\n\nc\n\n\x85Rd",
+		"}(U\x040000u",
+		"(\x88d",
 	}
 
 	for _, c := range crashers {