reflect/protodesc: fix panic when working with dynamicpb

Thanks to Joshua Humphries and Edward McFarlane for the excellent bug report, reproducer and fix suggestion! Fixes golang/protobuf#1669 Change-Id: I03df76f789e6e11b53396396a1f6b58bb3fb840b Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/642575 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Damien Neil <[email protected]> Reviewed-by: Chressie Himpel <[email protected]>
protocolbuffers · Jan 15, 2025 · 7cbd915 · 7cbd915
1 parent 2f60868
commit 7cbd915
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 10 deletions.
diff --git a/reflect/protodesc/editions.go b/reflect/protodesc/editions.go
@@ -125,16 +125,27 @@ func mergeEditionFeatures(parentDesc protoreflect.Descriptor, child *descriptorp
 		parentFS.IsJSONCompliant = *jf == descriptorpb.FeatureSet_ALLOW
 	}
 
-	if goFeatures, ok := proto.GetExtension(child, gofeaturespb.E_Go).(*gofeaturespb.GoFeatures); ok && goFeatures != nil {
-		if luje := goFeatures.LegacyUnmarshalJsonEnum; luje != nil {
-			parentFS.GenerateLegacyUnmarshalJSON = *luje
-		}
-		if sep := goFeatures.StripEnumPrefix; sep != nil {
-			parentFS.StripEnumPrefix = int(*sep)
-		}
-		if al := goFeatures.ApiLevel; al != nil {
-			parentFS.APILevel = int(*al)
-		}
+	// 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.
+	goFeatures := child.ProtoReflect().Get(gofeaturespb.E_Go.TypeDescriptor())
+	if !goFeatures.IsValid() {
+		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) {
+		parentFS.GenerateLegacyUnmarshalJSON = gf.Get(fd).Bool()
+	}
+
+	if fd := fields.ByName("strip_enum_prefix"); gf.Has(fd) {
+		parentFS.StripEnumPrefix = int(gf.Get(fd).Enum())
+	}
+
+	if fd := fields.ByName("api_level"); gf.Has(fd) {
+		parentFS.APILevel = int(gf.Get(fd).Enum())
 	}
 
 	return parentFS

diff --git a/reflect/protodesc/editions_test.go b/reflect/protodesc/editions_test.go
@@ -0,0 +1,47 @@
+// Copyright 2025 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package protodesc
+
+import (
+	"testing"
+
+	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/reflect/protoreflect"
+	"google.golang.org/protobuf/types/descriptorpb"
+	"google.golang.org/protobuf/types/dynamicpb"
+	"google.golang.org/protobuf/types/gofeaturespb"
+)
+
+func TestGoFeaturesDynamic(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{}
+	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",
+		},
+		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)
+	}
+}