From 24b91a7fec2fae2ee633c6bd2600ec2ebbe72f3f Mon Sep 17 00:00:00 2001
From: Mike Kruskal <mkruskal@google.com>
Date: Thu, 16 May 2024 13:33:50 -0700
Subject: [PATCH] Prohibit using features in the same file they're defined in.

This is an edge case we can't handle properly today.  Rather than allowing poorly defined behavior, we'll make this an error condition until we can actually support it.  In the future, it may be necessary to upgrade feature files to newer editions.

Closes #16756

PiperOrigin-RevId: 634512378
---
 src/google/protobuf/descriptor.cc          | 26 +++++-----
 src/google/protobuf/descriptor_unittest.cc | 60 ++++++++++++++++++++--
 2 files changed, 70 insertions(+), 16 deletions(-)

diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc
index 7264daff6a42..f0eb24f85043 100644
--- 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();
@@ -8784,6 +8771,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
index 0c173c97d37b..e82185c48959 100644
--- 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.
@@ -10354,6 +10370,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(