[protoreflect] Message.WhichOneof for generated message type panics when given synthetic oneof #1659

jhump · 2024-12-18T01:02:58Z

What version of protobuf and what language are you using?
v1.36.0

What did you do?
Here's a reproducer test case.
protocolbuffers/protobuf-go@master...jhump:protobuf-go:jh/panic-repro
The panic happens in the call to msg.WhichOneof (line 20).

What did you expect to see?
In prior versions, at least as recently as 1.34.0, this worked as expected. It would return nil if the underlying proto3 optional field was present and otherwise it would return its field descriptor.

What did you see instead?

panic: invalid oneof descriptor goproto.proto.test3.TestAllTypes._optional_int32 for message goproto.proto.test3.TestAllTypes [recovered]
	panic: invalid oneof descriptor goproto.proto.test3.TestAllTypes._optional_int32 for message goproto.proto.test3.TestAllTypes

goroutine 21 [running]:
testing.tRunner.func1.2({0x10096da60, 0x1400008f2a0})
	/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.darwin-arm64/src/testing/testing.go:1632 +0x1bc
testing.tRunner.func1()
	/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.darwin-arm64/src/testing/testing.go:1635 +0x334
panic({0x10096da60?, 0x1400008f2a0?})
	/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.darwin-arm64/src/runtime/panic.go:785 +0x124
google.golang.org/protobuf/internal/impl.(*messageState).WhichOneof(0x140000fc908, {0x1009cb318, 0x1400014a098})
	/Users/jhumphries/src/protobuf-go/internal/impl/message_reflect_gen.go:126 +0x194
google.golang.org/protobuf/reflect/protoreflect_test.TestSyntheticOneof(0x140000ac9c0)
	/Users/jhumphries/src/protobuf-go/reflect/protoreflect/oneof_test.go:20 +0x108
testing.tRunner(0x140000ac9c0, 0x1009c5868)
	/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.darwin-arm64/src/testing/testing.go:1690 +0xe4
created by testing.(*T).Run in goroutine 1
	/Users/jhumphries/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.23.4.darwin-arm64/src/testing/testing.go:1743 +0x314

Anything else we should know about your project / environment?
Occurs on both OS X and Linux.

The following patch fixes the issue:

diff --git a/internal/impl/message_reflect_gen.go b/internal/impl/message_reflect_gen.go
index 99dc23c6..f12524be 100644
--- a/internal/impl/message_reflect_gen.go
+++ b/internal/impl/message_reflect_gen.go
@@ -123,6 +123,17 @@ func (m *messageState) WhichOneof(od protoreflect.OneofDescriptor) protoreflect.
        if oi := mi.oneofs[od.Name()]; oi != nil && oi.oneofDesc == od {
                return od.Fields().ByNumber(oi.which(m.pointer()))
        }
+       if od.Parent() == m.Descriptor() && od.IsSynthetic() {
+               // Synthetic oneofs are absent from mi.oneofs, so fallback to slow way.
+               fields := od.Fields()
+               for i, length := 0, fields.Len(); i < length; i++ {
+                       fd := fields.Get(i)
+                       if m.Has(fd) {
+                               return fd
+                       }
+               }
+               return nil
+       }
        panic("invalid oneof descriptor " + string(od.FullName()) + " for message " + string(m.Descriptor().FullName()))
 }
 func (m *messageState) GetUnknown() protoreflect.RawFields {

However, the file in question (internal/impl/message_reflect_gen.go) is generated. So a real fix will have to be made in the code generator or perhaps to reverse whatever change caused synthetic oneofs to no longer appear in mi.oneofs.

The text was updated successfully, but these errors were encountered:

bwester · 2024-12-18T07:01:23Z

I also just encountered this panic in v1.36.0 through a different route. Attempting to encode a message using github.com/golang/protobuf/jsonpb when that message's descriptor contains a synthetic oneof will trigger the panic at message_reflect_gen.go:126. I bisected google.golang.org/protobuf to find that this behavior was introduced with commit bdcc7adc94f2.

I am aware jsonpb has been deprecated a long time. It is unfortunately still in use in some code I work with. Also, I was thankfully unable to get protojson to show the same error.

rbroggi · 2024-12-18T13:25:32Z

version v1.35.2 still did not have this issue. Breaking change was introduced in v1.36.0

puellanivis · 2024-12-19T06:41:55Z

Current behavior is documented to be correct:

  		// Synthetic oneofs are used to represent optional fields in
  		// proto3. While they show up in protoreflect, WhichOneof does
  		// not work on these (only on non-synthetic, explicit oneofs).

That is: calling WhichOneOf for an optional field should not work exactly like call calling WhichOneOf for a non-optional simple scalar outside of a oneof.

rbroggi · 2024-12-19T07:03:02Z

Regardless of the documentation correctness, this seems to be a breaking change - I've experienced many services breaking due to this and it breaks in a vey bad way since it's something that is frequently not tested - production servers panicking. Maybe it has the extremes for a security vulnerability?

puellanivis · 2024-12-19T07:19:53Z

I believe this is not the Security exception to go1compat, but Bugs according to the go1compat rules. This behavior never should have worked, because optional fields were never actually oneofs. Therefore, that this worked before was out of specification.

rbroggi · 2024-12-19T08:32:18Z

I am quite unknowledgeable on the rules, it still feel very that is a drastic behavioral change, if this wasn't supposed to work before I would have expected compilation issues, right now we have a situation where attackers can crash a server by just tuning some query parameters which is a great recipe for denial of service attacks. Maybe it shouldn't have been there from the beginning but it is already out and there are people running servers out there leveraging this behavior. If we want to pursue this correction my take is that it should come with a more diligent governance starting from the compiler, going through (if possible) deprecation warnings and only at the very end panicking.

rbroggi · 2024-12-19T09:48:59Z

In my case I am experiencing that when using in conjunction with grpc-gateway:

rpc Echo(MessageRequest) returns (MessageResponse) {
  option (google.api.http) = {
    get: "/v1/example/resource"
  };
}

message MessageRequest {
  int32 query_filter = 1;
  optional string optional_query_filter = 2;

}

jhump · 2024-12-19T14:31:12Z

Possibly relevant observation: *dynamicpb.Message works just fine when its WhichOneof method is called with a synthetic oneof.

puellanivis · 2024-12-19T19:17:44Z

Relevant grpc-gateway PR grpc-ecosystem/grpc-gateway#5072

neild · 2024-12-19T19:24:29Z

If this changed, we should probably restore the original behavior regardless of what the documentation says.

stapelberg · 2024-12-21T10:03:32Z

Sorry for the breakage, folks. Maybe I misunderstood something in how this behavior is meant to work. I’ll look into it on Monday when I’m in the office.

This reverts change https://go.dev/cl/632735, in which I misunderstood what the Protobuf documentation wanted to convey: The quoted docs in CL 632735 refer to reflection for proto3 optional fields, not to reflection for proto3 synthetic oneofs. Synthetic oneofs should remain visible through reflection. For the Open API, this change restores the old behavior. For the Opaque API, another fix is needed and will be sent in a separate, follow-up CL (follow golang/protobuf#1659). For golang/protobuf#1659 Thanks to Joshua Humphries for the report and reproducer! Change-Id: I3a924eed6d2425581adc65651395a68fc1576f4d Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/638495 Reviewed-by: Chressie Himpel <chressie@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

This is the equivalent of CL 638495, but for the Opaque API. While the behavior is the same for non-synthetic oneofs between the Open Struct API and Opaque API, the behavior for synthetic oneofs is not the same: Because the Opaque API uses a bitfield to store presence, we need to use the Opaque presence check in WhichOneof(). This CL extends the testproto generator to use the protoc command-line flag for the test3 testproto (which specifies syntax = "proto3";), as the in-file api_level is only available in .proto files using editions (but editions does not use synthetic oneofs for optional fields, only proto3 does). For golang/protobuf#1659 Change-Id: I0a1fd6e5fc6f96eeab043f966728ce2a14dbd446 Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/638515 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Chressie Himpel <chressie@google.com>

stapelberg · 2024-12-23T13:42:08Z

OK, I have had a chance to look into this issue some more.

As I suspected, I had not correctly understood the situation when I made this change. I’m sorry for the extra work this caused everyone.

By now, we have made two changes:

We released version v1.36.1 which contains a fix that restores the old behavior for the Open Struct API. If you ran into this issue when updating Go Protobuf and you are not yet using the Opaque API, upgrading to v1.36.1 should be sufficient to pick up the fix.
I submitted another fix that unifies behavior of the Opaque API with the other API levels when using optional fields in proto3 files. This change is not included in the v1.36.1 release yet to not cause any unnecessary risk over the holiday period. Please upgrade to version v1.36.2-0.20241223133329-8878926ea790 or newer to pick up this fix.

We will release v1.36.2 in January and will leave this issue open until then for visibility. Happy holidays everyone!

weeco mentioned this issue Dec 18, 2024

Update backend deps redpanda-data/console#1563

Merged

stapelberg self-assigned this Dec 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[protoreflect] Message.WhichOneof for generated message type panics when given synthetic oneof #1659

[protoreflect] Message.WhichOneof for generated message type panics when given synthetic oneof #1659

jhump commented Dec 18, 2024 •

edited

Loading

bwester commented Dec 18, 2024

rbroggi commented Dec 18, 2024

puellanivis commented Dec 19, 2024

rbroggi commented Dec 19, 2024 •

edited

Loading

puellanivis commented Dec 19, 2024

rbroggi commented Dec 19, 2024 •

edited

Loading

rbroggi commented Dec 19, 2024

jhump commented Dec 19, 2024

puellanivis commented Dec 19, 2024

neild commented Dec 19, 2024

stapelberg commented Dec 21, 2024

stapelberg commented Dec 23, 2024

[protoreflect] Message.WhichOneof for generated message type panics when given synthetic oneof #1659

[protoreflect] Message.WhichOneof for generated message type panics when given synthetic oneof #1659

Comments

jhump commented Dec 18, 2024 • edited Loading

bwester commented Dec 18, 2024

rbroggi commented Dec 18, 2024

puellanivis commented Dec 19, 2024

rbroggi commented Dec 19, 2024 • edited Loading

puellanivis commented Dec 19, 2024

rbroggi commented Dec 19, 2024 • edited Loading

rbroggi commented Dec 19, 2024

jhump commented Dec 19, 2024

puellanivis commented Dec 19, 2024

neild commented Dec 19, 2024

stapelberg commented Dec 21, 2024

stapelberg commented Dec 23, 2024

jhump commented Dec 18, 2024 •

edited

Loading

rbroggi commented Dec 19, 2024 •

edited

Loading

rbroggi commented Dec 19, 2024 •

edited

Loading