Skip to content

Commit

Permalink
Do not allow packed option on anything but repeated primitive fields (#…
Browse files Browse the repository at this point in the history 
…189)

This is a check that `protoc` does. It is also enforced by the Go
protobuf runtime: if you try to create create a
`protoreflect.FileDescriptor` where the underlying proto uses the packed
field option on a field that is not packable, it will return an error.
  • Loading branch information
@jhump
jhump authored Oct 26, 2023
1 parent 67365fa commit 7bba7a2
Show file tree
Hide file tree
Showing 3 changed files with 219 additions and 24 deletions.
3 changes: 3 additions & 0 deletions compiler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -594,6 +594,9 @@ func (t *task) link(parseRes parser.Result, deps linker.Files, overrideDescripto
if t.r.explicitFile {
file.CheckForUnusedImports(t.h)
}
if err := t.h.Error(); err != nil {
return nil, err
}

if needsSourceInfo(parseRes, t.e.c.SourceInfoMode) {
var srcInfoOpts []sourceinfo.GenerateOption
Expand Down
171 changes: 171 additions & 0 deletions linker/linker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2079,6 +2079,177 @@ func TestLinkerValidation(t *testing.T) {
expectedErr: `test.proto:4:5: feature field "raw_features" may not be used explicitly`,
expectedDiffWithProtoc: true, // seems like a bug in protoc that it allows use of raw_features
},
"success_proto2_packed": {
input: map[string]string{
"test.proto": `
syntax = "proto2";
message Foo {
repeated int32 i32 = 1 [packed=true];
repeated int64 i64 = 2 [packed=true];
repeated uint32 u32 = 3 [packed=true];
repeated uint64 u64 = 4 [packed=true];
repeated sint32 s32 = 5 [packed=true];
repeated sint64 s64 = 6 [packed=true];
repeated fixed32 f32 = 7 [packed=true];
repeated fixed64 f64 = 8 [packed=true];
repeated sfixed32 sf32 = 9 [packed=true];
repeated sfixed64 sf64 = 10 [packed=true];
repeated float flt = 11 [packed=true];
repeated double dbl = 12 [packed=true];
repeated bool bool = 13 [packed=true];
repeated En en = 14 [packed=true];
enum En { Z=0; A=1; B=2; }
}
`,
},
},
"success_proto3_packed": {
input: map[string]string{
"test.proto": `
syntax = "proto3";
message Foo {
repeated int32 i32 = 1 [packed=true];
repeated int64 i64 = 2 [packed=true];
repeated uint32 u32 = 3 [packed=true];
repeated uint64 u64 = 4 [packed=true];
repeated sint32 s32 = 5 [packed=true];
repeated sint64 s64 = 6 [packed=true];
repeated fixed32 f32 = 7 [packed=true];
repeated fixed64 f64 = 8 [packed=true];
repeated sfixed32 sf32 = 9 [packed=true];
repeated sfixed64 sf64 = 10 [packed=true];
repeated float flt = 11 [packed=true];
repeated double dbl = 12 [packed=true];
repeated bool bool = 13 [packed=true];
repeated En en = 14 [packed=true];
enum En { Z=0; A=1; B=2; }
}
`,
},
},
"failure_proto2_packed_string": {
input: map[string]string{
"test.proto": `
syntax = "proto2";
message Foo {
repeated string s = 1 [packed=true];
}
`,
},
expectedErr: `test.proto:3:12: packed option is only allowed on numeric, boolean, and enum fields`,
},
"failure_proto2_packed_bytes": {
input: map[string]string{
"test.proto": `
syntax = "proto2";
message Foo {
repeated bytes b = 1 [packed=true];
}
`,
},
expectedErr: `test.proto:3:12: packed option is only allowed on numeric, boolean, and enum fields`,
},
"failure_proto2_packed_msg": {
input: map[string]string{
"test.proto": `
syntax = "proto2";
message Foo {
repeated Foo msgs = 1 [packed=true];
}
`,
},
expectedErr: `test.proto:3:12: packed option is only allowed on numeric, boolean, and enum fields`,
},
"failure_proto2_packed_group": {
input: map[string]string{
"test.proto": `
syntax = "proto2";
message Foo {
repeated group G = 1 [packed=true] {
optional string name = 1;
}
}
`,
},
expectedErr: `test.proto:3:12: packed option is only allowed on numeric, boolean, and enum fields`,
},
"failure_proto2_packed_map": {
input: map[string]string{
"test.proto": `
syntax = "proto2";
message Foo {
map<int32,int32> m = 1 [packed=true];
}
`,
},
expectedErr: `test.proto:3:3: packed option is only allowed on numeric, boolean, and enum fields`,
},
"failure_proto2_packed_nonrepeated": {
input: map[string]string{
"test.proto": `
syntax = "proto2";
message Foo {
optional int32 i32 = 1 [packed=true];
}
`,
},
expectedErr: `test.proto:3:3: packed option is only allowed on repeated fields`,
},
"failure_proto3_packed_string": {
input: map[string]string{
"test.proto": `
syntax = "proto3";
message Foo {
repeated string s = 1 [packed=true];
}
`,
},
expectedErr: `test.proto:3:12: packed option is only allowed on numeric, boolean, and enum fields`,
},
"failure_proto3_packed_bytes": {
input: map[string]string{
"test.proto": `
syntax = "proto3";
message Foo {
repeated bytes b = 1 [packed=true];
}
`,
},
expectedErr: `test.proto:3:12: packed option is only allowed on numeric, boolean, and enum fields`,
},
"failure_proto3_packed_msg": {
input: map[string]string{
"test.proto": `
syntax = "proto3";
message Foo {
repeated Foo msgs = 1 [packed=true];
}
`,
},
expectedErr: `test.proto:3:12: packed option is only allowed on numeric, boolean, and enum fields`,
},
"failure_proto3_packed_map": {
input: map[string]string{
"test.proto": `
syntax = "proto3";
message Foo {
map<int32,int32> m = 1 [packed=true];
}
`,
},
expectedErr: `test.proto:3:3: packed option is only allowed on numeric, boolean, and enum fields`,
},
"failure_proto3_packed_nonrepeated": {
input: map[string]string{
"test.proto": `
syntax = "proto3";
message Foo {
optional int32 i32 = 1 [packed=true];
}
`,
},
expectedErr: `test.proto:3:3: packed option is only allowed on repeated fields`,
},
}

for name, tc := range testCases {
Expand Down
69 changes: 45 additions & 24 deletions linker/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,29 +25,36 @@ import (

"github.com/bufbuild/protocompile/internal"
"github.com/bufbuild/protocompile/reporter"
"github.com/bufbuild/protocompile/walk"
)

// ValidateOptions runs some validation checks on the result that can only
// be done after options are interpreted.
func (r *result) ValidateOptions(handler *reporter.Handler) error {
if err := r.validateExtensions(r, handler); err != nil {
return err
}
return r.validateJSONNamesInFile(handler)
}

func (r *result) validateExtensions(d hasExtensionsAndMessages, handler *reporter.Handler) error {
for i := 0; i < d.Extensions().Len(); i++ {
if err := r.validateExtension(d.Extensions().Get(i), handler); err != nil {
return err
}
}
for i := 0; i < d.Messages().Len(); i++ {
if err := r.validateExtensions(d.Messages().Get(i), handler); err != nil {
return err
return walk.Descriptors(r, func(d protoreflect.Descriptor) error {
switch d := d.(type) {
case protoreflect.FieldDescriptor:
if d.IsExtension() {
if err := r.validateExtension(d, handler); err != nil {
return err
}
}
if err := r.validatePacked(d, handler); err != nil {
return err
}
case protoreflect.MessageDescriptor:
md := d.(*msgDescriptor) //nolint:errcheck
if err := r.validateJSONNamesInMessage(md.proto, handler); err != nil {
return err
}
case protoreflect.EnumDescriptor:
ed := d.(*enumDescriptor) //nolint:errcheck
if err := r.validateJSONNamesInEnum(ed.proto, handler); err != nil {
return err
}
}
}
return nil
return nil
})
}

func (r *result) validateExtension(fld protoreflect.FieldDescriptor, handler *reporter.Handler) error {
Expand Down Expand Up @@ -83,17 +90,31 @@ func (r *result) validateExtension(fld protoreflect.FieldDescriptor, handler *re
return nil
}

func (r *result) validateJSONNamesInFile(handler *reporter.Handler) error {
for _, md := range r.FileDescriptorProto().GetMessageType() {
if err := r.validateJSONNamesInMessage(md, handler); err != nil {
return err
}
func (r *result) validatePacked(fld protoreflect.FieldDescriptor, handler *reporter.Handler) error {
if xtd, ok := fld.(protoreflect.ExtensionTypeDescriptor); ok {
fld = xtd.Descriptor()
}
for _, ed := range r.FileDescriptorProto().GetEnumType() {
if err := r.validateJSONNamesInEnum(ed, handler); err != nil {

fd := fld.(*fldDescriptor) //nolint:errcheck
if !fd.proto.GetOptions().GetPacked() {
// if packed isn't true, nothing to validate
return nil
}
if fd.proto.GetLabel() != descriptorpb.FieldDescriptorProto_LABEL_REPEATED {
file := r.FileNode()
pos := file.NodeInfo(r.FieldNode(fd.proto).FieldLabel()).Start()
err := handler.HandleErrorf(pos, "packed option is only allowed on repeated fields")
if err != nil {
return err
}
}
switch fd.proto.GetType() {
case descriptorpb.FieldDescriptorProto_TYPE_STRING, descriptorpb.FieldDescriptorProto_TYPE_BYTES,
descriptorpb.FieldDescriptorProto_TYPE_MESSAGE, descriptorpb.FieldDescriptorProto_TYPE_GROUP:
file := r.FileNode()
pos := file.NodeInfo(r.FieldNode(fd.proto).FieldType()).Start()
return handler.HandleErrorf(pos, "packed option is only allowed on numeric, boolean, and enum fields")
}
return nil
}

Expand Down

0 comments on commit 7bba7a2

Please sign in to comment.