Skip to content

Commit

Permalink
protoc: validate that reserved range start is before end (#13474)
Browse files Browse the repository at this point in the history 
This addresses #13442. This also backfills the tests to add tests for the two checks that were already implemented as well as one for the newly added check.

Finally, this fixes the location information so that positions for reserved ranges are correctly reported. (The previous check that already existed, for enums, failed to show line and column info.)

Closes #13474

COPYBARA_INTEGRATE_REVIEW=#13474 from jhump:jh/validate-msg-reserved-range-order 661adc7
PiperOrigin-RevId: 556979032
  • Loading branch information
@jhump @copybara-github
jhump authored and copybara-github committed Aug 15, 2023
1 parent e8a2d45 commit 3be00f7
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 24 deletions.
9 changes: 6 additions & 3 deletions src/google/protobuf/compiler/parser.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -661,9 +661,8 @@ bool Parser::Parse(io::Tokenizer* input, FileDescriptorProto* file) {
root_location.RecordLegacyLocation(file,
DescriptorPool::ErrorCollector::OTHER);

if (require_syntax_identifier_ || LookingAt("syntax")
|| LookingAt("edition")
) {
if (require_syntax_identifier_ || LookingAt("syntax") ||
LookingAt("edition")) {
if (!ParseSyntaxIdentifier(file, root_location)) {
// Don't attempt to parse the file if we didn't recognize the syntax
// identifier.
Expand Down Expand Up @@ -1853,6 +1852,8 @@ bool Parser::ParseReservedNumbers(DescriptorProto* message,
LocationRecorder location(parent_location, message->reserved_range_size());

DescriptorProto::ReservedRange* range = message->add_reserved_range();
location.RecordLegacyLocation(range,
DescriptorPool::ErrorCollector::NUMBER);
int start, end;
io::Tokenizer::Token start_token;
{
Expand Down Expand Up @@ -1959,6 +1960,8 @@ bool Parser::ParseReservedNumbers(EnumDescriptorProto* proto,
LocationRecorder location(parent_location, proto->reserved_range_size());

EnumDescriptorProto::EnumReservedRange* range = proto->add_reserved_range();
location.RecordLegacyLocation(range,
DescriptorPool::ErrorCollector::NUMBER);
int start, end;
io::Tokenizer::Token start_token;
{
Expand Down
28 changes: 26 additions & 2 deletions src/google/protobuf/compiler/parser_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2135,6 +2135,22 @@ TEST_F(ParserValidationErrorTest, ExtensionRangeNumberError) {
"1:13: Suggested field numbers for Foo: 1\n");
}

TEST_F(ParserValidationErrorTest, ExtensionRangeNumberOrderError) {
ExpectHasValidationErrors(
"message Foo {\n"
" extensions 2 to 1;\n"
"}\n",
"1:13: Extension range end number must be greater than start number.\n");
}

TEST_F(ParserValidationErrorTest, ReservedRangeError) {
ExpectHasValidationErrors(
"message Foo {\n"
" reserved 2 to 1;\n"
"}\n",
"1:11: Reserved range end number must be greater than start number.\n");
}

TEST_F(ParserValidationErrorTest, Proto3ExtensionError) {
ExpectHasValidationErrors(
"syntax = 'proto3';\n"
Expand Down Expand Up @@ -2331,6 +2347,15 @@ TEST_F(ParserValidationErrorTest, EnumValueAliasError) {
"definition. The next available enum value is 2.\n");
}

TEST_F(ParserValidationErrorTest, EnumReservedRangeError) {
ExpectHasValidationErrors(
"enum Foo {\n"
" BAR = 1;\n"
" reserved 2 to 1;\n"
"}\n",
"2:11: Reserved range end number must be greater than start number.\n");
}

TEST_F(ParserValidationErrorTest, ExplicitlyMapEntryError) {
ExpectHasValidationErrors(
"message Foo {\n"
Expand Down Expand Up @@ -3047,7 +3072,7 @@ class SourceInfoTest : public ParserTest {
}
}

return false;
return false;
}

private:
Expand Down Expand Up @@ -4240,7 +4265,6 @@ TEST_F(ParseEditionsTest, FeaturesWithoutEditions) {
}



} // anonymous namespace

} // namespace compiler
Expand Down
40 changes: 21 additions & 19 deletions src/google/protobuf/descriptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ class FlatAllocation {
template <typename U>
bool Destroy() {
if (std::is_trivially_destructible<U>::value) return true;
for (U* it = Begin<U>(), *end = End<U>(); it != end; ++it) {
for (U *it = Begin<U>(), *end = End<U>(); it != end; ++it) {
it->~U();
}
return true;
Expand Down Expand Up @@ -6125,8 +6125,7 @@ void DescriptorBuilder::BuildMessage(const DescriptorProto& proto,
result->reserved_names_ =
alloc.AllocateArray<const std::string*>(reserved_name_count);
for (int i = 0; i < reserved_name_count; ++i) {
result->reserved_names_[i] =
alloc.AllocateStrings(proto.reserved_name(i));
result->reserved_names_[i] = alloc.AllocateStrings(proto.reserved_name(i));
}

// Copy options.
Expand Down Expand Up @@ -6676,6 +6675,10 @@ void DescriptorBuilder::BuildReservedRange(
AddError(parent->full_name(), proto, DescriptorPool::ErrorCollector::NUMBER,
"Reserved numbers must be positive integers.");
}
if (result->start >= result->end) {
AddError(parent->full_name(), proto, DescriptorPool::ErrorCollector::NUMBER,
"Reserved range end number must be greater than start number.");
}
}

void DescriptorBuilder::BuildReservedRange(
Expand Down Expand Up @@ -6824,8 +6827,7 @@ void DescriptorBuilder::BuildEnum(const EnumDescriptorProto& proto,
result->reserved_names_ =
alloc.AllocateArray<const std::string*>(reserved_name_count);
for (int i = 0; i < reserved_name_count; ++i) {
result->reserved_names_[i] =
alloc.AllocateStrings(proto.reserved_name(i));
result->reserved_names_[i] = alloc.AllocateStrings(proto.reserved_name(i));
}

// Copy options.
Expand Down Expand Up @@ -7570,8 +7572,7 @@ void DescriptorBuilder::SuggestFieldNumbers(FileDescriptor* file,
std::vector<Range> used_ordinals;
auto add_ordinal = [&](int ordinal) {
if (ordinal <= 0 || ordinal > FieldDescriptor::kMaxNumber) return;
if (!used_ordinals.empty() &&
ordinal == used_ordinals.back().to) {
if (!used_ordinals.empty() && ordinal == used_ordinals.back().to) {
used_ordinals.back().to = ordinal + 1;
} else {
used_ordinals.push_back({ordinal, ordinal + 1});
Expand Down Expand Up @@ -8784,23 +8785,24 @@ bool DescriptorBuilder::OptionInterpreter::ExamineIfOptionIsSet(
namespace {
// Helpers for method below

template <typename T> std::string ValueOutOfRange(
absl::string_view type_name, absl::string_view option_name) {
return absl::StrFormat(
"Value out of range, %d to %d, for %s option \"%s\".", \
std::numeric_limits<T>::min(), std::numeric_limits<T>::max(),
type_name, option_name);
template <typename T>
std::string ValueOutOfRange(absl::string_view type_name,
absl::string_view option_name) {
return absl::StrFormat("Value out of range, %d to %d, for %s option \"%s\".",
std::numeric_limits<T>::min(),
std::numeric_limits<T>::max(), type_name, option_name);
}

template <typename T> std::string ValueMustBeInt(
absl::string_view type_name, absl::string_view option_name) {
template <typename T>
std::string ValueMustBeInt(absl::string_view type_name,
absl::string_view option_name) {
return absl::StrFormat(
"Value must be integer, from %d to %d, for %s option \"%s\".", \
std::numeric_limits<T>::min(), std::numeric_limits<T>::max(),
type_name, option_name);
"Value must be integer, from %d to %d, for %s option \"%s\".",
std::numeric_limits<T>::min(), std::numeric_limits<T>::max(), type_name,
option_name);
}

} // namespace
} // namespace

bool DescriptorBuilder::OptionInterpreter::SetOptionValue(
const FieldDescriptor* option_field, UnknownFieldSet* unknown_fields) {
Expand Down

0 comments on commit 3be00f7

Please sign in to comment.