attic-labs / noms

The versioned, forkable, syncable database
Apache License 2.0
7.45k stars 267 forks source link

Encoding an empty types.Value has surprising and dangerous result #3850

Open aboodman opened 4 years ago

aboodman commented 4 years ago

Repo:

package main

import (
    "fmt"

    "github.com/attic-labs/noms/go/chunks"
    //"github.com/attic-labs/noms/go/marshal"
    "github.com/attic-labs/noms/go/types"
)

type Foo struct {
    Bar types.Ref
    Baz types.String
}

func main() {
    ts := &chunks.TestStorage{}
    vs := types.NewValueStore(ts.NewView())

    // This is how I discovered this bug - very easy mistake to make
    // note missing `noms:",omitempty"` on `Bar` field.
    //
    // nf := marshal.MustMarshal(vs, Foo{Baz:"monkey"}).(types.Struct)
    // fmt.Println(nf.Get("bar"))
    //
    // ... but you can also get the bug directly via the types API:

    foo := types.NewStruct("", types.StructData{
        "bar": types.Ref{},
    })
    c := types.EncodeValue(foo)
    v := types.DecodeValue(c, vs).(types.Struct)
    fmt.Println(v.Get("bar"))
}

yields:

panic: runtime error: index out of range

... deep inside serialization. The issue is that in this case valueImpl contains a completely empty buffer, which is what ends up being serialized, which completely confuses the deserializer.

I'm not sure if the right thing is to panic inside valueImpl serialization if we're uninitialized? That would violate the Go philosophy of making zero values useful. Or if we can instead serialize an empty value instead... in that case it seems like for consistency we should make sure that all the zero types.Values are useful.