diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index e63a2d9dbfc42..da0c9819e7cdd 100644 --- 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 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 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::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 index 7ecd987c85927..bcf065539efed 100644 --- 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 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 index 55d9988964dd9..1318c5fed3e3d 100644 --- 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> expected_errors, + testing::Matcher> 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 index 41a2080ca4b69..4afd83787d3d3 100644 --- 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 text_lines_; + std::vector 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 expected_errors); + // As above, but with separate errors + void BuildFileWithErrorList( + const FileDescriptorProto& file_proto, + testing::Matcher> expected_errors, + testing::Matcher> 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 index 472ae944cdb3e..9f115838c7702 100644 --- 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 @@ -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\" "