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>

Author: mkruskal-google

reflect/protodesc/desc.go

@@ -152,6 +152,7 @@ func (o FileOptions) New(fd *descriptorpb.FileDescriptorProto, r Resolver) (prot
 		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 @@ func (o FileOptions) New(fd *descriptorpb.FileDescriptorProto, r Resolver) (prot
 			}
 			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

reflect/protodesc/file_test.go

@@ -1016,6 +1016,66 @@ func TestNewFilesImportCycle(t *testing.T) {
 	}
 }
 
+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"