diff --git a/private/bufpkg/bufcheck/client.go b/private/bufpkg/bufcheck/client.go index ae802dbfa2..d76004f980 100644 --- a/private/bufpkg/bufcheck/client.go +++ b/private/bufpkg/bufcheck/client.go @@ -109,7 +109,12 @@ func (c *client) Lint( return err } logRulesConfig(c.logger, config.rulesConfig) - files, err := descriptor.FileDescriptorsForProtoFileDescriptors(imageToProtoFileDescriptors(image)) + protoFileDescriptors, err := imageToProtoFileDescriptors(image) + if err != nil { + // If a validated Image results in an error, this is a system error. + return syserror.Wrap(err) + } + files, err := descriptor.FileDescriptorsForProtoFileDescriptors(protoFileDescriptors) if err != nil { // If a validated Image results in an error, this is a system error. return syserror.Wrap(err) @@ -174,12 +179,22 @@ func (c *client) Breaking( return err } logRulesConfig(c.logger, config.rulesConfig) - fileDescriptors, err := descriptor.FileDescriptorsForProtoFileDescriptors(imageToProtoFileDescriptors(image)) + protoFileDescriptors, err := imageToProtoFileDescriptors(image) + if err != nil { + // If a validated Image results in an error, this is a system error. + return syserror.Wrap(err) + } + fileDescriptors, err := descriptor.FileDescriptorsForProtoFileDescriptors(protoFileDescriptors) + if err != nil { + // If a validated Image results in an error, this is a system error. + return syserror.Wrap(err) + } + againstProtoFileDescriptors, err := imageToProtoFileDescriptors(againstImage) if err != nil { // If a validated Image results in an error, this is a system error. return syserror.Wrap(err) } - againstFileDescriptors, err := descriptor.FileDescriptorsForProtoFileDescriptors(imageToProtoFileDescriptors(againstImage)) + againstFileDescriptors, err := descriptor.FileDescriptorsForProtoFileDescriptors(againstProtoFileDescriptors) if err != nil { // If a validated Image results in an error, this is a system error. return syserror.Wrap(err) diff --git a/private/bufpkg/bufcheck/util.go b/private/bufpkg/bufcheck/util.go index 06b143703d..31b647af08 100644 --- a/private/bufpkg/bufcheck/util.go +++ b/private/bufpkg/bufcheck/util.go @@ -15,16 +15,61 @@ package bufcheck import ( + "slices" + descriptorv1 "buf.build/gen/go/bufbuild/bufplugin/protocolbuffers/go/buf/plugin/descriptor/v1" "github.com/bufbuild/buf/private/bufpkg/bufimage" + "github.com/bufbuild/buf/private/pkg/protoencoding" "github.com/bufbuild/buf/private/pkg/slicesext" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/descriptorpb" ) -func imageToProtoFileDescriptors(image bufimage.Image) []*descriptorv1.FileDescriptor { +func imageToProtoFileDescriptors(image bufimage.Image) ([]*descriptorv1.FileDescriptor, error) { if image == nil { - return nil + return nil, nil + } + descriptors := slicesext.Map(image.Files(), imageToProtoFileDescriptor) + // We need to ensure that if a FileDescriptorProto includes a Go + // feature set extension, that it matches the runtime version in + // gofeaturespb. The runtime will use the gofeaturespb.E_Go extension + // type to determine how to parse the file. This must match the expected + // go type to avoid panics on getting the extension when using + // proto.GetExtension or protodesc.NewFiles. We therefore reparse all + // extensions if it may contain any Go feature set extensions. + // See the issue: https://github.com/golang/protobuf/issues/1669 + const goFeaturesImportPath = "google/protobuf/go_features.proto" + var reparseDescriptors []*descriptorv1.FileDescriptor + for _, descriptor := range descriptors { + fileDescriptorProto := descriptor.FileDescriptorProto + // Trigger reparsing on any file that includes the gofeatures import. + if slices.Contains(fileDescriptorProto.Dependency, goFeaturesImportPath) { + reparseDescriptors = append(reparseDescriptors, descriptor) + } + } + if len(reparseDescriptors) == 0 { + return descriptors, nil + } + goFeaturesResolver, err := protoencoding.NewGoFeaturesResolver() + if err != nil { + return nil, err + } + resolver := protoencoding.CombineResolvers( + goFeaturesResolver, + protoencoding.NewLazyResolver(slicesext.Map(descriptors, func(fileDescriptor *descriptorv1.FileDescriptor) *descriptorpb.FileDescriptorProto { + return fileDescriptor.FileDescriptorProto + })...), + ) + for _, descriptor := range reparseDescriptors { + // We clone the FileDescriptorProto to avoid modifying the original. + fileDescriptorProto := &descriptorpb.FileDescriptorProto{} + proto.Merge(fileDescriptorProto, descriptor.FileDescriptorProto) + if err := protoencoding.ReparseExtensions(resolver, fileDescriptorProto.ProtoReflect()); err != nil { + return nil, err + } + descriptor.FileDescriptorProto = fileDescriptorProto } - return slicesext.Map(image.Files(), imageToProtoFileDescriptor) + return descriptors, nil } func imageToProtoFileDescriptor(imageFile bufimage.ImageFile) *descriptorv1.FileDescriptor { diff --git a/private/pkg/protoencoding/protoencoding.go b/private/pkg/protoencoding/protoencoding.go index 812887473d..fa590e7c6f 100644 --- a/private/pkg/protoencoding/protoencoding.go +++ b/private/pkg/protoencoding/protoencoding.go @@ -15,6 +15,8 @@ package protoencoding import ( + "sync" + "buf.build/go/protoyaml" "github.com/bufbuild/buf/private/pkg/protodescriptor" "google.golang.org/protobuf/proto" @@ -26,6 +28,13 @@ import ( // EmptyResolver is a resolver that never resolves any descriptors. All methods will return (nil, NotFound). var EmptyResolver Resolver = emptyResolver{} +var ( + // goFeaturesOnce is used to lazily create goFeaturesValue in NewGoFeaturesResolver. + goFeaturesOnce sync.Once + goFeaturesValue *goFeaturesResolver + goFeaturesErr error +) + // Resolver can resolve files, messages, enums, and extensions. type Resolver interface { protodesc.Resolver @@ -54,6 +63,15 @@ func NewLazyResolver[F protodescriptor.FileDescriptor](fileDescriptors ...F) Res }} } +// NewGoFeaturesResolver returns a new Resolver that resolves Go features to +// the gofeaturespb package. +func NewGoFeaturesResolver() (Resolver, error) { + goFeaturesOnce.Do(func() { + goFeaturesValue, goFeaturesErr = newGoFeaturesResolver() + }) + return goFeaturesValue, goFeaturesErr +} + // CombineResolvers returns a resolver that uses all of the given resolvers. It // will use the first resolver, and if it returns an error, the second will be // tried, and so on. diff --git a/private/pkg/protoencoding/reparse_extensions_test.go b/private/pkg/protoencoding/reparse_extensions_test.go index e11bd07cd6..4067b09342 100644 --- a/private/pkg/protoencoding/reparse_extensions_test.go +++ b/private/pkg/protoencoding/reparse_extensions_test.go @@ -26,9 +26,11 @@ import ( "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protodesc" "google.golang.org/protobuf/reflect/protoreflect" + "google.golang.org/protobuf/reflect/protoregistry" "google.golang.org/protobuf/testing/protocmp" "google.golang.org/protobuf/types/descriptorpb" "google.golang.org/protobuf/types/dynamicpb" + "google.golang.org/protobuf/types/gofeaturespb" "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/protobuf/types/known/timestamppb" ) @@ -126,3 +128,66 @@ func TestReparseExtensions(t *testing.T) { }) assert.Equal(t, 2, found) } + +func TestReparseExtensionsGoFeatures(t *testing.T) { + t.Parallel() + + goFeaturesMessageDesc := gofeaturespb.File_google_protobuf_go_features_proto.Messages().ByName("GoFeatures") + dynamicGoFeatures := dynamicpb.NewMessage(goFeaturesMessageDesc) + dynamicGoFeatures.Set( + goFeaturesMessageDesc.Fields().ByName("api_level"), + protoreflect.ValueOfEnum(gofeaturespb.GoFeatures_API_OPAQUE.Number()), + ) + assert.True(t, dynamicGoFeatures.IsValid()) + dynamicExt := dynamicpb.NewExtensionType(gofeaturespb.E_Go.TypeDescriptor().Descriptor()) + + featureSet := &descriptorpb.FeatureSet{} + featureSetReflect := featureSet.ProtoReflect() + featureSetReflect.Set( + dynamicExt.TypeDescriptor(), + protoreflect.ValueOfMessage(dynamicGoFeatures), + ) + + // Validates the error conditions that cause this panic. + // See issue https://github.com/golang/protobuf/issues/1669 + assert.Panics(t, func() { + proto.GetExtension(featureSet, gofeaturespb.E_Go) + }) + descFileDesc, err := protoregistry.GlobalFiles.FindFileByPath("google/protobuf/descriptor.proto") + assert.NoError(t, err) + goFeaturesFileDesc, err := protoregistry.GlobalFiles.FindFileByPath("google/protobuf/go_features.proto") + assert.NoError(t, err) + fileDesc := &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, + }, + } + fileSet := &descriptorpb.FileDescriptorSet{ + File: []*descriptorpb.FileDescriptorProto{ + protodesc.ToFileDescriptorProto(descFileDesc), + protodesc.ToFileDescriptorProto(goFeaturesFileDesc), + fileDesc, + }, + } + assert.Panics(t, func() { + // TODO: if this no longer panics, we can remove the code handling + // this workaround in bufcheck.imageToProtoFileDescriptors. + _, err := protodesc.NewFiles(fileSet) + assert.NoError(t, err) + }) + + // Run the resvoler to convert the extension. + goFeaturesResolver, err := newGoFeaturesResolver() + require.NoError(t, err) + err = ReparseExtensions(goFeaturesResolver, featureSetReflect) + require.NoError(t, err) + goFeatures, ok := proto.GetExtension(featureSet, gofeaturespb.E_Go).(*gofeaturespb.GoFeatures) + require.True(t, ok) + assert.Equal(t, goFeatures.GetApiLevel(), gofeaturespb.GoFeatures_API_OPAQUE) +} diff --git a/private/pkg/protoencoding/resolver.go b/private/pkg/protoencoding/resolver.go index 3b6c182fc2..99d720435a 100644 --- a/private/pkg/protoencoding/resolver.go +++ b/private/pkg/protoencoding/resolver.go @@ -22,6 +22,7 @@ import ( "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/reflect/protoregistry" "google.golang.org/protobuf/types/dynamicpb" + "google.golang.org/protobuf/types/gofeaturespb" ) const maxTagNumber = 536870911 // 2^29 - 1 @@ -248,3 +249,34 @@ func (emptyResolver) FindMessageByName(protoreflect.FullName) (protoreflect.Mess func (emptyResolver) FindMessageByURL(string) (protoreflect.MessageType, error) { return nil, protoregistry.NotFound } + +type goFeaturesResolver struct { + protoregistry.Files + protoregistry.Types +} + +func newGoFeaturesResolver() (*goFeaturesResolver, error) { + var protoregistryFiles protoregistry.Files + if err := protoregistryFiles.RegisterFile( + gofeaturespb.File_google_protobuf_go_features_proto, + ); err != nil { + return nil, err + } + + var protoregistryTypes protoregistry.Types + if err := protoregistryTypes.RegisterExtension( + gofeaturespb.E_Go.TypeDescriptor().Type(), + ); err != nil { + return nil, err + } + if err := protoregistryTypes.RegisterMessage( + (&gofeaturespb.GoFeatures{}).ProtoReflect().Type(), + ); err != nil { + return nil, err + } + + return &goFeaturesResolver{ + Files: protoregistryFiles, + Types: protoregistryTypes, + }, nil +}