reflect/protodesc: fix handling of import options in dynamic builds
Without this, we allow non-option symbols from import option dependencies to be referenced inside of protos. This can both break valid protos and allow invalid protos in various edge cases, and is inconsistent with protoc.
Change-Id: I12e657435173186b8fe4e84315fc2e1b9fd2dc6a
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/711015
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Stapelberg <stapelberg@google.com>
Commit-Queue: Michael Stapelberg <stapelberg@google.com>
diff --git a/reflect/protodesc/desc.go b/reflect/protodesc/desc.go
index 9196288..f69c78a 100644
--- a/reflect/protodesc/desc.go
+++ b/reflect/protodesc/desc.go
@@ -152,6 +152,7 @@
imp := &f.L2.Imports[i]
imps.importPublic(imp.Imports())
}
+ optionImps := importSet{f.Path(): true}
if len(fd.GetOptionDependency()) > 0 {
optionImports := make(filedesc.FileImports, len(fd.GetOptionDependency()))
for i, path := range fd.GetOptionDependency() {
@@ -165,10 +166,12 @@
}
imp.FileDescriptor = f
- if imps[imp.Path()] {
+ if imps[imp.Path()] || optionImps[imp.Path()] {
return nil, errors.New("already imported %q", path)
}
- imps[imp.Path()] = true
+ // This needs to be a separate map so that we don't recognize non-options
+ // symbols coming from option imports.
+ optionImps[imp.Path()] = true
}
f.L2.OptionImports = func() protoreflect.FileImports {
return &optionImports
diff --git a/reflect/protodesc/file_test.go b/reflect/protodesc/file_test.go
index 8b9fae4..e699d01 100644
--- a/reflect/protodesc/file_test.go
+++ b/reflect/protodesc/file_test.go
@@ -1016,6 +1016,66 @@
}
}
+func TestNewFilesMissingImports(t *testing.T) {
+ tests := []struct {
+ label string
+ files []*descriptorpb.FileDescriptorProto
+ }{
+ {
+ label: "missing import",
+ files: []*descriptorpb.FileDescriptorProto{
+ mustParseFile(`
+ name: "import.proto"
+ message_type {
+ name: "MissingMessage"
+ }
+ `),
+ mustParseFile(`
+ name: "test.proto"
+ message_type: [{
+ name: "M"
+ field: [{name:"field" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:"MissingMessage"}]
+ }]
+ `),
+ },
+ },
+ {
+ label: "missing import via option_dependency",
+ files: []*descriptorpb.FileDescriptorProto{
+ mustParseFile(`
+ name: "import.proto"
+ message_type {
+ name: "MissingMessage"
+ }
+ `),
+ mustParseFile(`
+ name: "test.proto"
+ edition: EDITION_2024
+ option_dependency: "import.proto"
+ message_type: [{
+ name: "M"
+ field: [{name:"field" number:1 label:LABEL_OPTIONAL type:TYPE_MESSAGE type_name:"MissingMessage"}]
+ }]
+ `),
+ },
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.label, func(t *testing.T) {
+ fdset := &descriptorpb.FileDescriptorSet{File: tt.files}
+
+ _, err := NewFiles(fdset)
+
+ if err == nil {
+ t.Fatal("NewFiles with missing import: success, want error")
+ }
+ if !strings.Contains(err.Error(), `cannot resolve`) {
+ t.Fatalf("NewFiles with missing import: got error \"%v\", want import error", err)
+ }
+ })
+ }
+}
+
func TestSourceLocations(t *testing.T) {
fd := mustParseFile(`
name: "comments.proto"
