Repository: jhump/protoreflect (go)

Issue:
protoparse: does not enforce required message set properties
When a message has message set wire encoding set to true, the message should have no fields and at least one extension range defined. Furthermore, all extensions defined for a message that uses message set encoding must be messages themselves: no scalar extension fields. 

These are constraints enforced by protoc, but are not enforced by protoparse. So protoparse will accept an invalid proto source file, that protoc rejects.

Pull request:
protoparse enforces add'l requirements for message sets
There are several requirements for messages that have message-set wire format that protoc enforces, but that protoparse was failing to enforce.

Resolves #359

Patch:
diff --git a/desc/protoparse/descriptor_protos.go b/desc/protoparse/descriptor_protos.go
index 9266456..4113454 100644
--- a/desc/protoparse/descriptor_protos.go
+++ b/desc/protoparse/descriptor_protos.go
@@ -389,9 +389,10 @@ func (r *parseResult) addMessageBody(msgd *dpb.DescriptorProto, body *ast.Messag
 	// now that we have options, we can see if this uses messageset wire format, which
 	// impacts how we validate tag numbers in any fields in the message
 	maxTag := int32(internal.MaxNormalTag)
-	if isMessageSet, err := isMessageSetWireFormat(r, "message "+msgd.GetName(), msgd); err != nil {
+	messageSetOpt, err := isMessageSetWireFormat(r, "message "+msgd.GetName(), msgd)
+	if err != nil {
 		return
-	} else if isMessageSet {
+	} else if messageSetOpt != nil {
 		maxTag = internal.MaxTag // higher limit for messageset wire format
 	}
 
@@ -463,33 +464,44 @@ func (r *parseResult) addMessageBody(msgd *dpb.DescriptorProto, body *ast.Messag
 		}
 	}
 
+	if messageSetOpt != nil {
+		if len(msgd.Field) > 0 {
+			node := r.getFieldNode(msgd.Field[0])
+			_ = r.errs.handleErrorWithPos(node.Start(), "messages with message-set wire format cannot contain non-extension fields")
+		}
+		if len(msgd.ExtensionRange) == 0 {
+			node := r.getOptionNode(messageSetOpt)
+			_ = r.errs.handleErrorWithPos(node.Start(), "messages with message-set wire format must contain at least one extension range")
+		}
+	}
+
 	// process any proto3_optional fields
 	if isProto3 {
 		internal.ProcessProto3OptionalFields(msgd)
 	}
 }
 
-func isMessageSetWireFormat(res *parseResult, scope string, md *dpb.DescriptorProto) (bool, error) {
+func isMessageSetWireFormat(res *parseResult, scope string, md *dpb.DescriptorProto) (*dpb.UninterpretedOption, error) {
 	uo := md.GetOptions().GetUninterpretedOption()
 	index, err := findOption(res, scope, uo, "message_set_wire_format")
 	if err != nil {
-		return false, err
+		return nil, err
 	}
 	if index == -1 {
-		// no such option, so default to false
-		return false, nil
+		// no such option
+		return nil, nil
 	}
 
 	opt := uo[index]
-	optNode := res.getOptionNode(opt)
 
 	switch opt.GetIdentifierValue() {
 	case "true":
-		return true, nil
+		return opt, nil
 	case "false":
-		return false, nil
+		return nil, nil
 	default:
-		return false, res.errs.handleErrorWithPos(optNode.GetValue().Start(), "%s: expecting bool value for message_set_wire_format option", scope)
+		optNode := res.getOptionNode(opt)
+		return nil, res.errs.handleErrorWithPos(optNode.GetValue().Start(), "%s: expecting bool value for message_set_wire_format option", scope)
 	}
 }
 
diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go
index b1ecb9d..872b42f 100644
--- a/desc/protoparse/linker.go
+++ b/desc/protoparse/linker.go
@@ -69,7 +69,7 @@ func (l *linker) linkFiles() (map[string]*desc.FileDescriptor, error) {
 		}
 		// we should now have any message_set_wire_format options parsed
 		// and can do further validation on tag ranges
-		if err := checkExtensionTagsInFile(fd, r); err != nil {
+		if err := checkExtensionsInFile(fd, r); err != nil {
 			return nil, err
 		}
 	}
diff --git a/desc/protoparse/parser.go b/desc/protoparse/parser.go
index 3785c2f..bbfec8d 100644
--- a/desc/protoparse/parser.go
+++ b/desc/protoparse/parser.go
@@ -798,46 +798,55 @@ func checkTag(pos *SourcePos, v uint64, maxTag int32) error {
 	return nil
 }
 
-func checkExtensionTagsInFile(fd *desc.FileDescriptor, res *parseResult) error {
+func checkExtensionsInFile(fd *desc.FileDescriptor, res *parseResult) error {
 	for _, fld := range fd.GetExtensions() {
-		if err := checkExtensionTag(fld, res); err != nil {
+		if err := checkExtension(fld, res); err != nil {
 			return err
 		}
 	}
 	for _, md := range fd.GetMessageTypes() {
-		if err := checkExtensionTagsInMessage(md, res); err != nil {
+		if err := checkExtensionsInMessage(md, res); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
-func checkExtensionTagsInMessage(md *desc.MessageDescriptor, res *parseResult) error {
+func checkExtensionsInMessage(md *desc.MessageDescriptor, res *parseResult) error {
 	for _, fld := range md.GetNestedExtensions() {
-		if err := checkExtensionTag(fld, res); err != nil {
+		if err := checkExtension(fld, res); err != nil {
 			return err
 		}
 	}
 	for _, nmd := range md.GetNestedMessageTypes() {
-		if err := checkExtensionTagsInMessage(nmd, res); err != nil {
+		if err := checkExtensionsInMessage(nmd, res); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
-func checkExtensionTag(fld *desc.FieldDescriptor, res *parseResult) error {
-	// NB: This is kind of gross that we don't enforce this in validateBasic(). But it would
-	// require doing some minimal linking there (to identify the extendee and locate its
-	// descriptor). To keep the code simpler, we just wait until things are fully linked.
-
-	// In validateBasic() we just made sure these were within bounds for any message. But
-	// now that things are linked, we can check if the extendee is messageset wire format
-	// and, if not, enforce tighter limit.
-	if !fld.GetOwner().GetMessageOptions().GetMessageSetWireFormat() && fld.GetNumber() > internal.MaxNormalTag {
-		pos := res.getFieldNode(fld.AsFieldDescriptorProto()).FieldTag().Start()
-		return errorWithPos(pos, "tag number %d is higher than max allowed tag number (%d)", fld.GetNumber(), internal.MaxNormalTag)
+func checkExtension(fld *desc.FieldDescriptor, res *parseResult) error {
+	// NB: It's a little gross that we don't enforce these in validateBasic().
+	// But requires some minimal linking to resolve the extendee, so we can
+	// interrogate its descriptor.
+	if fld.GetOwner().GetMessageOptions().GetMessageSetWireFormat() {
+		// Message set wire format requires that all extensions be messages
+		// themselves (no scalar extensions)
+		if fld.GetType() != dpb.FieldDescriptorProto_TYPE_MESSAGE {
+			pos := res.getFieldNode(fld.AsFieldDescriptorProto()).FieldType().Start()
+			return errorWithPos(pos, "messages with message-set wire format cannot contain scalar extensions, only messages")
+		}
+	} else {
+		// In validateBasic() we just made sure these were within bounds for any message. But
+		// now that things are linked, we can check if the extendee is messageset wire format
+		// and, if not, enforce tighter limit.
+		if fld.GetNumber() > internal.MaxNormalTag {
+			pos := res.getFieldNode(fld.AsFieldDescriptorProto()).FieldTag().Start()
+			return errorWithPos(pos, "tag number %d is higher than max allowed tag number (%d)", fld.GetNumber(), internal.MaxNormalTag)
+		}
 	}
+
 	return nil
 }
 


Test patch:
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 274a7c2..b378ac5 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -325,6 +325,30 @@ func TestLinkerValidation(t *testing.T) {
 			},
 			"foo.proto:1:1: error in file options: some required fields missing: (f).b",
 		},
+		{
+			map[string]string{
+				"foo.proto": "message Foo { option message_set_wire_format = true; extensions 1 to 100; } extend Foo { optional int32 bar = 1; }",
+			},
+			"foo.proto:1:99: messages with message-set wire format cannot contain scalar extensions, only messages",
+		},
+		{
+			map[string]string{
+				"foo.proto": "message Foo { option message_set_wire_format = true; extensions 1 to 100; } extend Foo { optional Foo bar = 1; }",
+			},
+			"", // should succeed
+		},
+		{
+			map[string]string{
+				"foo.proto": "message Foo { extensions 1 to max; } extend Foo { optional int32 bar = 536870912; }",
+			},
+			"foo.proto:1:72: field bar: tag 536870912 is not in valid range for extended type Foo",
+		},
+		{
+			map[string]string{
+				"foo.proto": "message Foo { option message_set_wire_format = true; extensions 1 to max; } extend Foo { optional Foo bar = 536870912; }",
+			},
+			"", // should succeed
+		},
 	}
 	for i, tc := range testCases {
 		acc := func(filename string) (io.ReadCloser, error) {
@@ -339,7 +363,11 @@ func TestLinkerValidation(t *testing.T) {
 			names = append(names, k)
 		}
 		_, err := Parser{Accessor: acc}.ParseFiles(names...)
-		if err == nil {
+		if tc.errMsg == "" {
+			if err != nil {
+				t.Errorf("case %d: expecting no error; instead got error %q", i, err)
+			}
+		} else if err == nil {
 			t.Errorf("case %d: expecting validation error %q; instead got no error", i, tc.errMsg)
 		} else if err.Error() != tc.errMsg {
 			t.Errorf("case %d: expecting validation error %q; instead got: %q", i, tc.errMsg, err)
diff --git a/desc/protoparse/validate_test.go b/desc/protoparse/validate_test.go
index 872a8d5..3f85de9 100644
--- a/desc/protoparse/validate_test.go
+++ b/desc/protoparse/validate_test.go
@@ -27,13 +27,21 @@ func TestBasicValidation(t *testing.T) {
 			succeeds: true,
 		},
 		{
-			contents: `message Foo { optional double bar = 536870912; option message_set_wire_format = true; }`,
+			contents: `message Foo { oneof bar { group Baz = 1 [deprecated=true] { optional int abc = 1; } } }`,
 			succeeds: true,
 		},
 		{
-			contents: `message Foo { oneof bar { group Baz = 1 [deprecated=true] { optional int abc = 1; } } }`,
+			contents: `message Foo { option message_set_wire_format = true; extensions 1 to 100; }`,
 			succeeds: true,
 		},
+		{
+			contents: `message Foo { optional double bar = 536870912; option message_set_wire_format = true; }`,
+			errMsg:   "test.proto:1:15: messages with message-set wire format cannot contain non-extension fields",
+		},
+		{
+			contents: `message Foo { option message_set_wire_format = true; }`,
+			errMsg:   "test.proto:1:15: messages with message-set wire format must contain at least one extension range",
+		},
 		{
 			contents: `syntax = "proto1";`,
 			errMsg:   `test.proto:1:10: syntax value must be "proto2" or "proto3"`,