proto: malformed oneof field values can make Marshal panic #478
Comments
- Make sure badly constructed oneof fields are handled properly. - Add an additional test case for a non-empty proto extension. - Add logic to make sure that the HashProto method recovers properly if the proto library panics (See golang/protobuf/issues/478 for an example). This case is included in the malformed oneof field tests as `&Singleton{Singleton: (*Singleton_TheString)(nil)}`.
|
The line It looks, like using an actual nil pointer struct for the Oneof is violating a contract of how protobuf oneof data structures will be composed. Of note, you construct the first as This said, I recall a comment to a recent pull request, which is to check for required fields being set, noting the assumptions being made (that the intermediate/wrapper struct will not be nil) should not be made, and one should check the oneof struct to ensure that it is not nil before proceeding to take it’s Type(). The central question, I suppose, is how these things should be composed. Is it reasonable to expect that the intermediate/wrapper oneof structs shall always be non-nil, as they will be composed from a As the best-practices in Go protobufs is to compose messages with But in general no one should be instantiating a pb.Message with a zero-value pointer (i.e. |
dsnet
changed the title
proto.Marshal panic
|
When dealing with interfaces in Go, there is this insane concept of typed nils. How I think of it is that an interface variable has a type and a value. The type being nil is independent from whether or not the value is nil. And it is very easy to accidentally type your nils by passing them around. See more about it here: https://golang.org/doc/faq#nil_error Here is an example of a marshaller that was generated by this library that can have the exception described above. SendEmailRequest may have a ReplyToOption that is one of ReplyTo or ReplyToAttributes. In this function, doing
Yeah, Go doesn't care. This is a type switch, and the type of the variable stored in the struct was not nil even though the value was nil. Here is an example of a Getter for one of the possibilities in a oneof that also can encounter nil pointer exceptions: Again, |
|
Nothing internally to the protobuf code would assign a typed nil to that interface anyways. Which is what the point of my discussion was about. Should we expect that someone might have put a typed nil into a field external to the protobuf code, or can we reasonably rely that one would not assign a typed nil into the oneof field? Where does the equilibrium lie between “you’ve doing something out of spec, and your code is wrong“ and “always validate inputs”? This is not a trivial question, and any answer to it should be non-trivial as well. Clearly misusing the API is “your code is wrong” but is managing to store a typed nil into this specific type of interface something that should be guarded against, so as to save developers, who are out of spec? 🤷♀️ |
|
@puellanivis I would normally agree, but the code is already checking for nil, it's just not doing it correctly. I think the original (contrived) example is misleading in that it is "obviously" not something you would do normally. In reality this is most likely caused by situations where you have a zero value variable that is conditionally assigned, like so: var str testdata.Oneof_F_String
if some_condition {
str = &testdata.Oneof_F_String{"something"}
}
msg := &testdata.Oneof{Union: str},In this case I think the API consumer would reasonably expect that to not panic. |
|
While I believe that typed nils in interface variables are insane, they are
a perfectly valid part of the Go language, and people use them for
legitimate purposes. They certainly aren’t “malformed” as the ticket
suggests.
If you provide people a struct with a public interface variable on it, and
you tell people they can set fields on the struct and pass it back you,
then typed nils are a part of the interface to interact with the library.
The solution is pretty simple as well. Anytime you cast an interface to a
struct pointer, you must check for nil before dereferencing the pointer. I
think the two places I pointed out are the only locations where this occurs
in the generated code for oneof.
…On Tue, Jun 19, 2018 at 7:54 PM Alec Thomas ***@***.***> wrote:
@puellanivis <https://github.com/puellanivis> I would normally agree, but
the code is already checking for nil, it's just not doing it correctly.
I think the original (contrived) example is misleading in that it is
"obviously" not something you would do normally. In reality this is most
likely caused by situations where you have a zero value variable that is
conditionally assigned, like so:
var str testdata.Oneof_F_String
if some_condition {
str = &testdata.Oneof_F_String{"something"}
}
msg := &testdata.Oneof{Union: str},
In this case I think the API consumer would reasonably expect that to not
panic.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#478 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACXP6SfIYLVkUgB1p2YKAd298UndH336ks5t-Y9HgaJpZM4RPLo5>
.
|
|
@alecthomas but wouldn’t this code be better written as: or even better (because it does not rely upon an unexported type (see below)): Now, I note that this will not work, because there is an unexported type used there. A different issue discusses how oneof union types should be exported. And its precisely in my opinion for this reason. The proper conditional store of this value should not be the concrete type, but the union. This is also to address @ryho ’s point though as well, this exported interface may be exported, but its type is not. The whole oneof implementation in Go is generally required to be complicated because Go does not have a matching idiom, and so unions have to be cobbled together from imperfect idioms. This has led to just more complicated code to account for things that developers never expected API users to try and do. 😕 While typed nils are a part of the language, this does not necessarily mean that they need to be handled by this code. There are many functions that panic on fundamentally bad or broken inputs, and this is not automatically a bad design. |
|
@alecthomas now that I look over your code, the |
proto.Marshaldefines anerrOneofHasNilerror which it returns when it encounters oneof fields that are invalid due to the presence of unexpected nil values.For an example of this working properly, see this test: https://github.com/golang/protobuf/blob/master/proto/all_test.go#L1356
As the test linked above illustrates, this works in some cases. However, here's a code snippet that illustrates a case where
proto.Marshalfails to detect the nil values properly and instead panics:When running this using the latest protobuf library, I get the following output:
The first two lines from the output show the behavior working properly for the first proto in the test, while the remaining lines show the bad case.
The text was updated successfully, but these errors were encountered: