From 1eb29b8257a3c3e9e40cbcb16b968df7538ab61c Mon Sep 17 00:00:00 2001
From: Josh Humphries <jhumphries@buf.build>
Date: Thu, 15 Sep 2022 10:26:45 -0400
Subject: [PATCH] validate reserved names are identifiers

---
 src/google/protobuf/compiler/parser.cc        | 39 ++++++++++++-------
 src/google/protobuf/compiler/parser.h         |  1 +
 .../protobuf/compiler/parser_unittest.cc      | 17 ++++++++
 3 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/src/google/protobuf/compiler/parser.cc b/src/google/protobuf/compiler/parser.cc
index 4cae1a1187..8158b9724e 100644
--- a/src/google/protobuf/compiler/parser.cc
+++ b/src/google/protobuf/compiler/parser.cc
@@ -51,6 +51,7 @@
 #include "absl/strings/ascii.h"
 #include "absl/strings/escaping.h"
 #include "absl/strings/str_cat.h"
+#include "absl/strings/str_format.h"
 #include "google/protobuf/descriptor.h"
 #include "google/protobuf/descriptor.pb.h"
 #include "google/protobuf/io/tokenizer.h"
@@ -1728,11 +1729,23 @@ bool Parser::ParseReserved(DescriptorProto* message,
   }
 }
 
+bool Parser::ParseReservedName(std::string* name, const char* error_message) {
+  // capture position of token
+  int line = input_->current().line;
+  int col = input_->current().column;
+  DO(ConsumeString(name, error_message));
+  if (!io::Tokenizer::IsIdentifier(*name)) {
+    AddError(line, col, absl::StrFormat("Reserved name \"%s\" is not a valid identifier.", *name));
+    return false;
+  }
+  return true;
+}
+
 bool Parser::ParseReservedNames(DescriptorProto* message,
                                 const LocationRecorder& parent_location) {
   do {
     LocationRecorder location(parent_location, message->reserved_name_size());
-    DO(ConsumeString(message->add_reserved_name(), "Expected field name."));
+    DO(ParseReservedName(message->add_reserved_name(), "Expected field name."));
   } while (TryConsume(","));
   DO(ConsumeEndOfDeclaration(";", &parent_location));
   return true;
@@ -1787,42 +1800,42 @@ bool Parser::ParseReservedNumbers(DescriptorProto* message,
   return true;
 }
 
-bool Parser::ParseReserved(EnumDescriptorProto* message,
-                           const LocationRecorder& message_location) {
+bool Parser::ParseReserved(EnumDescriptorProto* proto,
+                           const LocationRecorder& enum_location) {
   io::Tokenizer::Token start_token = input_->current();
   // Parse the declaration.
   DO(Consume("reserved"));
   if (LookingAtType(io::Tokenizer::TYPE_STRING)) {
-    LocationRecorder location(message_location,
+    LocationRecorder location(enum_location,
                               EnumDescriptorProto::kReservedNameFieldNumber);
     location.StartAt(start_token);
-    return ParseReservedNames(message, location);
+    return ParseReservedNames(proto, location);
   } else {
-    LocationRecorder location(message_location,
+    LocationRecorder location(enum_location,
                               EnumDescriptorProto::kReservedRangeFieldNumber);
     location.StartAt(start_token);
-    return ParseReservedNumbers(message, location);
+    return ParseReservedNumbers(proto, location);
   }
 }
 
-bool Parser::ParseReservedNames(EnumDescriptorProto* message,
+bool Parser::ParseReservedNames(EnumDescriptorProto* proto,
                                 const LocationRecorder& parent_location) {
   do {
-    LocationRecorder location(parent_location, message->reserved_name_size());
-    DO(ConsumeString(message->add_reserved_name(), "Expected enum value."));
+    LocationRecorder location(parent_location, proto->reserved_name_size());
+    DO(ParseReservedName(proto->add_reserved_name(), "Expected enum value."));
   } while (TryConsume(","));
   DO(ConsumeEndOfDeclaration(";", &parent_location));
   return true;
 }
 
-bool Parser::ParseReservedNumbers(EnumDescriptorProto* message,
+bool Parser::ParseReservedNumbers(EnumDescriptorProto* proto,
                                   const LocationRecorder& parent_location) {
   bool first = true;
   do {
-    LocationRecorder location(parent_location, message->reserved_range_size());
+    LocationRecorder location(parent_location, proto->reserved_range_size());
 
     EnumDescriptorProto::EnumReservedRange* range =
-        message->add_reserved_range();
+        proto->add_reserved_range();
     int start, end;
     io::Tokenizer::Token start_token;
     {
diff --git a/src/google/protobuf/compiler/parser.h b/src/google/protobuf/compiler/parser.h
index ccd3e5a5f7..b7dfec6e25 100644
--- a/src/google/protobuf/compiler/parser.h
+++ b/src/google/protobuf/compiler/parser.h
@@ -394,6 +394,7 @@ class PROTOBUF_EXPORT Parser {
                      const LocationRecorder& message_location);
   bool ParseReservedNames(DescriptorProto* message,
                           const LocationRecorder& parent_location);
+  bool ParseReservedName(std::string* name, const char* error_message);
   bool ParseReservedNumbers(DescriptorProto* message,
                             const LocationRecorder& parent_location);
   bool ParseReserved(EnumDescriptorProto* message,
diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc
index 3b32451916..c6dae84494 100644
--- a/src/google/protobuf/compiler/parser_unittest.cc
+++ b/src/google/protobuf/compiler/parser_unittest.cc
@@ -1675,6 +1675,15 @@ TEST_F(ParseErrorTest, EnumReservedMissingQuotes) {
       "2:11: Expected enum value or number range.\n");
 }
 
+TEST_F(ParseErrorTest, EnumReservedInvalidIdentifier) {
+  ExpectHasErrors(
+      "enum TestEnum {\n"
+      "  FOO = 1;\n"
+      "  reserved \"foo bar\";\n"
+      "}\n",
+      "2:11: Reserved name \"foo bar\" is not a valid identifier.\n");
+}
+
 // -------------------------------------------------------------------
 // Reserved field number errors
 
@@ -1702,6 +1711,14 @@ TEST_F(ParseErrorTest, ReservedMissingQuotes) {
       "1:11: Expected field name or number range.\n");
 }
 
+TEST_F(ParseErrorTest, ReservedInvalidIdentifier) {
+  ExpectHasErrors(
+      "message Foo {\n"
+      "  reserved \"foo bar\";\n"
+      "}\n",
+      "1:11: Reserved name \"foo bar\" is not a valid identifier.\n");
+}
+
 TEST_F(ParseErrorTest, ReservedNegativeNumber) {
   ExpectHasErrors(
       "message Foo {\n"