Merge pull request #16875 from protocolbuffers/ban-recursive-features

Prohibit using features in the same file they're defined in.
protocolbuffers · May 16, 2024 · 0a05aa8 · 0a05aa8
2 parents dda73f0 + 8c5f3a7
commit 0a05aa8
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 16 deletions.
diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc
@@ -1110,19 +1110,6 @@ void RestoreFeaturesToOptions(const FeatureSet* features, ProtoT* proto) {
   }
 }
 
-template <typename OptionsT>
-bool HasFeatures(const OptionsT& options) {
-  if (options.has_features()) return true;
-
-  for (const auto& opt : options.uninterpreted_option()) {
-    if (opt.name_size() > 0 && opt.name(0).name_part() == "features" &&
-        !opt.name(0).is_extension()) {
-      return true;
-    }
-  }
-  return false;
-}
-
 template <typename DescriptorT>
 absl::string_view GetFullName(const DescriptorT& desc) {
   return desc.full_name();
@@ -8774,6 +8761,19 @@ bool DescriptorBuilder::OptionInterpreter::InterpretSingleOption(
       // accumulate field numbers to form path to interpreted option
       dest_path.push_back(field->number());
 
+      // Special handling to prevent feature use in the same file as the
+      // definition.
+      // TODO Add proper support for cases where this can work.
+      if (field->file() == builder_->file_ &&
+          uninterpreted_option_->name(0).name_part() == "features" &&
+          !uninterpreted_option_->name(0).is_extension()) {
+        return AddNameError([&] {
+          return absl::StrCat(
+              "Feature \"", debug_msg_name,
+              "\" can't be used in the same file it's defined in.");
+        });
+      }
+
       if (i < uninterpreted_option_->name_size() - 1) {
         if (field->cpp_type() != FieldDescriptor::CPPTYPE_MESSAGE) {
           return AddNameError([&] {

diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc
@@ -4059,8 +4059,8 @@ class ValidationErrorTest : public testing::Test {
     return ABSL_DIE_IF_NULL(pool_.BuildFile(file_proto));
   }
 
-  const FileDescriptor* ParseAndBuildFile(absl::string_view file_name,
-                                          absl::string_view file_text) {
+  FileDescriptorProto ParseFile(absl::string_view file_name,
+                                absl::string_view file_text) {
     io::ArrayInputStream input_stream(file_text.data(), file_text.size());
     SimpleErrorCollector error_collector;
     io::Tokenizer tokenizer(&input_stream, &error_collector);
@@ -4072,7 +4072,12 @@ class ValidationErrorTest : public testing::Test {
         << file_text;
     ABSL_CHECK_EQ("", error_collector.last_error());
     proto.set_name(file_name);
-    return pool_.BuildFile(proto);
+    return proto;
+  }
+
+  const FileDescriptor* ParseAndBuildFile(absl::string_view file_name,
+                                          absl::string_view file_text) {
+    return pool_.BuildFile(ParseFile(file_name, file_text));
   }
 
 
@@ -4096,6 +4101,17 @@ class ValidationErrorTest : public testing::Test {
     BuildFileWithErrors(file_proto, expected_errors);
   }
 
+  // Parse a proto file and build it.  Expect errors to be produced which match
+  // the given error text.
+  void ParseAndBuildFileWithErrors(absl::string_view file_name,
+                                   absl::string_view file_text,
+                                   absl::string_view expected_errors) {
+    MockErrorCollector error_collector;
+    EXPECT_TRUE(pool_.BuildFileCollectingErrors(ParseFile(file_name, file_text),
+                                                &error_collector) == nullptr);
+    EXPECT_EQ(expected_errors, error_collector.text_);
+  }
+
   // Parse file_text as a FileDescriptorProto in text format and add it
   // to the DescriptorPool.  Expect errors to be produced which match the
   // given warning text.
@@ -10283,6 +10299,44 @@ TEST_F(FeaturesTest, InvalidOpenEnumNonZeroFirstValue) {
       "enums.\n");
 }
 
+TEST_F(FeaturesTest, InvalidUseFeaturesInSameFile) {
+  BuildDescriptorMessagesInTestPool();
+  ParseAndBuildFileWithErrors("foo.proto", R"schema(
+    edition = "2023";
+
+    package test;
+    import "google/protobuf/descriptor.proto";
+
+    message Foo {
+      string bar = 1 [
+        features.(test.custom).foo = "xyz",
+        features.(test.another) = {foo: -321}
+      ];
+    }
+
+    message Custom {
+      string foo = 1 [features = { [test.custom]: {foo: "abc"} }];
+    }
+    message Another {
+      Enum foo = 1;
+    }
+
+    enum Enum {
+      option features.enum_type = CLOSED;
+      ZERO = 0;
+      ONE = 1;
+    }
+
+    extend google.protobuf.FeatureSet {
+      Custom custom = 1002 [features.message_encoding=DELIMITED];
+      Another another = 1001;
+    }
+  )schema",
+                              "foo.proto: test.Foo.bar: OPTION_NAME: Feature "
+                              "\"features.(test.custom)\" can't be used in the "
+                              "same file it's defined in.\n");
+}
+
 TEST_F(FeaturesTest, ClosedEnumNonZeroFirstValue) {
   BuildDescriptorMessagesInTestPool();
   const FileDescriptor* file = BuildFile(