protoc-gen-go: oneof getters panic on typed-nil values.

[anzboi]

What version of protobuf and what language are you using? Golang

google.golang.org/protobuf v1.26.0

$ protoc --version
libprotoc 3.17.3

What did you do? Panic occurs when you set a typed nil on a oneof field

// test.proto
message Test {
    oneof myOneof {
        string str = 1;
    }
}

// main.go
var typedNil *pb.Test_myOneof
t := &pb.Test{MyOneof: typedNil}
fmt.Println(t.GetStr())

What did you expect to see? Empty string

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference

Possibly related: #478

Anything else we should know about your project / environment?

I tracked down the source of the panic. The generated getter for the oneof field does a nil check on the interface value, but not the Str value after casting it. Following code was generated with the above tool versions.

func (x *Test) GetStr() string {
    if x, ok := x.GetMyOneof().(*Test_Str); ok {
        return x.Str
    }
    return ""
}

In the typed-nil case, the cast works and reports true, so the code attempts to access x.Str, but x is in fact nil. There should be an additional nil check on x.

I imagine this case is probably not intended to come about in practice, but it is certainly possible.

[neild]

This, so far as I know, is working as intended; (*pb.Test_myOneof)(nil) is not a valid value for the MyOneof field, and setting the field to that value is an error.

[anzboi]

I guess that makes sense, if the API consider typed-nil in a oneof field as an error. I don't think its easy to discover this fact though. The generated code developer reference section for oneof fields doesn't mention it at all. In fact the wording could reasonably be interpreted that any nil value would return a zero value on the getters.

Each get function returns the value for that field or the zero value if it is not set.

https://developers.google.com/protocol-buffers/docs/reference/go-generated#oneof

[puellanivis]

Hm. Coming from the last common on the linked possibly relevant issue:

Nil pointers to wrapper types for oneof field: Protobuf fields within a oneof have presence. The generated Go represents oneofs as an interface, where a given field is populated by allocating a wrapper struct with the field value set (e.g., m.Oneof = &foopb.Message_Field{"hello"}). The Go API unfortunately allows for two-levels of presence: 1) for whether the interface itself is nil, and 2) for whether the field wrapper struct itself is nil. Since the second layer cannot be represented in the protobuf data model, we treat a nil wrapper type (e.g., (*foopb.Message_Field)(nil)) as being identical to the entire oneof being unpopulated. When unmarshaling or merging, a nil interface value is used to represent an unpopulated oneof.

It seems we should be doing the additional nil check as requested, in order to ensure “consistent handling of nil values.“

[jhump]

FWIW, reflection works with this situation just fine and treats the typed-nil as if it were a nil interface -- the oneof is treated as if unset: https://go.dev/play/p/fFn0Dj6cjR7. The msg.ProtoReflect().WhichOneof call returns nil and the msg.ProtoReflect().Get call does not panic.

So, for consistency, updating the generated accessors to behave like msg.ProtoReflect().Get seems like the right choice.

Also, interestingly, if just using reflection, it is impossible to detect if a message is incorrectly constructed this way. In order to generically check for such malformed messages, it would be nice to remedy this, such as an IsOneofValid(protoreflect.OneofDescriptor) bool method on protoreflect.Message.