-
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