• Kirill Smelkov's avatar
    decoder: Fix crashes found by fuzzer (#32) · da5f0342
    Kirill Smelkov authored
    * 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 #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 #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
    
    https://github.com/kisielk/og-rek/issues/30#issuecomment-283609067
    
    and likely will become more faster in the future:
    
    https://github.com/golang/go/issues/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: #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 #30 (for all known cases so far).
    da5f0342
ogorek_test.go 16 KB