proto, encoding/prototext, encoding/protojson: document that users should pass non-nil pointers to Unmarshal

[johanbrandhorst]

What version of protobuf and what language are you using?
golang/protobuf/jsonpb@1.3.2.

What did you do?
A bug report was raised against the grpc-gateway project whereby a certain type of invalid JSON input could sometimes cause a panic in the JSON unmarshalling code, as shown by this golang playground example:

https://play.golang.org/p/2VS9bEb7sgD

What did you expect to see?

I expected an error to be returned.

What did you see instead?

A panic with the following stack trace:

panic: reflect: call of reflect.Value.Type on zero Value

goroutine 1 [running]:
reflect.Value.Type(0x0, 0x0, 0x0, 0x0, 0xeeda5a00, 0x0)
	/usr/local/go/src/reflect/value.go:1813 +0x1c0
github.com/golang/protobuf/jsonpb.(*Unmarshaler).unmarshalValue(0x40a0e0, 0x0, 0x0, 0x0, 0x414030, 0x3, 0x8, 0x0, 0xd0, 0x1dfa00)
	/tmp/gopath227256185/pkg/mod/github.com/golang/protobuf@v1.3.2/jsonpb/jsonpb.go:720 +0x60
github.com/golang/protobuf/jsonpb.(*Unmarshaler).UnmarshalNext(0x40a0e0, 0x45c000, 0x232470, 0x0, 0x1e2360, 0x45a000)
	/tmp/gopath227256185/pkg/mod/github.com/golang/protobuf@v1.3.2/jsonpb/jsonpb.go:682 +0x1c0
github.com/golang/protobuf/jsonpb.(*Unmarshaler).Unmarshal(...)
	/tmp/gopath227256185/pkg/mod/github.com/golang/protobuf@v1.3.2/jsonpb/jsonpb.go:693
github.com/golang/protobuf/jsonpb.UnmarshalString(0x1e939f, 0x3, 0x232470, 0x0, 0x432070, 0xbf)
	/tmp/gopath227256185/pkg/mod/github.com/golang/protobuf@v1.3.2/jsonpb/jsonpb.go:714 +0x120
main.main()
	/tmp/sandbox283234818/prog.go:12 +0x40

Anything else we should know about your project / environment?

Interestingly, the same panic does not happen when using a JSON string instead of a JSON number:

https://play.golang.org/p/kxOjuAuf4HM

It also doesn't happen when using a reference to a concrete value:

https://play.golang.org/p/kIAksR51kaz

I would argue that because case #1 above works fine, this is a bug in the unmarshalling code.

[johanbrandhorst]

This could be considered a DOS vulnerability for any grpc-gateway servers using timestamps in their public API.

[dsnet]

I'm sympathetic to this issue, but I'm hesitant to address it because of the performance overhead it introduces. The "fix" implies that the start of Unmarshal has some code that looks like:

v := reflect.ValueOf(m)
if v.IsValid() && v.Kind() == reflect.Ptr && v.IsNil() {
    return fmt.Errorf("Unmarshal(nil %T)", m)
}

While it isn't particularly onerous, it is a non-trivial amount of overhead. If we're going to add this check to jsonpb.Unmarshal, we'll want to also add it to the proto.Unmarshal for consistency. However, wire unmarshaling is a function that is called in hot loops where this check actually is a significant performance cost.

[johanbrandhorst]

Ok, is it generally advised that we do not use nil pointers as arguments to Unmarshal, then? We can probably implement a fix for this in the gateway runtime.

[dsnet]

I can't think of any reasonable reason to pass a typed nil pointer to Unmarshal.

[johanbrandhorst]

Fair enough, I will close this. What do you think about adding something to the documentation to discourage this use?

[dsnet]

What do you think about adding something to the documentation to discourage this use?

We really should. Thanks for the suggestion.

[neild]

If we want to explicitly document that Unmarshal needs a non-nil pointer, then we should do so consistently across all unmarshal functions.

[dsnet]

Fixed with https://golang.org/cl/302329