Repository: jhump/protoreflect (go)

Issue:
nil pointer dereference in linker.go
I was trying to compile the [envoy protobufs](https://github.com/envoyproxy/data-plane-api) using buf-1.3.1 and protoreflect-1.11.1 when the program crashed:

```
$ buf lint
Failure: panic: runtime error: invalid memory address or nil pointer dereference, stack:
goroutine 67 [running]:
runtime/debug.Stack()
	/gnu/store/aiw1716ia4qgwsckcls59sw57dwqr3fg-go-1.17.6/lib/go/src/runtime/debug/stack.go:24 +0x65
github.com/bufbuild/buf/private/bufpkg/bufimage/bufimagebuild.getBuildResults.func1.2()
	/tmp/guix-build-buf-1.3.1.drv-0/source/source/private/bufpkg/bufimage/bufimagebuild/builder.go:158 +0x46
panic({0xd00680, 0x1612040})
	/gnu/store/aiw1716ia4qgwsckcls59sw57dwqr3fg-go-1.17.6/lib/go/src/runtime/panic.go:1038 +0x215
github.com/jhump/protoreflect/desc/protoparse.(*linker).createDescriptorPool(0xc02d703720)
	/tmp/guix-build-buf-1.3.1.drv-0/source/go/pkg/mod/github.com/jhump/protoreflect@v1.11.1-0.20220213155251-0c2aedc66cf4/desc/protoparse/linker.go:149 +0x5dd
github.com/jhump/protoreflect/desc/protoparse.(*linker).linkFiles(0xc02d703720)
	/tmp/guix-build-buf-1.3.1.drv-0/source/go/pkg/mod/github.com/jhump/protoreflect@v1.11.1-0.20220213155251-0c2aedc66cf4/desc/protoparse/linker.go:35 +0x33
github.com/jhump/protoreflect/desc/protoparse.Parser.ParseFiles({{0x0, 0x0, 0x0}, 0x0, 0x0, 0x0, 0xc000d0a048, 0x1, 0x0, 0x0, ...}, ...)
	/tmp/guix-build-buf-1.3.1.drv-0/source/go/pkg/mod/github.com/jhump/protoreflect@v1.11.1-0.20220213155251-0c2aedc66cf4/desc/protoparse/parser.go:232 +0x2f6
github.com/bufbuild/buf/private/bufpkg/bufimage/bufimagebuild.getBuildResult({0xf935f0, 0xc0002e85a0}, {0xf9a0a0, 0xc000d09bc0}, {0xc000d90000, 0x0, 0x9a}, 0x0)
	/tmp/guix-build-buf-1.3.1.drv-0/source/source/private/bufpkg/bufimage/bufimagebuild/builder.go:211 +0x250
github.com/bufbuild/buf/private/bufpkg/bufimage/bufimagebuild.getBuildResults.func1()
	/tmp/guix-build-buf-1.3.1.drv-0/source/source/private/bufpkg/bufimage/bufimagebuild/builder.go:162 +0xd1
created by github.com/bufbuild/buf/private/bufpkg/bufimage/bufimagebuild.getBuildResults
	/tmp/guix-build-buf-1.3.1.drv-0/source/source/private/bufpkg/bufimage/bufimagebuild/builder.go:141 +0x7ba
```

The pertinent line for the stack trace is here: https://github.com/jhump/protoreflect/blob/e5b652849f7f16dfb9db81453ec5d2804266750b/desc/protoparse/linker.go#L149

I'm not sure exacty which value is nil. The envoy protobuf repository is pretty complex so it's probably a bit of an Acid test to handle it.

Pull request description:
protoparse: fix panic in linker; handle synthetic oneofs
Resolves #508 

The only case I could find, through code inspection, that could cause the nil dereference described in the bug is related to symbol collisions for synthetic oneofs (which are created on behalf of "proto3 optional" fields). The test case reproduces the panic.

The fix is two-fold:
1. Address the shortcoming related to synthetic oneofs: we need to be able to lookup an AST node for the synthetic oneof, so that we can correctly report its source position.
2. Add a check so if for any other reason we fail to lookup an AST node, we can proceed without panic'ing. Instead of a panic, the error message will fail to include location information. (Strictly better than a panic; if we see such errors reported in the future, we can fix those underlying issues as they are discovered.)


Patch:
diff --git a/desc/builder/message.go b/desc/builder/message.go
index 5956a8a..2f0aa7f 100644
--- a/desc/builder/message.go
+++ b/desc/builder/message.go
@@ -819,7 +819,7 @@ func (mb *MessageBuilder) buildProto(path []int32, sourceInfo *dpb.SourceCodeInf
 	}
 
 	if mb.GetFile().IsProto3 {
-		internal.ProcessProto3OptionalFields(md)
+		internal.ProcessProto3OptionalFields(md, nil)
 	}
 
 	return md, nil
diff --git a/desc/internal/proto3_optional.go b/desc/internal/proto3_optional.go
index 9aa4a3e..ef35cdd 100644
--- a/desc/internal/proto3_optional.go
+++ b/desc/internal/proto3_optional.go
@@ -68,8 +68,9 @@ func SetProto3Optional(fd *dpb.FieldDescriptorProto) {
 
 // ProcessProto3OptionalFields adds synthetic oneofs to the given message descriptor
 // for each proto3 optional field. It also updates the fields to have the correct
-// oneof index reference.
-func ProcessProto3OptionalFields(msgd *dpb.DescriptorProto) {
+// oneof index reference. The given callback, if not nil, is called for each synthetic
+// oneof created.
+func ProcessProto3OptionalFields(msgd *dpb.DescriptorProto, callback func(*dpb.FieldDescriptorProto, *dpb.OneofDescriptorProto)) {
 	var allNames map[string]struct{}
 	for _, fd := range msgd.Field {
 		if GetProto3Optional(fd) {
@@ -79,8 +80,17 @@ func ProcessProto3OptionalFields(msgd *dpb.DescriptorProto) {
 				for _, fd := range msgd.Field {
 					allNames[fd.GetName()] = struct{}{}
 				}
-				for _, fd := range msgd.Extension {
-					allNames[fd.GetName()] = struct{}{}
+				for _, od := range msgd.OneofDecl {
+					allNames[od.GetName()] = struct{}{}
+				}
+				// NB: protoc only considers names of other fields and oneofs
+				// when computing the synthetic oneof name. But that feels like
+				// a bug, since it means it could generate a name that conflicts
+				// with some other symbol defined in the message. If it's decided
+				// that's NOT a bug and is desirable, then we should remove the
+				// following four loops to mimic protoc's behavior.
+				for _, xd := range msgd.Extension {
+					allNames[xd.GetName()] = struct{}{}
 				}
 				for _, ed := range msgd.EnumType {
 					allNames[ed.GetName()] = struct{}{}
@@ -114,7 +124,11 @@ func ProcessProto3OptionalFields(msgd *dpb.DescriptorProto) {
 			}
 
 			fd.OneofIndex = proto.Int32(int32(len(msgd.OneofDecl)))
-			msgd.OneofDecl = append(msgd.OneofDecl, &dpb.OneofDescriptorProto{Name: proto.String(ooName)})
+			ood := &dpb.OneofDescriptorProto{Name: proto.String(ooName)}
+			msgd.OneofDecl = append(msgd.OneofDecl, ood)
+			if callback != nil {
+				callback(fd, ood)
+			}
 		}
 	}
 }
diff --git a/desc/protoparse/ast/field.go b/desc/protoparse/ast/field.go
index b5183af..0d77f27 100644
--- a/desc/protoparse/ast/field.go
+++ b/desc/protoparse/ast/field.go
@@ -8,6 +8,7 @@ import "fmt"
 //  - *FieldNode
 //  - *GroupNode
 //  - *MapFieldNode
+//  - *SyntheticMapField
 // This also allows NoSourceNode to be used in place of one of the above
 // for some usages.
 type FieldDeclNode interface {
@@ -306,6 +307,17 @@ func (n *GroupNode) MessageName() Node {
 	return n.Name
 }
 
+// OneOfDeclNode is a node in the AST that defines a oneof. There are
+// multiple types of AST nodes that declare oneofs:
+//  - *OneOfNode
+//  - *SyntheticOneOf
+// This also allows NoSourceNode to be used in place of one of the above
+// for some usages.
+type OneOfDeclNode interface {
+	Node
+	OneOfName() Node
+}
+
 // OneOfNode represents a one-of declaration. Example:
 //
 //  oneof query {
@@ -373,6 +385,43 @@ func NewOneOfNode(keyword *KeywordNode, name *IdentNode, openBrace *RuneNode, de
 	}
 }
 
+func (n *OneOfNode) OneOfName() Node {
+	return n.Name
+}
+
+// SyntheticOneOf is not an actual node in the AST but a synthetic node
+// that implements OneOfDeclNode. These are used to represent the implicit
+// oneof declarations that enclose "proto3 optional" fields.
+type SyntheticOneOf struct {
+	Field *FieldNode
+}
+
+// NewSyntheticOneOf creates a new *SyntheticOneOf that corresponds to the
+// given proto3 optional field.
+func NewSyntheticOneOf(field *FieldNode) *SyntheticOneOf {
+	return &SyntheticOneOf{Field: field}
+}
+
+func (n *SyntheticOneOf) Start() *SourcePos {
+	return n.Field.Start()
+}
+
+func (n *SyntheticOneOf) End() *SourcePos {
+	return n.Field.End()
+}
+
+func (n *SyntheticOneOf) LeadingComments() []Comment {
+	return nil
+}
+
+func (n *SyntheticOneOf) TrailingComments() []Comment {
+	return nil
+}
+
+func (n *SyntheticOneOf) OneOfName() Node {
+	return n.Field.FieldName()
+}
+
 // OneOfElement is an interface implemented by all AST nodes that can
 // appear in the body of a oneof declaration.
 type OneOfElement interface {
diff --git a/desc/protoparse/descriptor_protos.go b/desc/protoparse/descriptor_protos.go
index eee8cb6..740677d 100644
--- a/desc/protoparse/descriptor_protos.go
+++ b/desc/protoparse/descriptor_protos.go
@@ -477,7 +477,14 @@ func (r *parseResult) addMessageBody(msgd *dpb.DescriptorProto, body *ast.Messag
 
 	// process any proto3_optional fields
 	if isProto3 {
-		internal.ProcessProto3OptionalFields(msgd)
+		callback := func(fd *dpb.FieldDescriptorProto, ood *dpb.OneofDescriptorProto) {
+			node, ok := r.getFieldNode(fd).(*ast.FieldNode)
+			if ok {
+				r.putOneOfNode(ood, ast.NewSyntheticOneOf(node))
+			}
+			// TODO: the !ok case should really never happen... log an error?
+		}
+		internal.ProcessProto3OptionalFields(msgd, callback)
 	}
 }
 
diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go
index 7bc27b0..30e306f 100644
--- a/desc/protoparse/linker.go
+++ b/desc/protoparse/linker.go
@@ -146,6 +146,12 @@ func (l *linker) createDescriptorPool() error {
 					desc1, desc2 = desc2, desc1
 				}
 				node := l.files[file2].getNode(desc2)
+				if node == nil {
+					// TODO: this should never happen, but in case there is a bug where
+					// we get back a nil node, we'd rather fail to report line+column
+					// info than panic with a nil dereference below
+					node = ast.NewNoSourceNode(file2)
+				}
 				if err := l.errs.handleErrorWithPos(node.Start(), "duplicate symbol %s: already defined as %s in %q", k, descriptorType(desc1), file1); err != nil {
 					return err
 				}
diff --git a/desc/protoparse/parser.go b/desc/protoparse/parser.go
index 428a382..817d0f0 100644
--- a/desc/protoparse/parser.go
+++ b/desc/protoparse/parser.go
@@ -739,7 +739,7 @@ func (r *parseResult) putFieldNode(f *dpb.FieldDescriptorProto, n ast.FieldDeclN
 	r.nodes[f] = n
 }
 
-func (r *parseResult) putOneOfNode(o *dpb.OneofDescriptorProto, n *ast.OneOfNode) {
+func (r *parseResult) putOneOfNode(o *dpb.OneofDescriptorProto, n ast.OneOfDeclNode) {
 	r.nodes[o] = n
 }
 


Test patch:
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 5efd5b9..339cb42 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -1033,3 +1033,50 @@ message ReferencesFooOption {
 		t.Errorf("expecting validation error %q; instead got: %q", errs[0].Error(), errMsg)
 	}
 }
+
+func TestSyntheticOneOfCollisions(t *testing.T) {
+	input := map[string]string{
+		"foo1.proto": "syntax = \"proto3\";\n" +
+			"message Foo {\n" +
+			"  optional string bar = 1;\n" +
+			"}\n",
+		"foo2.proto": "syntax = \"proto3\";\n" +
+			"message Foo {\n" +
+			"  optional string bar = 1;\n" +
+			"}\n",
+	}
+	acc := func(filename string) (io.ReadCloser, error) {
+		f, ok := input[filename]
+		if !ok {
+			return nil, fmt.Errorf("file not found: %s", filename)
+		}
+		return ioutil.NopCloser(strings.NewReader(f)), nil
+	}
+
+	var errs []error
+	errReporter := func(errorWithPos ErrorWithPos) error {
+		errs = append(errs, errorWithPos)
+		// need to return nil to accumulate all errors so we can report synthetic
+		// oneof collision; otherwise, the link will fail after the first collision
+		// and we'll never test the synthetic oneofs
+		return nil
+	}
+
+	_, err := Parser{
+		Accessor:      acc,
+		ErrorReporter: errReporter,
+	}.ParseFiles("foo1.proto", "foo2.proto")
+
+	testutil.Eq(t, ErrInvalidSource, err)
+
+	expected := []string{
+		`foo2.proto:2:1: duplicate symbol Foo: already defined as message in "foo1.proto"`,
+		`foo2.proto:3:3: duplicate symbol Foo._bar: already defined as oneof in "foo1.proto"`,
+		`foo2.proto:3:3: duplicate symbol Foo.bar: already defined as field in "foo1.proto"`,
+	}
+	var actual []string
+	for _, err := range errs {
+		actual = append(actual, err.Error())
+	}
+	testutil.Eq(t, actual, expected)
+}