Skip to content

Commit

Permalink
Fix Go feature set handling in editions for checks
Browse files Browse the repository at this point in the history
This fixes the handling of the Go feature set in editions for use with
checks. Dynamic types would cause the google.golang.org/protobuf
library to panic when trying to fetch the Go feature extension. This is
a temporary workaround until upstream can be resolved. See the issue:
golang/protobuf#1669

When resolving file descriptors for checks, we now check for use of
google/protobuf/go_features.proto and reparse extensions with a resolver
targetted at the gofeaturespb package. This ensure the types will match
the ones set in the extension type gofeaturespb.E_go.

Fixes #3580
  • Loading branch information
emcfarlane committed Jan 13, 2025
1 parent c60d858 commit 8fc576e
Show file tree
Hide file tree
Showing 5 changed files with 181 additions and 6 deletions.
21 changes: 18 additions & 3 deletions private/bufpkg/bufcheck/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
51 changes: 48 additions & 3 deletions private/bufpkg/bufcheck/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
18 changes: 18 additions & 0 deletions private/pkg/protoencoding/protoencoding.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
package protoencoding

import (
"sync"

"buf.build/go/protoyaml"
"github.com/bufbuild/buf/private/pkg/protodescriptor"
"google.golang.org/protobuf/proto"
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down
65 changes: 65 additions & 0 deletions private/pkg/protoencoding/reparse_extensions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
}
32 changes: 32 additions & 0 deletions private/pkg/protoencoding/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

0 comments on commit 8fc576e

Please sign in to comment.