dvyukov / go-fuzz

Randomized testing for Go
Apache License 2.0
4.78k stars 279 forks source link

go-fuzz-dep: sonar serialization contains faulty type assumption #341

Open josharian opened 2 years ago

josharian commented 2 years ago

While fuzzing something, I got this crash:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x8 pc=0x10210d414]

goroutine 1 [running]:
go-fuzz-dep.serialize({0x0, 0x0}, {0x102205300?, 0x140000aa900?}, {0x1400011db60?, 0xbdbfefbfbdef2f20?, 0x1400011db78?})
    go-fuzz-dep/sonar.go:150 +0xd94
go-fuzz-dep.Sonar({0x0, 0x0}, {0x102205300, 0x140000aa900}, 0x7c700)
    go-fuzz-dep/sonar.go:31 +0x54
reflect.DeepEqual.func2(...)
    /Users/josh/go/1.18/src/reflect/deepequal.go:230
reflect.DeepEqual({0x0, 0x0}, {0x102205300?, 0x140000aa900})
    /Users/josh/go/1.18/src/reflect/deepequal.go:230 +0xcc
[...]

The relevant reflect.DeepEqual code is:

func DeepEqual(x, y any) bool {
    if x == nil || y == nil {
        return x == y  // <------
    }

The relevant sonar.go line is:

func serialize(v, v2 interface{}, buf []byte) (n, flags uint8) {
    switch vv := v.(type) {
        // omit many cases
    default:
        // Special case: string literal is compared with a variable of
        // user type with string underlying type:
        //  type Name string
        //  var name Name
        //  if name == "foo" { ... }
        if _, ok := v2.(string); ok {
            s := *(*string)((*iface)(unsafe.Pointer(&v)).val)  // <--------
            if len(s) <= SonarMaxLen {
                return uint8(copy(buf[:], s)), SonarString
            }
        }
        // ...

I think this code contains a faulty assumption. If v has an interface type (as with reflect.DeepEqual), it can be compared with a string without itself being a string. The iface conversion is thus unsafe. I believe in this case it is due to a nil interface being compared with a string, causing a nil pointer dereference when reading the iface.val field.

If anyone cares (unlikely), this should be made safer, probably by using reflect.Type.