From 8f5929230d5c5d117b84953463ef0da6d29ad36d Mon Sep 17 00:00:00 2001
From: Matt Kulukundis <kfm@google.com>
Date: Tue, 15 Aug 2023 11:43:00 -0700
Subject: [PATCH] protoc: parser rejects explicit use of map_entry option
 (#13479)

This addresses #13441.  Second try, now with more internal fixes.

This preserves the similar check at the _point of use_ of invalid messages in `DescriptorBuilder` (and there's an existing test that verifies that check still works).

But it adds another check in the parser, to catch this error at the _point of definition_ of an invalid message. And the corresponding test is updated: we no longer need a usage of the message to catch the error, and the reported position is the definition of the option, not the usage site of the message.

The way this works feels kinda gross, but I wasn't sure of a better way to do it. The only place we know for certain that it was an explicit option (vs. auto-added by the parser when synthesizing a map entry message) is when after processing the message body, we can look at the uninterpreted options. So that's what this does. If you have ideas on better/cleaner approaches, I'd be happy to revise.

Closes #13479

PiperOrigin-RevId: 557199190
---
 src/google/protobuf/compiler/parser.cc        | 24 +++++++++++++++++++
 src/google/protobuf/compiler/parser.h         |  1 +
 .../protobuf/compiler/parser_unittest.cc      |  7 +++---
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/google/protobuf/compiler/parser.cc b/src/google/protobuf/compiler/parser.cc
index 7ff4cb2a8aec..6620085c79f4 100644
--- a/src/google/protobuf/compiler/parser.cc
+++ b/src/google/protobuf/compiler/parser.cc
@@ -571,6 +571,26 @@ void Parser::SkipRestOfBlock() {
 
 // ===================================================================
 
+bool Parser::ValidateMessage(const DescriptorProto* proto) {
+  for (int i = 0; i < proto->options().uninterpreted_option_size(); i++) {
+    const UninterpretedOption& option =
+        proto->options().uninterpreted_option(i);
+    if (option.name_size() > 0 && !option.name(0).is_extension() &&
+        option.name(0).name_part() == "map_entry") {
+      int line = -1, col = 0;  // indicates line and column not known
+      if (source_location_table_ != nullptr) {
+        source_location_table_->Find(
+            &option, DescriptorPool::ErrorCollector::OPTION_NAME, &line, &col);
+      }
+      RecordError(line, col,
+                  "map_entry should not be set explicitly. "
+                  "Use map<KeyType, ValueType> instead.");
+      return false;
+    }
+  }
+  return true;
+}
+
 bool Parser::ValidateEnum(const EnumDescriptorProto* proto) {
   bool has_allow_alias = false;
   bool allow_alias = false;
@@ -866,6 +886,7 @@ bool IsMessageSetWireFormatMessage(const DescriptorProto& message) {
   for (int i = 0; i < options.uninterpreted_option_size(); ++i) {
     const UninterpretedOption& uninterpreted = options.uninterpreted_option(i);
     if (uninterpreted.name_size() == 1 &&
+        !uninterpreted.name(0).is_extension() &&
         uninterpreted.name(0).name_part() == "message_set_wire_format" &&
         uninterpreted.identifier_value() == "true") {
       return true;
@@ -930,6 +951,9 @@ bool Parser::ParseMessageBlock(DescriptorProto* message,
   if (message->reserved_range_size() > 0) {
     AdjustReservedRangesWithMaxEndNumber(message);
   }
+
+  DO(ValidateMessage(message));
+
   return true;
 }
 
diff --git a/src/google/protobuf/compiler/parser.h b/src/google/protobuf/compiler/parser.h
index 4808be3b7353..04028c192ad0 100644
--- a/src/google/protobuf/compiler/parser.h
+++ b/src/google/protobuf/compiler/parser.h
@@ -540,6 +540,7 @@ class PROTOBUF_EXPORT Parser {
     return syntax_identifier_ == "proto3";
   }
 
+  bool ValidateMessage(const DescriptorProto* proto);
   bool ValidateEnum(const EnumDescriptorProto* proto);
 
   // =================================================================
diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc
index 4c9763b2aad6..31a0c78b822b 100644
--- a/src/google/protobuf/compiler/parser_unittest.cc
+++ b/src/google/protobuf/compiler/parser_unittest.cc
@@ -171,6 +171,8 @@ class ParserTest : public testing::Test {
   // input.
   void ExpectHasEarlyExitErrors(const char* text, const char* expected_errors) {
     SetupParser(text);
+    SourceLocationTable source_locations;
+    parser_->RecordSourceLocationsTo(&source_locations);
     FileDescriptorProto file;
     EXPECT_FALSE(parser_->Parse(input_.get(), &file));
     EXPECT_EQ(expected_errors, error_collector_.text_);
@@ -2357,7 +2359,7 @@ TEST_F(ParserValidationErrorTest, EnumReservedRangeError) {
 }
 
 TEST_F(ParserValidationErrorTest, ExplicitlyMapEntryError) {
-  ExpectHasValidationErrors(
+  ExpectHasErrors(
       "message Foo {\n"
       "  message ValueEntry {\n"
       "    option map_entry = true;\n"
@@ -2365,9 +2367,8 @@ TEST_F(ParserValidationErrorTest, ExplicitlyMapEntryError) {
       "    optional int32 value = 2;\n"
       "    extensions 99 to 999;\n"
       "  }\n"
-      "  repeated ValueEntry value = 1;\n"
       "}",
-      "7:11: map_entry should not be set explicitly. Use "
+      "2:11: map_entry should not be set explicitly. Use "
       "map<KeyType, ValueType> instead.\n");
 }