From 8c5f3a747bae377c92a780970416945c32780bfd Mon Sep 17 00:00:00 2001
From: Mike Kruskal <mkruskal@google.com>
Date: Wed, 15 May 2024 16:54:33 -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: 634122584
---
 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 23eb8b6dbd7d..ce5e0cc1660f 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();
@@ -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
index 9a83c682f351..97065c55cb4f 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.
@@ -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(