Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
copybara-service wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 20 additions & 16 deletions src/google/protobuf/descriptor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_("") {}

Expand All @@ -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_
<< "\":";
}
Expand All @@ -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(
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 "
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand All @@ -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));
}
Expand All @@ -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_)) {
Expand All @@ -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)) {
Expand All @@ -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)
Expand All @@ -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();
Expand All @@ -5912,7 +5916,7 @@ FileDescriptor* internal::DescriptorBuilder::BuildFileImpl(
}
}

if (had_errors_) {
if (has_errors()) {
return nullptr;
} else {
return result;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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();
Expand Down
7 changes: 6 additions & 1 deletion src/google/protobuf/descriptor_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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_;
Expand All @@ -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;
Expand Down
18 changes: 16 additions & 2 deletions src/google/protobuf/descriptor_test_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ---------------------------------------
Expand All @@ -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");
}

// ===================================================================
Expand Down Expand Up @@ -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(
Expand Down
9 changes: 9 additions & 0 deletions src/google/protobuf/descriptor_test_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
39 changes: 39 additions & 0 deletions src/google/protobuf/descriptor_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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\" "
Expand Down
Loading