Strip extensions from option imports that are known in CollectExtensions due to polluted pool from protoc parse when used with protoc full + java built in generator.

zhangskz · zhangskz · commit ef3f9ca1959d · 2025-08-14T01:23:17.000Z
Fixes #22918 PiperOrigin-RevId: 794764601
diff --git a/java/core/BUILD.bazel b/java/core/BUILD.bazel
@@ -551,6 +551,7 @@ LITE_TEST_EXCLUSIONS = [
     "src/test/java/com/google/protobuf/FieldPresenceTest.java",
     "src/test/java/com/google/protobuf/ForceFieldBuildersPreRun.java",
     "src/test/java/com/google/protobuf/GeneratedMessageTest.java",
+    "src/test/java/com/google/protobuf/ImportOptionTest.java",
     "src/test/java/com/google/protobuf/LazilyParsedMessageSetTest.java",
     "src/test/java/com/google/protobuf/LazyFieldTest.java",
     "src/test/java/com/google/protobuf/LazyStringEndToEndTest.java",
@@ -640,6 +641,7 @@ junit_tests(
             "src/test/java/com/google/protobuf/UnredactedDebugFormatForTestTest.java",
             "src/test/java/com/google/protobuf/LargeEnumTest.java",
             "src/test/java/com/google/protobuf/LargeEnumLiteTest.java",
+            "src/test/java/com/google/protobuf/ImportOptionTest.java",
             # Excluded in core_tests
             "src/test/java/com/google/protobuf/DecodeUtf8Test.java",
             "src/test/java/com/google/protobuf/IsValidUtf8Test.java",
@@ -692,6 +694,7 @@ junit_tests(
             "src/test/java/com/google/protobuf/FieldPresenceTest.java",
             "src/test/java/com/google/protobuf/LargeEnumTest.java",
             "src/test/java/com/google/protobuf/LargeEnumLiteTest.java",
+            "src/test/java/com/google/protobuf/ImportOptionTest.java",
             # Excluded in core_tests
             "src/test/java/com/google/protobuf/DecodeUtf8Test.java",
             "src/test/java/com/google/protobuf/IsValidUtf8Test.java",
diff --git a/java/core/src/test/java/com/google/protobuf/ImportOptionTest.java b/java/core/src/test/java/com/google/protobuf/ImportOptionTest.java
@@ -0,0 +1,54 @@
+package com.google.protobuf;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.protobuf.Descriptors.Descriptor;
+import com.google.protobuf.Descriptors.FieldDescriptor;
+import com.google.protobuf.Descriptors.FileDescriptor;
+import proto2_unittest.UnittestCustomOptions;
+import proto2_unittest_import_option.TestMessage;
+import proto2_unittest_import_option.UnittestImportOptionProto;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Unit test for import option. */
+@RunWith(JUnit4.class)
+public final class ImportOptionTest {
+
+  @Test
+  public void testImportOption() throws Exception {
+    // Ensure that UnittestCustomOptions is linked in and referenced.
+    FileDescriptor unused = UnittestCustomOptions.getDescriptor();
+
+    FileDescriptor fileDescriptor = UnittestImportOptionProto.getDescriptor();
+    Descriptor messageDescriptor = TestMessage.getDescriptor();
+    FieldDescriptor fieldDescriptor = messageDescriptor.findFieldByName("field1");
+
+    UnknownFieldSet unknownFieldsFile = fileDescriptor.getOptions().getUnknownFields();
+    UnknownFieldSet unknownFieldsMessage = messageDescriptor.getOptions().getUnknownFields();
+    UnknownFieldSet unknownFieldsField = fieldDescriptor.getOptions().getUnknownFields();
+
+    // TODO: Currently linked in options also end up in unknown fields.
+    // TODO: Exclude for open source tests once linked in options are treated
+    // differently, since `option_deps` are treated as `deps` in Bazel 7.
+    // assertThat(fileDescriptor.getOptions().getExtension(UnittestCustomOptions.fileOpt1))
+    //     .isEqualTo(1);
+    // assertThat(messageDescriptor.getOptions().getExtension(UnittestCustomOptions.messageOpt1))
+    //     .isEqualTo(2);
+    // assertThat(fieldDescriptor.getOptions().getExtension(UnittestCustomOptions.fieldOpt1))
+    //     .isEqualTo(3);
+    assertThat(unknownFieldsFile.getField(7736974).getVarintList()).containsExactly(1L);
+    assertThat(unknownFieldsMessage.getField(7739036).getVarintList()).containsExactly(2L);
+    assertThat(unknownFieldsField.getField(7740936).getFixed64List()).containsExactly(3L);
+
+    // Options from import option that are not linked in should be in unknown fields.
+    assertThat(unknownFieldsFile.getField(7736975).getVarintList()).containsExactly(1L);
+    assertThat(unknownFieldsMessage.getField(7739037).getVarintList()).containsExactly(2L);
+    assertThat(unknownFieldsField.getField(7740937).getFixed64List()).containsExactly(3L);
+
+    assertThat(unknownFieldsFile.asMap()).hasSize(2);
+    assertThat(unknownFieldsMessage.asMap()).hasSize(2);
+    assertThat(unknownFieldsField.asMap()).hasSize(2);
+  }
+}
diff --git a/python/build_targets.bzl b/python/build_targets.bzl
@@ -272,9 +272,22 @@ def build_targets(name):
         ],
         strip_prefix = "java/core/src/main/resources",
     )
+
+    internal_copy_files(
+        name = "copied_unittest_custom_options_unlinked_proto_files",
+        srcs = [
+            "//src/google/protobuf:unittest_custom_options_unlinked_proto_srcs",
+        ],
+        strip_prefix = "src",
+    )
+
     internal_py_proto_library(
         name = "test_dependency_proto_py_pb2",
-        srcs = [":copied_cpp_features_test_dependency_proto_files", ":copied_java_features_test_dependency_proto_files"],
+        srcs = [
+            ":copied_cpp_features_test_dependency_proto_files",
+            ":copied_java_features_test_dependency_proto_files",
+            ":copied_unittest_custom_options_unlinked_proto_files",
+        ],
         include = ".",
         default_runtime = "",
         protoc = "//:protoc",
diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel
@@ -984,7 +984,6 @@ filegroup(
         "map_unittest.proto",
         "unittest.proto",
         "unittest_custom_options.proto",
-        "unittest_custom_options_unlinked.proto",
         "unittest_embed_optimize_for.proto",
         "unittest_empty.proto",
         "unittest_enormous_descriptor.proto",
@@ -1013,6 +1012,12 @@ filegroup(
     visibility = ["//:__subpackages__"],
 )
 
+filegroup(
+    name = "unittest_custom_options_unlinked_proto_srcs",
+    srcs = ["unittest_custom_options_unlinked.proto"],
+    visibility = ["//:__subpackages__"],
+)
+
 filegroup(
     name = "unittest_proto_srcs",
     srcs = [
@@ -1104,7 +1109,6 @@ protobuf_test_proto_library(
     deps = [
         ":any_proto",
         ":api_proto",
-        ":cpp_features_proto",
         ":descriptor_proto",
         ":duration_proto",
         ":empty_proto",
@@ -1197,7 +1201,11 @@ cc_proto_library(
 protobuf_test_proto_library(
     name = "generic_test_protos",
     srcs = [":test_proto_srcs"],
-    option_deps = [":cpp_features_proto"],
+    option_deps = [
+        "//:java_features_proto",
+        ":cpp_features_proto",
+        ":unittest_custom_options_unlinked_proto",
+    ],
     strip_import_prefix = "/src",
     visibility = ["//:__subpackages__"],
     deps = [
diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc
@@ -789,6 +789,99 @@ TEST_F(CommandLineInterfaceTest, MultipleInputsWithOptionImport) {
                                     "bar.proto", "Bar");
 }
 
+TEST_F(CommandLineInterfaceTest,
+       ExtensionsPublicImportsTransitiveImportNotAllowed) {
+  CreateTempFile("google/protobuf/descriptor.proto",
+                 google::protobuf::DescriptorProto::descriptor()->file()->DebugString());
+  CreateTempFile("custom_option.proto", R"schema(
+      edition = "2024";
+      package foo;
+      import "google/protobuf/descriptor.proto";
+      extend google.protobuf.FileOptions {
+        int32 transitive_import_file_opt = 9997;
+      }
+      )schema");
+  CreateTempFile("import_public2.proto", R"schema(
+      edition = "2024";
+      package foo;
+      import "google/protobuf/descriptor.proto";
+      import "custom_option.proto";
+      extend google.protobuf.FileOptions {
+        int32 public_import_file_opt = 9998;
+      }
+      )schema");
+  CreateTempFile("import_public1.proto", R"schema(
+      edition = "2024";
+      package foo;
+      import public "import_public1.proto";
+      message MyMessage {}
+      )schema");
+  CreateTempFile("foo.proto", R"schema(
+      edition = "2024";
+      package foo;
+      import "google/protobuf/descriptor.proto";
+      import public "import_public2.proto";
+
+      option (transitive_import_file_opt) = 123;
+      option (public_import_file_opt) = 123;
+      option (file_opt) = 123;
+
+      extend google.protobuf.FileOptions {
+        int32 file_opt = 9999;
+      }
+      )schema");
+  Run("protocol_compiler --java_out=$tmpdir --experimental_editions -I$tmpdir "
+      "foo.proto");
+  ExpectErrorSubstring("Option \"(transitive_import_file_opt)\" unknown.");
+}
+
+TEST_F(CommandLineInterfaceTest,
+       ExtensionsPublicImportsTransitiveOptionImportNotAllowed) {
+  CreateTempFile("google/protobuf/descriptor.proto",
+                 google::protobuf::DescriptorProto::descriptor()->file()->DebugString());
+  CreateTempFile("custom_option.proto", R"schema(
+      edition = "2024";
+      package foo;
+      import "google/protobuf/descriptor.proto";
+      extend google.protobuf.FileOptions {
+        int32 transitive_option_import_file_opt = 9997;
+      }
+      )schema");
+  CreateTempFile("import_public2.proto", R"schema(
+      edition = "2024";
+      package foo;
+      import "google/protobuf/descriptor.proto";
+      import option "custom_option.proto";
+      extend google.protobuf.FileOptions {
+        int32 public_import_file_opt = 9998;
+      }
+      )schema");
+  CreateTempFile("import_public1.proto", R"schema(
+      edition = "2024";
+      package foo;
+      import public "import_public1.proto";
+      message MyMessage {}
+      )schema");
+  CreateTempFile("foo.proto", R"schema(
+      edition = "2024";
+      package foo;
+      import "google/protobuf/descriptor.proto";
+      import public "import_public2.proto";
+
+      option (transitive_option_import_file_opt) = 123;
+      option (public_import_file_opt) = 123;
+      option (file_opt) = 123;
+
+      extend google.protobuf.FileOptions {
+        int32 file_opt = 9999;
+      }
+      )schema");
+  Run("protocol_compiler --java_out=$tmpdir --experimental_editions -I$tmpdir "
+      "foo.proto");
+  ExpectErrorSubstring(
+      "Option \"(transitive_option_import_file_opt)\" unknown.");
+}
+
 
 TEST_F(CommandLineInterfaceTest, MultipleInputsWithImport_DescriptorSetIn) {
   // Test parsing multiple input files with an import of a separate file.
diff --git a/src/google/protobuf/compiler/java/file.cc b/src/google/protobuf/compiler/java/file.cc
@@ -16,6 +16,7 @@
 #include <vector>
 
 #include "absl/container/btree_set.h"
+#include "absl/container/flat_hash_set.h"
 #include "absl/log/absl_check.h"
 #include "absl/log/absl_log.h"
 #include "absl/status/status.h"
@@ -70,11 +71,14 @@ using FieldDescriptorSet =
 // Recursively searches the given message to collect extensions.
 // Returns true if all the extensions can be recognized. The extensions will be
 // appended in to the extensions parameter.
-// Unknown extensions may be present in the case of option imports and will be
-// ignored.
-void CollectExtensions(const Message& message, FieldDescriptorSet* extensions) {
+// Returns false when there are unknown fields, in which case the data in the
+// extensions output parameter is not reliable and should be discarded.
+bool CollectExtensions(const Message& message, FieldDescriptorSet* extensions) {
   const Reflection* reflection = message.GetReflection();
 
+  // There are unknown fields that could be extensions, thus this call fails.
+  if (reflection->GetUnknownFields(message).field_count() > 0) return false;
+
   std::vector<const FieldDescriptor*> fields;
   reflection->ListFields(message, &fields);
 
@@ -89,14 +93,25 @@ void CollectExtensions(const Message& message, FieldDescriptorSet* extensions) {
         for (int j = 0; j < size; j++) {
           const Message& sub_message =
               reflection->GetRepeatedMessage(message, fields[i], j);
-          CollectExtensions(sub_message, extensions);
+          if (!CollectExtensions(sub_message, extensions)) return false;
         }
       } else {
         const Message& sub_message = reflection->GetMessage(message, fields[i]);
-        CollectExtensions(sub_message, extensions);
+        if (!CollectExtensions(sub_message, extensions)) return false;
       }
     }
   }
+
+  return true;
+}
+
+void CollectPublicDependencies(
+    const FileDescriptor* file,
+    absl::flat_hash_set<const FileDescriptor*>* dependencies) {
+  if (file == nullptr || !dependencies->insert(file).second) return;
+  for (int i = 0; file != nullptr && i < file->public_dependency_count(); i++) {
+    CollectPublicDependencies(file->public_dependency(i), dependencies);
+  }
 }
 
 // Finds all extensions for custom options in the given file descriptor with the
@@ -123,6 +138,20 @@ void CollectExtensions(const FileDescriptor& file,
   extensions->clear();
   // Unknown extensions are ok and expected in the case of option imports.
   CollectExtensions(*dynamic_file_proto, extensions);
+
+  // TODO: Remove descriptor pool pollution from protoc full.
+  // Check against dependencies to handle option dependencies polluting pool
+  // from using protoc_full with built-in generators instead of plugins.
+  // Option dependencies and transitive dependencies are not allowed, except in
+  // the case of import public.
+  absl::flat_hash_set<const FileDescriptor*> dependencies;
+  dependencies.insert(&file);
+  for (int i = 0; i < file.dependency_count(); i++) {
+    CollectPublicDependencies(file.dependency(i), &dependencies);
+  }
+  absl::erase_if(*extensions, [&](const FieldDescriptor* fieldDescriptor) {
+    return !dependencies.contains(fieldDescriptor->file());
+  });
 }
 
 // Our static initialization methods can become very, very large.
diff --git a/src/google/protobuf/compiler/java/generator_unittest.cc b/src/google/protobuf/compiler/java/generator_unittest.cc
