Add a hard limit on the number of errors and warnings we generate.

sbenzaquen · copybara-github · commit 479cd1d6cb52 · 2026-07-23T11:56:47.000-07:00
This prevents pathological inputs from wasting too much time/space on error
output.

PiperOrigin-RevId: 952879265
diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc
@@ -4356,7 +4356,6 @@ internal::DescriptorBuilder::DescriptorBuilder(
       tables_(tables),
       deferred_validation_(deferred_validation),
       error_collector_(error_collector),
-      had_errors_(false),
       possible_undeclared_dependency_(nullptr),
       undefine_resolved_name_("") {}
 
@@ -4366,9 +4365,12 @@ PROTOBUF_NOINLINE void internal::DescriptorBuilder::AddError(
     const absl::string_view element_name, const Message& descriptor,
     DescriptorPool::ErrorCollector::ErrorLocation location,
     absl::FunctionRef<std::string()> make_error) {
+  if (++error_count_ > kMaxNumErrors) {
+    return;
+  }
   std::string error = make_error();
   if (error_collector_ == nullptr) {
-    if (!had_errors_) {
+    if (!has_errors()) {
       ABSL_LOG(ERROR) << "Invalid proto descriptor for file \"" << filename_
                       << "\":";
     }
@@ -4377,7 +4379,6 @@ PROTOBUF_NOINLINE void internal::DescriptorBuilder::AddError(
     error_collector_->RecordError(filename_, element_name, &descriptor,
                                   location, error);
   }
-  had_errors_ = true;
 }
 
 PROTOBUF_NOINLINE void internal::DescriptorBuilder::AddError(
@@ -4426,6 +4427,9 @@ PROTOBUF_NOINLINE void internal::DescriptorBuilder::AddWarning(
     const absl::string_view element_name, const Message& descriptor,
     DescriptorPool::ErrorCollector::ErrorLocation location,
     absl::FunctionRef<std::string()> make_error) {
+  if (++warning_count_ > kMaxNumErrors) {
+    return;
+  }
   std::string error = make_error();
   if (error_collector_ == nullptr) {
     ABSL_LOG(WARNING) << filename_ << " " << element_name << ": " << error;
@@ -4845,7 +4849,7 @@ bool internal::DescriptorBuilder::AddSymbol(const absl::string_view full_name,
     if (!file_tables_->AddAliasUnderParent(parent, name, symbol)) {
       // This is only possible if there was already an error adding something of
       // the same name.
-      if (!had_errors_) {
+      if (!has_errors()) {
         ABSL_DLOG(FATAL) << "\"" << full_name
                          << "\" not previously defined in "
                             "symbols_by_name_, but was defined in "
@@ -5785,7 +5789,7 @@ FileDescriptor* internal::DescriptorBuilder::BuildFileImpl(
   // Interpret only the non-extension options first, including features and
   // their extensions.  This has to be done in two passes, since option
   // extensions defined in this file may have features attached to them.
-  if (!had_errors_) {
+  if (!has_errors()) {
     OptionInterpreter option_interpreter(this);
     for (std::vector<OptionsToInterpret>::iterator iter =
              options_to_interpret_.begin();
@@ -5837,7 +5841,7 @@ FileDescriptor* internal::DescriptorBuilder::BuildFileImpl(
 
   // Validate options. See comments at InternalSetLazilyBuildDependencies about
   // error checking and lazy import building.
-  if (!had_errors_ && !pool_->lazily_build_dependencies_) {
+  if (!has_errors() && !pool_->lazily_build_dependencies_) {
     internal::VisitDescriptors(
         *result, proto, [&](const auto& descriptor, const auto& desc_proto) {
           ValidateOptions(&descriptor, desc_proto);
@@ -5846,7 +5850,7 @@ FileDescriptor* internal::DescriptorBuilder::BuildFileImpl(
 
   // Additional naming conflict check for map entry types. Only need to check
   // this if there are already errors.
-  if (had_errors_) {
+  if (has_errors()) {
     for (int i = 0; i < proto.message_type_size(); ++i) {
       DetectMapConflicts(result->message_type(i), proto.message_type(i));
     }
@@ -5856,14 +5860,14 @@ FileDescriptor* internal::DescriptorBuilder::BuildFileImpl(
   // Again, see comments at InternalSetLazilyBuildDependencies about error
   // checking. Also, don't log unused dependencies if there were previous
   // errors, since the results might be inaccurate.
-  if (!had_errors_ && !unused_dependency_.empty() &&
+  if (!has_errors() && !unused_dependency_.empty() &&
       !pool_->lazily_build_dependencies_) {
     LogUnusedDependency(proto, result);
   }
 
   // Store feature information for deferred validation outside of the database
   // mutex.
-  if (!had_errors_ && !pool_->lazily_build_dependencies_) {
+  if (!has_errors() && !pool_->lazily_build_dependencies_) {
     internal::VisitDescriptors(
         *result, proto, [&](const auto& descriptor, const auto& desc_proto) {
           if (!IsDefaultInstance(*descriptor.proto_features_)) {
@@ -5880,7 +5884,7 @@ FileDescriptor* internal::DescriptorBuilder::BuildFileImpl(
         });
   }
 
-  if (!had_errors_ && pool_->enforce_naming_style_) {
+  if (!has_errors() && pool_->enforce_naming_style_) {
     internal::VisitDescriptors(
         *result, proto, [&](const auto& descriptor, const auto& desc_proto) {
           if (IsStyleOrGreater(&descriptor, FeatureSet::STYLE2024)) {
@@ -5889,7 +5893,7 @@ FileDescriptor* internal::DescriptorBuilder::BuildFileImpl(
         });
   }
 
-  if (!had_errors_ && pool_->enforce_proto_limits_) {
+  if (!has_errors() && pool_->enforce_proto_limits_) {
     internal::VisitDescriptors(
         *result, proto, [&](const auto& descriptor, const auto& desc_proto) {
           if (internal::InternalFeatureHelper::GetFeatures(descriptor)
@@ -5899,7 +5903,7 @@ FileDescriptor* internal::DescriptorBuilder::BuildFileImpl(
           }
         });
   }
-  if (!had_errors_ && pool_->enforce_symbol_visibility_) {
+  if (!has_errors() && pool_->enforce_symbol_visibility_) {
     SymbolChecker symbol_checker(result, proto);
     // Check Symbol Visibility and future co-location Rules.
     auto errors = symbol_checker.CheckSymbolVisibilityRules();
@@ -5912,7 +5916,7 @@ FileDescriptor* internal::DescriptorBuilder::BuildFileImpl(
     }
   }
 
-  if (had_errors_) {
+  if (has_errors()) {
     return nullptr;
   } else {
     return result;
@@ -7057,7 +7061,7 @@ void internal::DescriptorBuilder::CrossLinkMessage(
         out_oneof_decl.fields_ = message->field(i);
       }
 
-      if (!had_errors_) {
+      if (!has_errors()) {
         // Verify that they are contiguous.
         // This is assumed by OneofDescriptor::field(i).
         // But only if there are no errors.
@@ -7118,13 +7122,13 @@ void internal::DescriptorBuilder::CrossLinkMessage(
 void internal::DescriptorBuilder::CheckExtensionDeclarationFieldType(
     const FieldDescriptor& field, const FieldDescriptorProto& proto,
     absl::string_view type) {
-  if (had_errors_) return;
+  if (has_errors()) return;
   std::string actual_type(field.type_name());
   std::string expected_type(type);
   if (field.message_type() || field.enum_type()) {
     // Field message type descriptor can be in a partial state which will cause
     // segmentation fault if it is being accessed.
-    if (had_errors_) return;
+    if (has_errors()) return;
     absl::string_view full_name = field.message_type() != nullptr
                                       ? field.message_type()->full_name()
                                       : field.enum_type()->full_name();
diff --git a/src/google/protobuf/descriptor_builder.h b/src/google/protobuf/descriptor_builder.h
@@ -91,6 +91,8 @@ class DescriptorBuilder {
   const FileDescriptor* BuildFile(const FileDescriptorProto& proto);
 
  private:
+  static constexpr size_t kMaxNumErrors = 1000;
+
   DescriptorBuilder(const DescriptorPool* pool, DescriptorPool::Tables* tables,
                     DescriptorPool::DeferredValidation& deferred_validation,
                     DescriptorPool::ErrorCollector* error_collector);
@@ -101,6 +103,8 @@ class DescriptorBuilder {
   FileDescriptor* BuildFileImpl(const FileDescriptorProto& proto,
                                 internal::FlatAllocator& alloc);
 
+  bool has_errors() const { return error_count_ != 0; }
+
   const DescriptorPool* pool_;
   DescriptorPool::Tables* tables_;  // for convenience
   DescriptorPool::DeferredValidation& deferred_validation_;
@@ -113,7 +117,8 @@ class DescriptorBuilder {
   // can later (after cross-linking) interpret those options.
   std::vector<OptionsToInterpret> options_to_interpret_;
 
-  bool had_errors_;
+  size_t error_count_ = 0;
+  size_t warning_count_ = 0;
   std::string filename_;
   FileDescriptor* file_;
   FileDescriptorTables* file_tables_ = nullptr;
diff --git a/src/google/protobuf/descriptor_test_utils.cc b/src/google/protobuf/descriptor_test_utils.cc
@@ -42,8 +42,10 @@ void MockErrorCollector::RecordError(absl::string_view filename,
                                      const Message* descriptor,
                                      ErrorLocation location,
                                      absl::string_view message) {
-  absl::SubstituteAndAppend(&text_, "$0: $1: $2: $3\n", filename, element_name,
+  std::string& e = text_lines_.emplace_back();
+  absl::SubstituteAndAppend(&e, "$0: $1: $2: $3", filename, element_name,
                             ErrorLocationName(location), message);
+  absl::StrAppend(&text_, e, "\n");
 }
 
 // implements ErrorCollector ---------------------------------------
@@ -52,8 +54,10 @@ void MockErrorCollector::RecordWarning(absl::string_view filename,
                                        const Message* descriptor,
                                        ErrorLocation location,
                                        absl::string_view message) {
-  absl::SubstituteAndAppend(&warning_text_, "$0: $1: $2: $3\n", filename,
+  std::string& w = warn_lines_.emplace_back();
+  absl::SubstituteAndAppend(&warning_text_, "$0: $1: $2: $3", filename,
                             element_name, ErrorLocationName(location), message);
+  absl::StrAppend(&warning_text_, w, "\n");
 }
 
 // ===================================================================
@@ -115,6 +119,16 @@ void ValidationErrorTest::BuildFileWithErrors(
   BuildFileWithErrors(file_proto, expected_errors);
 }
 
+void ValidationErrorTest::BuildFileWithErrorList(
+    const FileDescriptorProto& file_proto,
+    testing::Matcher<std::vector<std::string>> expected_errors,
+    testing::Matcher<std::vector<std::string>> expected_warnings) {
+  MockErrorCollector error_collector;
+  pool_.BuildFileCollectingErrors(file_proto, &error_collector);
+  EXPECT_THAT(error_collector.text_lines_, expected_errors);
+  EXPECT_THAT(error_collector.warn_lines_, expected_warnings);
+}
+
 // Parse a proto file and build it.  Expect errors to be produced which match
 // the given error text.
 void ValidationErrorTest::ParseAndBuildFileWithErrors(
diff --git a/src/google/protobuf/descriptor_test_utils.h b/src/google/protobuf/descriptor_test_utils.h
@@ -35,6 +35,8 @@ class MockErrorCollector : public DescriptorPool::ErrorCollector {
 
   std::string text_;
   std::string warning_text_;
+  std::vector<std::string> text_lines_;
+  std::vector<std::string> warn_lines_;
 
   // implements ErrorCollector ---------------------------------------
   void RecordError(absl::string_view filename, absl::string_view element_name,
@@ -87,6 +89,13 @@ class ValidationErrorTest : public testing::Test {
   void BuildFileWithErrors(const std::string& file_text,
                            testing::Matcher<std::string> expected_errors);
 
+  // As above, but with separate errors
+  void BuildFileWithErrorList(
+      const FileDescriptorProto& file_proto,
+      testing::Matcher<std::vector<std::string>> expected_errors,
+      testing::Matcher<std::vector<std::string>> expected_warnings =
+          testing::_);
+
   // 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,
diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc
@@ -92,12 +92,14 @@ using ::google::protobuf::internal::cpp::HasbitMode;
 using ::google::protobuf::internal::cpp::HasHasbitWithoutProfile;
 using ::google::protobuf::internal::cpp::HasPreservingUnknownEnumSemantics;
 using ::google::protobuf::internal::cpp::Utf8CheckMode;
+using ::testing::_;
 using ::testing::AnyOf;
 using ::testing::AtLeast;
 using ::testing::ElementsAre;
 using ::testing::HasSubstr;
 using ::testing::NotNull;
 using ::testing::Return;
+using ::testing::SizeIs;
 
 absl::Status GetStatus(const absl::Status& s) { return s; }
 template <typename T>
@@ -5544,6 +5546,43 @@ TEST_F(ValidationErrorTest, ReservedRangeOverlap) {
       " overlaps with already-defined range 10 to 19.\n");
 }
 
+TEST_F(ValidationErrorTest, LimitNumberOfErrors) {
+  FileDescriptorProto file;
+  file.set_name("foo.proto");
+  auto* m = file.add_message_type();
+  m->set_name("Foo");
+  // This would generate O(N^2) errors
+  for (int i = 0; i < 100; ++i) {
+    auto* r = m->add_reserved_range();
+    r->set_start(100);
+    r->set_end(200);
+  }
+  // DescriptorBuilder::kMaxNumErrors
+  BuildFileWithErrorList(file, SizeIs(1000));
+}
+
+TEST_F(ValidationErrorTest, LimitNumberOfWarnings) {
+  constexpr int N = 1100;
+  // Create N deps.
+  for (int i = 0; i < N; ++i) {
+    FileDescriptorProto file;
+    file.set_name(absl::StrCat("dep", i, ".proto"));
+    ASSERT_TRUE(pool_.BuildFile(file));
+  }
+
+  FileDescriptorProto file;
+  file.set_name("foo.proto");
+
+  // This generates one warning per dep.
+  for (int i = 0; i < N; ++i) {
+    file.add_dependency(absl::StrCat("dep", i, ".proto"));
+  }
+
+  pool_.AddDirectInputFile(file.name());
+  // DescriptorBuilder::kMaxNumErrors
+  BuildFileWithErrorList(file, _, SizeIs(1000));
+}
+
 TEST_F(ValidationErrorTest, ReservedNameError) {
   BuildFileWithErrors(
       "name: \"foo.proto\" "
