Skip to content

Commit ef3f9ca

Browse files
zhangskzzhangskz
committed
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.
Fixes #22918 PiperOrigin-RevId: 794764601
1 parent 694eedd commit ef3f9ca

7 files changed

Lines changed: 337 additions & 9 deletions

File tree

‎java/core/BUILD.bazel‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -551,6 +551,7 @@ LITE_TEST_EXCLUSIONS = [
551551
"src/test/java/com/google/protobuf/FieldPresenceTest.java",
552552
"src/test/java/com/google/protobuf/ForceFieldBuildersPreRun.java",
553553
"src/test/java/com/google/protobuf/GeneratedMessageTest.java",
554+
"src/test/java/com/google/protobuf/ImportOptionTest.java",
554555
"src/test/java/com/google/protobuf/LazilyParsedMessageSetTest.java",
555556
"src/test/java/com/google/protobuf/LazyFieldTest.java",
556557
"src/test/java/com/google/protobuf/LazyStringEndToEndTest.java",
@@ -640,6 +641,7 @@ junit_tests(
640641
"src/test/java/com/google/protobuf/UnredactedDebugFormatForTestTest.java",
641642
"src/test/java/com/google/protobuf/LargeEnumTest.java",
642643
"src/test/java/com/google/protobuf/LargeEnumLiteTest.java",
644+
"src/test/java/com/google/protobuf/ImportOptionTest.java",
643645
# Excluded in core_tests
644646
"src/test/java/com/google/protobuf/DecodeUtf8Test.java",
645647
"src/test/java/com/google/protobuf/IsValidUtf8Test.java",
@@ -692,6 +694,7 @@ junit_tests(
692694
"src/test/java/com/google/protobuf/FieldPresenceTest.java",
693695
"src/test/java/com/google/protobuf/LargeEnumTest.java",
694696
"src/test/java/com/google/protobuf/LargeEnumLiteTest.java",
697+
"src/test/java/com/google/protobuf/ImportOptionTest.java",
695698
# Excluded in core_tests
696699
"src/test/java/com/google/protobuf/DecodeUtf8Test.java",
697700
"src/test/java/com/google/protobuf/IsValidUtf8Test.java",

‎java/core/src/test/java/com/google/protobuf/ImportOptionTest.java‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
package com.google.protobuf;
2+
3+
import static com.google.common.truth.Truth.assertThat;
4+
5+
import com.google.protobuf.Descriptors.Descriptor;
6+
import com.google.protobuf.Descriptors.FieldDescriptor;
7+
import com.google.protobuf.Descriptors.FileDescriptor;
8+
import proto2_unittest.UnittestCustomOptions;
9+
import proto2_unittest_import_option.TestMessage;
10+
import proto2_unittest_import_option.UnittestImportOptionProto;
11+
import org.junit.Test;
12+
import org.junit.runner.RunWith;
13+
import org.junit.runners.JUnit4;
14+
15+
/** Unit test for import option. */
16+
@RunWith(JUnit4.class)
17+
public final class ImportOptionTest {
18+
19+
@Test
20+
public void testImportOption() throws Exception {
21+
// Ensure that UnittestCustomOptions is linked in and referenced.
22+
FileDescriptor unused = UnittestCustomOptions.getDescriptor();
23+
24+
FileDescriptor fileDescriptor = UnittestImportOptionProto.getDescriptor();
25+
Descriptor messageDescriptor = TestMessage.getDescriptor();
26+
FieldDescriptor fieldDescriptor = messageDescriptor.findFieldByName("field1");
27+
28+
UnknownFieldSet unknownFieldsFile = fileDescriptor.getOptions().getUnknownFields();
29+
UnknownFieldSet unknownFieldsMessage = messageDescriptor.getOptions().getUnknownFields();
30+
UnknownFieldSet unknownFieldsField = fieldDescriptor.getOptions().getUnknownFields();
31+
32+
// TODO: Currently linked in options also end up in unknown fields.
33+
// TODO: Exclude for open source tests once linked in options are treated
34+
// differently, since `option_deps` are treated as `deps` in Bazel 7.
35+
// assertThat(fileDescriptor.getOptions().getExtension(UnittestCustomOptions.fileOpt1))
36+
// .isEqualTo(1);
37+
// assertThat(messageDescriptor.getOptions().getExtension(UnittestCustomOptions.messageOpt1))
38+
// .isEqualTo(2);
39+
// assertThat(fieldDescriptor.getOptions().getExtension(UnittestCustomOptions.fieldOpt1))
40+
// .isEqualTo(3);
41+
assertThat(unknownFieldsFile.getField(7736974).getVarintList()).containsExactly(1L);
42+
assertThat(unknownFieldsMessage.getField(7739036).getVarintList()).containsExactly(2L);
43+
assertThat(unknownFieldsField.getField(7740936).getFixed64List()).containsExactly(3L);
44+
45+
// Options from import option that are not linked in should be in unknown fields.
46+
assertThat(unknownFieldsFile.getField(7736975).getVarintList()).containsExactly(1L);
47+
assertThat(unknownFieldsMessage.getField(7739037).getVarintList()).containsExactly(2L);
48+
assertThat(unknownFieldsField.getField(7740937).getFixed64List()).containsExactly(3L);
49+
50+
assertThat(unknownFieldsFile.asMap()).hasSize(2);
51+
assertThat(unknownFieldsMessage.asMap()).hasSize(2);
52+
assertThat(unknownFieldsField.asMap()).hasSize(2);
53+
}
54+
}

‎python/build_targets.bzl‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,9 +272,22 @@ def build_targets(name):
272272
],
273273
strip_prefix = "java/core/src/main/resources",
274274
)
275+
276+
internal_copy_files(
277+
name = "copied_unittest_custom_options_unlinked_proto_files",
278+
srcs = [
279+
"//src/google/protobuf:unittest_custom_options_unlinked_proto_srcs",
280+
],
281+
strip_prefix = "src",
282+
)
283+
275284
internal_py_proto_library(
276285
name = "test_dependency_proto_py_pb2",
277-
srcs = [":copied_cpp_features_test_dependency_proto_files", ":copied_java_features_test_dependency_proto_files"],
286+
srcs = [
287+
":copied_cpp_features_test_dependency_proto_files",
288+
":copied_java_features_test_dependency_proto_files",
289+
":copied_unittest_custom_options_unlinked_proto_files",
290+
],
278291
include = ".",
279292
default_runtime = "",
280293
protoc = "//:protoc",

‎src/google/protobuf/BUILD.bazel‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -984,7 +984,6 @@ filegroup(
984984
"map_unittest.proto",
985985
"unittest.proto",
986986
"unittest_custom_options.proto",
987-
"unittest_custom_options_unlinked.proto",
988987
"unittest_embed_optimize_for.proto",
989988
"unittest_empty.proto",
990989
"unittest_enormous_descriptor.proto",
@@ -1013,6 +1012,12 @@ filegroup(
10131012
visibility = ["//:__subpackages__"],
10141013
)
10151014

1015+
filegroup(
1016+
name = "unittest_custom_options_unlinked_proto_srcs",
1017+
srcs = ["unittest_custom_options_unlinked.proto"],
1018+
visibility = ["//:__subpackages__"],
1019+
)
1020+
10161021
filegroup(
10171022
name = "unittest_proto_srcs",
10181023
srcs = [
@@ -1104,7 +1109,6 @@ protobuf_test_proto_library(
11041109
deps = [
11051110
":any_proto",
11061111
":api_proto",
1107-
":cpp_features_proto",
11081112
":descriptor_proto",
11091113
":duration_proto",
11101114
":empty_proto",
@@ -1197,7 +1201,11 @@ cc_proto_library(
11971201
protobuf_test_proto_library(
11981202
name = "generic_test_protos",
11991203
srcs = [":test_proto_srcs"],
1200-
option_deps = [":cpp_features_proto"],
1204+
option_deps = [
1205+
"//:java_features_proto",
1206+
":cpp_features_proto",
1207+
":unittest_custom_options_unlinked_proto",
1208+
],
12011209
strip_import_prefix = "/src",
12021210
visibility = ["//:__subpackages__"],
12031211
deps = [

‎src/google/protobuf/compiler/command_line_interface_unittest.cc‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -789,6 +789,99 @@ TEST_F(CommandLineInterfaceTest, MultipleInputsWithOptionImport) {
789789
"bar.proto", "Bar");
790790
}
791791

792+
TEST_F(CommandLineInterfaceTest,
793+
ExtensionsPublicImportsTransitiveImportNotAllowed) {
794+
CreateTempFile("google/protobuf/descriptor.proto",
795+
google::protobuf::DescriptorProto::descriptor()->file()->DebugString());
796+
CreateTempFile("custom_option.proto", R"schema(
797+
edition = "2024";
798+
package foo;
799+
import "google/protobuf/descriptor.proto";
800+
extend google.protobuf.FileOptions {
801+
int32 transitive_import_file_opt = 9997;
802+
}
803+
)schema");
804+
CreateTempFile("import_public2.proto", R"schema(
805+
edition = "2024";
806+
package foo;
807+
import "google/protobuf/descriptor.proto";
808+
import "custom_option.proto";
809+
extend google.protobuf.FileOptions {
810+
int32 public_import_file_opt = 9998;
811+
}
812+
)schema");
813+
CreateTempFile("import_public1.proto", R"schema(
814+
edition = "2024";
815+
package foo;
816+
import public "import_public1.proto";
817+
message MyMessage {}
818+
)schema");
819+
CreateTempFile("foo.proto", R"schema(
820+
edition = "2024";
821+
package foo;
822+
import "google/protobuf/descriptor.proto";
823+
import public "import_public2.proto";
824+
825+
option (transitive_import_file_opt) = 123;
826+
option (public_import_file_opt) = 123;
827+
option (file_opt) = 123;
828+
829+
extend google.protobuf.FileOptions {
830+
int32 file_opt = 9999;
831+
}
832+
)schema");
833+
Run("protocol_compiler --java_out=$tmpdir --experimental_editions -I$tmpdir "
834+
"foo.proto");
835+
ExpectErrorSubstring("Option \"(transitive_import_file_opt)\" unknown.");
836+
}
837+
838+
TEST_F(CommandLineInterfaceTest,
839+
ExtensionsPublicImportsTransitiveOptionImportNotAllowed) {
840+
CreateTempFile("google/protobuf/descriptor.proto",
841+
google::protobuf::DescriptorProto::descriptor()->file()->DebugString());
842+
CreateTempFile("custom_option.proto", R"schema(
843+
edition = "2024";
844+
package foo;
845+
import "google/protobuf/descriptor.proto";
846+
extend google.protobuf.FileOptions {
847+
int32 transitive_option_import_file_opt = 9997;
848+
}
849+
)schema");
850+
CreateTempFile("import_public2.proto", R"schema(
851+
edition = "2024";
852+
package foo;
853+
import "google/protobuf/descriptor.proto";
854+
import option "custom_option.proto";
855+
extend google.protobuf.FileOptions {
856+
int32 public_import_file_opt = 9998;
857+
}
858+
)schema");
859+
CreateTempFile("import_public1.proto", R"schema(
860+
edition = "2024";
861+
package foo;
862+
import public "import_public1.proto";
863+
message MyMessage {}
864+
)schema");
865+
CreateTempFile("foo.proto", R"schema(
866+
edition = "2024";
867+
package foo;
868+
import "google/protobuf/descriptor.proto";
869+
import public "import_public2.proto";
870+
871+
option (transitive_option_import_file_opt) = 123;
872+
option (public_import_file_opt) = 123;
873+
option (file_opt) = 123;
874+
875+
extend google.protobuf.FileOptions {
876+
int32 file_opt = 9999;
877+
}
878+
)schema");
879+
Run("protocol_compiler --java_out=$tmpdir --experimental_editions -I$tmpdir "
880+
"foo.proto");
881+
ExpectErrorSubstring(
882+
"Option \"(transitive_option_import_file_opt)\" unknown.");
883+
}
884+
792885

793886
TEST_F(CommandLineInterfaceTest, MultipleInputsWithImport_DescriptorSetIn) {
794887
// Test parsing multiple input files with an import of a separate file.

‎src/google/protobuf/compiler/java/file.cc‎

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include <vector>
1717

1818
#include "absl/container/btree_set.h"
19+
#include "absl/container/flat_hash_set.h"
1920
#include "absl/log/absl_check.h"
2021
#include "absl/log/absl_log.h"
2122
#include "absl/status/status.h"
@@ -70,11 +71,14 @@ using FieldDescriptorSet =
7071
// Recursively searches the given message to collect extensions.
7172
// Returns true if all the extensions can be recognized. The extensions will be
7273
// appended in to the extensions parameter.
73-
// Unknown extensions may be present in the case of option imports and will be
74-
// ignored.
75-
void CollectExtensions(const Message& message, FieldDescriptorSet* extensions) {
74+
// Returns false when there are unknown fields, in which case the data in the
75+
// extensions output parameter is not reliable and should be discarded.
76+
bool CollectExtensions(const Message& message, FieldDescriptorSet* extensions) {
7677
const Reflection* reflection = message.GetReflection();
7778

79+
// There are unknown fields that could be extensions, thus this call fails.
80+
if (reflection->GetUnknownFields(message).field_count() > 0) return false;
81+
7882
std::vector<const FieldDescriptor*> fields;
7983
reflection->ListFields(message, &fields);
8084

@@ -89,14 +93,25 @@ void CollectExtensions(const Message& message, FieldDescriptorSet* extensions) {
8993
for (int j = 0; j < size; j++) {
9094
const Message& sub_message =
9195
reflection->GetRepeatedMessage(message, fields[i], j);
92-
CollectExtensions(sub_message, extensions);
96+
if (!CollectExtensions(sub_message, extensions)) return false;
9397
}
9498
} else {
9599
const Message& sub_message = reflection->GetMessage(message, fields[i]);
96-
CollectExtensions(sub_message, extensions);
100+
if (!CollectExtensions(sub_message, extensions)) return false;
97101
}
98102
}
99103
}
104+
105+
return true;
106+
}
107+
108+
void CollectPublicDependencies(
109+
const FileDescriptor* file,
110+
absl::flat_hash_set<const FileDescriptor*>* dependencies) {
111+
if (file == nullptr || !dependencies->insert(file).second) return;
112+
for (int i = 0; file != nullptr && i < file->public_dependency_count(); i++) {
113+
CollectPublicDependencies(file->public_dependency(i), dependencies);
114+
}
100115
}
101116

102117
// Finds all extensions for custom options in the given file descriptor with the
@@ -123,6 +138,20 @@ void CollectExtensions(const FileDescriptor& file,
123138
extensions->clear();
124139
// Unknown extensions are ok and expected in the case of option imports.
125140
CollectExtensions(*dynamic_file_proto, extensions);
141+
142+
// TODO: Remove descriptor pool pollution from protoc full.
143+
// Check against dependencies to handle option dependencies polluting pool
144+
// from using protoc_full with built-in generators instead of plugins.
145+
// Option dependencies and transitive dependencies are not allowed, except in
146+
// the case of import public.
147+
absl::flat_hash_set<const FileDescriptor*> dependencies;
148+
dependencies.insert(&file);
149+
for (int i = 0; i < file.dependency_count(); i++) {
150+
CollectPublicDependencies(file.dependency(i), &dependencies);
151+
}
152+
absl::erase_if(*extensions, [&](const FieldDescriptor* fieldDescriptor) {
153+
return !dependencies.contains(fieldDescriptor->file());
154+
});
126155
}
127156

128157
// Our static initialization methods can become very, very large.

0 commit comments

Comments
 (0)