reflect/protodesc: fix panic when working with dynamicpb
Improves on CL 642575 with a few additional checks to make
sure a panic doesn't occur even under unexpected conditions.
Fixes golang/protobuf#1671
Change-Id: I25bf1cfdf0b35b381cab603f9e06581b3b630d73
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/642975
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
diff --git a/reflect/protodesc/editions.go b/reflect/protodesc/editions.go
index f55b036..697a61b 100644
--- a/reflect/protodesc/editions.go
+++ b/reflect/protodesc/editions.go
@@ -11,6 +11,7 @@
"google.golang.org/protobuf/internal/editiondefaults"
"google.golang.org/protobuf/internal/filedesc"
+ "google.golang.org/protobuf/internal/genid"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/types/descriptorpb"
@@ -128,23 +129,39 @@
// We must not use proto.GetExtension(child, gofeaturespb.E_Go)
// because that only works for messages we generated, but not for
// dynamicpb messages. See golang/protobuf#1669.
+ //
+ // Further, we harden this code against adversarial inputs: a
+ // service which accepts descriptors from a possibly malicious
+ // source shouldn't crash.
goFeatures := child.ProtoReflect().Get(gofeaturespb.E_Go.TypeDescriptor())
if !goFeatures.IsValid() {
return parentFS
}
+ gf, ok := goFeatures.Interface().(protoreflect.Message)
+ if !ok {
+ return parentFS
+ }
// gf.Interface() could be *dynamicpb.Message or *gofeaturespb.GoFeatures.
- gf := goFeatures.Message()
fields := gf.Descriptor().Fields()
- if fd := fields.ByName("legacy_unmarshal_json_enum"); gf.Has(fd) {
+ if fd := fields.ByNumber(genid.GoFeatures_LegacyUnmarshalJsonEnum_field_number); fd != nil &&
+ !fd.IsList() &&
+ fd.Kind() == protoreflect.BoolKind &&
+ gf.Has(fd) {
parentFS.GenerateLegacyUnmarshalJSON = gf.Get(fd).Bool()
}
- if fd := fields.ByName("strip_enum_prefix"); gf.Has(fd) {
+ if fd := fields.ByNumber(genid.GoFeatures_StripEnumPrefix_field_number); fd != nil &&
+ !fd.IsList() &&
+ fd.Kind() == protoreflect.EnumKind &&
+ gf.Has(fd) {
parentFS.StripEnumPrefix = int(gf.Get(fd).Enum())
}
- if fd := fields.ByName("api_level"); gf.Has(fd) {
+ if fd := fields.ByNumber(genid.GoFeatures_ApiLevel_field_number); fd != nil &&
+ !fd.IsList() &&
+ fd.Kind() == protoreflect.EnumKind &&
+ gf.Has(fd) {
parentFS.APILevel = int(gf.Get(fd).Enum())
}
diff --git a/reflect/protodesc/editions_test.go b/reflect/protodesc/editions_test.go
index 91c5f38..9d9b769 100644
--- a/reflect/protodesc/editions_test.go
+++ b/reflect/protodesc/editions_test.go
@@ -7,41 +7,106 @@
import (
"testing"
+ "google.golang.org/protobuf/internal/genid"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
+ "google.golang.org/protobuf/reflect/protoregistry"
"google.golang.org/protobuf/types/descriptorpb"
"google.golang.org/protobuf/types/dynamicpb"
"google.golang.org/protobuf/types/gofeaturespb"
)
-func TestGoFeaturesDynamic(t *testing.T) {
+func TestGoFeatures_NotExpectedType(t *testing.T) {
md := (*gofeaturespb.GoFeatures)(nil).ProtoReflect().Descriptor()
gf := dynamicpb.NewMessage(md)
opaque := protoreflect.ValueOfEnum(gofeaturespb.GoFeatures_API_OPAQUE.Number())
- gf.Set(md.Fields().ByName("api_level"), opaque)
- featureSet := &descriptorpb.FeatureSet{}
+ gf.Set(md.Fields().ByNumber(genid.GoFeatures_ApiLevel_field_number), opaque)
dynamicExt := dynamicpb.NewExtensionType(gofeaturespb.E_Go.TypeDescriptor().Descriptor())
- proto.SetExtension(featureSet, dynamicExt, gf)
- fd := &descriptorpb.FileDescriptorProto{
- Name: proto.String("a.proto"),
- Dependency: []string{
- "google/protobuf/go_features.proto",
+ extFile, err := NewFile(&descriptorpb.FileDescriptorProto{
+ Name: proto.String("test.proto"),
+ Dependency: []string{"google/protobuf/descriptor.proto"},
+ Extension: []*descriptorpb.FieldDescriptorProto{
+ {
+ Name: proto.String("ext1002"),
+ Number: proto.Int32(int32(gofeaturespb.E_Go.TypeDescriptor().Number())),
+ Type: descriptorpb.FieldDescriptorProto_TYPE_STRING.Enum(),
+ Label: descriptorpb.FieldDescriptorProto_LABEL_OPTIONAL.Enum(),
+ Extendee: proto.String(".google.protobuf.FeatureSet"),
+ },
},
- Edition: descriptorpb.Edition_EDITION_2023.Enum(),
- Syntax: proto.String("editions"),
- Options: &descriptorpb.FileOptions{
- Features: featureSet,
- },
- }
- fds := &descriptorpb.FileDescriptorSet{
- File: []*descriptorpb.FileDescriptorProto{
- ToFileDescriptorProto(descriptorpb.File_google_protobuf_descriptor_proto),
- ToFileDescriptorProto(gofeaturespb.File_google_protobuf_go_features_proto),
- fd,
- },
- }
- if _, err := NewFiles(fds); err != nil {
+ }, protoregistry.GlobalFiles)
+ if err != nil {
t.Fatal(err)
}
+ nonMessageExt := dynamicpb.NewExtensionType(extFile.Extensions().Get(0))
+
+ extFdProto := ToFieldDescriptorProto(gofeaturespb.E_Go.TypeDescriptor().Descriptor())
+ extMsgProto := ToDescriptorProto(gofeaturespb.E_Go.TypeDescriptor().Descriptor().Message())
+ extMsgProto.Field = extMsgProto.Field[:1] // remove all but first field from the message
+ extFile, err = NewFile(&descriptorpb.FileDescriptorProto{
+ Name: proto.String("google/protobuf/go_features.proto"),
+ Package: proto.String("pb"),
+ Dependency: []string{"google/protobuf/descriptor.proto"},
+ Extension: []*descriptorpb.FieldDescriptorProto{extFdProto},
+ MessageType: []*descriptorpb.DescriptorProto{extMsgProto},
+ }, protoregistry.GlobalFiles)
+ if err != nil {
+ t.Fatal(err)
+ }
+ missingFieldsExt := dynamicpb.NewExtensionType(extFile.Extensions().Get(0))
+ gfMissingFields := dynamicpb.NewMessage(extFile.Messages().Get(0))
+ gfPresentField := gfMissingFields.Descriptor().Fields().Get(0)
+ gfMissingFields.Set(gfPresentField, gfMissingFields.NewField(gfPresentField))
+
+ testCases := []struct {
+ name string
+ ext protoreflect.ExtensionType
+ val any
+ }{
+ {
+ name: "dynamic_message",
+ ext: dynamicExt,
+ val: gf,
+ },
+ {
+ name: "not_a_message",
+ ext: nonMessageExt,
+ val: "abc",
+ },
+ {
+ name: "message_missing_fields",
+ ext: missingFieldsExt,
+ val: gfMissingFields,
+ },
+ }
+ for _, tc := range testCases {
+ tc := tc
+ t.Run(tc.name, func(t *testing.T) {
+ featureSet := &descriptorpb.FeatureSet{}
+ proto.SetExtension(featureSet, tc.ext, tc.val)
+
+ fd := &descriptorpb.FileDescriptorProto{
+ Name: proto.String("a.proto"),
+ Dependency: []string{
+ "google/protobuf/go_features.proto",
+ },
+ Edition: descriptorpb.Edition_EDITION_2023.Enum(),
+ Syntax: proto.String("editions"),
+ Options: &descriptorpb.FileOptions{
+ Features: featureSet,
+ },
+ }
+ fds := &descriptorpb.FileDescriptorSet{
+ File: []*descriptorpb.FileDescriptorProto{
+ ToFileDescriptorProto(descriptorpb.File_google_protobuf_descriptor_proto),
+ ToFileDescriptorProto(gofeaturespb.File_google_protobuf_go_features_proto),
+ fd,
+ },
+ }
+ if _, err := NewFiles(fds); err != nil {
+ t.Fatal(err)
+ }
+ })
+ }
}
