Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic in protodesc.NewFiles if schema uses dynamic message with an older schema of go_features.proto #1671

Closed
emcfarlane opened this issue Jan 15, 2025 · 1 comment
Assignees

Comments

@emcfarlane
Copy link

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

What did you do?
This is a follow on from #1669 . I am loading a dynamic schema that contains an older version of go_features.proto (the version included at protobuf v1.29.1). This does not include the expected field strip_enum_prefix.

What did you expect to see?
Expect the field to act as not present if the dynamic message doesn't include the field.

What did you see instead?
With reflection the field lookup returns a nil field descriptor which panics when calling Has(fd) here: https://github.com/protocolbuffers/protobuf-go/blob/0c3cc2f8edbac7858cdff44cbaaaea2cb415cd0f/reflect/protodesc/editions.go#L143

google.golang.org/protobuf/types/dynamicpb.(*Message).checkField(0x14000739400, {0x0, 0x0})
        /Users/edward/.cache/buf/Darwin/arm64/go/pkg/mod/google.golang.org/[email protected]/types/dynamicpb/dynamic.go:342 +0x28
google.golang.org/protobuf/types/dynamicpb.(*Message).Has(0x14000739400, {0x0, 0x0})
        /Users/edward/.cache/buf/Darwin/arm64/go/pkg/mod/google.golang.org/[email protected]/types/dynamicpb/dynamic.go:156 +0x28
google.golang.org/protobuf/reflect/protodesc.mergeEditionFeatures({0x1061013e0?, 0x140018eef00?}, 0x0?)
        /Users/edward/.cache/buf/Darwin/arm64/go/pkg/mod/google.golang.org/[email protected]/reflect/protodesc/editions.go:143 +0x2d8
google.golang.org/protobuf/reflect/protodesc.initFileDescFromFeatureSet(0x140018eef00, 0x140001acae0)
        /Users/edward/.cache/buf/Darwin/arm64/go/pkg/mod/google.golang.org/[email protected]/reflect/protodesc/editions.go:163 +0x90
google.golang.org/protobuf/reflect/protodesc.FileOptions.New({{}, 0x40?}, 0x1400010f600, {0x1060dc2e8?, 0x14000731b48?})
        /Users/edward/.cache/buf/Darwin/arm64/go/pkg/mod/google.golang.org/[email protected]/reflect/protodesc/desc.go:126 +0x448
google.golang.org/protobuf/reflect/protodesc.FileOptions.addFileDeps({{}, 0x58?}, 0x14000731b48, 0x1400010f600, 0x1400121d228)
        /Users/edward/.cache/buf/Darwin/arm64/go/pkg/mod/google.golang.org/[email protected]/reflect/protodesc/desc.go:287 +0x1ec
google.golang.org/protobuf/reflect/protodesc.FileOptions.NewFiles({{}, 0x80?}, 0x1400121d360)
        /Users/edward/.cache/buf/Darwin/arm64/go/pkg/mod/google.golang.org/[email protected]/reflect/protodesc/desc.go:264 +0x26c
google.golang.org/protobuf/reflect/protodesc.NewFiles(...)
        /Users/edward/.cache/buf/Darwin/arm64/go/pkg/mod/google.golang.org/[email protected]/reflect/protodesc/desc.go:76
buf.build/go/bufplugin/descriptor.FileDescriptorsForProtoFileDescriptors({0x14000731b18, 0x3, 0x140011d9108?})
        /Users/edward/.cache/buf/Darwin/arm64/go/pkg/mod/buf.build/go/[email protected]/descriptor/file_descriptor.go:91 +0x24c

Anything else we should know about your project / environment?
I think a nil check is missing for each field descriptor to ensure the field exists before checking if it has been set:

diff --git a/reflect/protodesc/editions.go b/reflect/protodesc/editions.go
index f55b0369..b334e69a 100644
--- a/reflect/protodesc/editions.go
+++ b/reflect/protodesc/editions.go
@@ -136,15 +136,15 @@ func mergeEditionFeatures(parentDesc protoreflect.Descriptor, child *descriptorp
        gf := goFeatures.Message()
        fields := gf.Descriptor().Fields()

-       if fd := fields.ByName("legacy_unmarshal_json_enum"); gf.Has(fd) {
+       if fd := fields.ByName("legacy_unmarshal_json_enum"); fd != nil && gf.Has(fd) {
                parentFS.GenerateLegacyUnmarshalJSON = gf.Get(fd).Bool()
        }

-       if fd := fields.ByName("strip_enum_prefix"); gf.Has(fd) {
+       if fd := fields.ByName("strip_enum_prefix"); fd != nil && gf.Has(fd) {
                parentFS.StripEnumPrefix = int(gf.Get(fd).Enum())
        }

-       if fd := fields.ByName("api_level"); gf.Has(fd) {
+       if fd := fields.ByName("api_level"); fd != nil && gf.Has(fd) {
                parentFS.APILevel = int(gf.Get(fd).Enum())
        }
@stapelberg stapelberg self-assigned this Jan 15, 2025
@jhump
Copy link
Contributor

jhump commented Jan 15, 2025

I think this CL should do the trick here: https://go-review.googlesource.com/c/protobuf/+/642975

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants