From 2270d3f93ce3040380fa470807ce60db28c1339f Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Tue, 13 Sep 2022 16:17:10 -0400 Subject: [PATCH 01/11] allow excessively large int literal values (that would otherwise overflow uint64 or underflow int64) to be used as float/double values --- src/google/protobuf/compiler/parser.cc | 43 +++++++++++++++++--------- src/google/protobuf/compiler/parser.h | 3 ++ 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/google/protobuf/compiler/parser.cc b/src/google/protobuf/compiler/parser.cc index 4cae1a11874c..e5f42c45066f 100644 --- a/src/google/protobuf/compiler/parser.cc +++ b/src/google/protobuf/compiler/parser.cc @@ -288,6 +288,16 @@ bool Parser::ConsumeInteger64(uint64_t max_value, uint64_t* output, } } +bool Parser::TryConsumeInteger64(uint64_t max_value, uint64_t* output) { + if (LookingAtType(io::Tokenizer::TYPE_INTEGER) && + io::Tokenizer::ParseInteger(input_->current().text, max_value, + output)) { + input_->Next(); + return true; + } + return false; +} + bool Parser::ConsumeNumber(double* output, const char* error) { if (LookingAtType(io::Tokenizer::TYPE_FLOAT)) { *output = io::Tokenizer::ParseFloat(input_->current().text); @@ -296,13 +306,14 @@ bool Parser::ConsumeNumber(double* output, const char* error) { } else if (LookingAtType(io::Tokenizer::TYPE_INTEGER)) { // Also accept integers. uint64_t value = 0; - if (!io::Tokenizer::ParseInteger(input_->current().text, + if (io::Tokenizer::ParseInteger(input_->current().text, std::numeric_limits::max(), &value)) { - AddError("Integer out of range."); - // We still return true because we did, in fact, parse a number. + *output = value; + } else { + // out of int range, treat as double literal + *output = io::Tokenizer::ParseFloat(input_->current().text); } - *output = value; input_->Next(); return true; } else if (LookingAt("inf")) { @@ -1551,18 +1562,20 @@ bool Parser::ParseOption(Message* options, is_negative ? static_cast(std::numeric_limits::max()) + 1 : std::numeric_limits::max(); - DO(ConsumeInteger64(max_value, &value, "Expected integer.")); - if (is_negative) { - value_location.AddPath( - UninterpretedOption::kNegativeIntValueFieldNumber); - uninterpreted_option->set_negative_int_value( - static_cast(0 - value)); - } else { - value_location.AddPath( - UninterpretedOption::kPositiveIntValueFieldNumber); - uninterpreted_option->set_positive_int_value(value); + if (TryConsumeInteger64(max_value, &value)) { + if (is_negative) { + value_location.AddPath( + UninterpretedOption::kNegativeIntValueFieldNumber); + uninterpreted_option->set_negative_int_value( + static_cast(0 - value)); + } else { + value_location.AddPath( + UninterpretedOption::kPositiveIntValueFieldNumber); + uninterpreted_option->set_positive_int_value(value); + } + break; } - break; + // value too large for an integer; fall through below to treat as floating point } case io::Tokenizer::TYPE_FLOAT: { diff --git a/src/google/protobuf/compiler/parser.h b/src/google/protobuf/compiler/parser.h index ccd3e5a5f778..a5649fd5878a 100644 --- a/src/google/protobuf/compiler/parser.h +++ b/src/google/protobuf/compiler/parser.h @@ -180,6 +180,9 @@ class PROTOBUF_EXPORT Parser { // is greater than max_value, an error will be reported. bool ConsumeInteger64(uint64_t max_value, uint64_t* output, const char* error); + // Try to consume a 64-bit integer and store its value in "output". No + // error is reported on failure, allowing caller to consume token another way. + bool TryConsumeInteger64(uint64_t max_value, uint64_t* output); // Consume a number and store its value in "output". This will accept // tokens of either INTEGER or FLOAT type. bool ConsumeNumber(double* output, const char* error); From 87f24e47595ef5e382949346e50a81150d35c230 Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Tue, 13 Sep 2022 16:32:48 -0400 Subject: [PATCH 02/11] add allowed ranges to error messages --- src/google/protobuf/descriptor.cc | 18 ++++++++---------- src/google/protobuf/descriptor_unittest.cc | 16 ++++++++-------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index ddebcf618415..0e43bc3af6fc 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -7683,7 +7683,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( if (uninterpreted_option_->has_positive_int_value()) { if (uninterpreted_option_->positive_int_value() > static_cast(std::numeric_limits::max())) { - return AddValueError("Value out of range for int32 option \"" + + return AddValueError("Value out of range, -2,147,483,648 to 2,147,483,647, for int32 option \"" + option_field->full_name() + "\"."); } else { SetInt32(option_field->number(), @@ -7693,7 +7693,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( } else if (uninterpreted_option_->has_negative_int_value()) { if (uninterpreted_option_->negative_int_value() < static_cast(std::numeric_limits::min())) { - return AddValueError("Value out of range for int32 option \"" + + return AddValueError("Value out of range, -2,147,483,648 to 2,147,483,647, for int32 option \"" + option_field->full_name() + "\"."); } else { SetInt32(option_field->number(), @@ -7701,7 +7701,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( option_field->type(), unknown_fields); } } else { - return AddValueError("Value must be integer for int32 option \"" + + return AddValueError("Value must be integer, from -2,147,483,648 to 2,147,483,647, for int32 option \"" + option_field->full_name() + "\"."); } break; @@ -7710,7 +7710,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( if (uninterpreted_option_->has_positive_int_value()) { if (uninterpreted_option_->positive_int_value() > static_cast(std::numeric_limits::max())) { - return AddValueError("Value out of range for int64 option \"" + + return AddValueError("Value out of range, -9,223,372,036,854,775,808 to 9,223,372,036,854,775,807, for int64 option \"" + option_field->full_name() + "\"."); } else { SetInt64(option_field->number(), @@ -7722,7 +7722,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( uninterpreted_option_->negative_int_value(), option_field->type(), unknown_fields); } else { - return AddValueError("Value must be integer for int64 option \"" + + return AddValueError("Value must be integer, from -9,223,372,036,854,775,808 to 9,223,372,036,854,775,807, for int64 option \"" + option_field->full_name() + "\"."); } break; @@ -7731,7 +7731,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( if (uninterpreted_option_->has_positive_int_value()) { if (uninterpreted_option_->positive_int_value() > std::numeric_limits::max()) { - return AddValueError("Value out of range for uint32 option \"" + + return AddValueError("Value out of range, 0 to 4,294,967,295, for uint32 option \"" + option_field->name() + "\"."); } else { SetUInt32(option_field->number(), @@ -7740,8 +7740,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( } } else { return AddValueError( - "Value must be non-negative integer for uint32 " - "option \"" + + "Value must be integer, from 0 to 4,294,967,295, for uint32 option \"" + option_field->full_name() + "\"."); } break; @@ -7753,8 +7752,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( option_field->type(), unknown_fields); } else { return AddValueError( - "Value must be non-negative integer for uint64 " - "option \"" + + "Value must be integer, from 0 to 18,446,744,073,709,551,615, for uint64 option \"" + option_field->full_name() + "\"."); } break; diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index a2900454f646..33d4299fb647 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -5557,7 +5557,7 @@ TEST_F(ValidationErrorTest, Int32OptionValueOutOfPositiveRange) { " positive_int_value: 0x80000000 } " "}", - "foo.proto: foo.proto: OPTION_VALUE: Value out of range " + "foo.proto: foo.proto: OPTION_VALUE: Value out of range, -2,147,483,648 to 2,147,483,647, " "for int32 option \"foo\".\n"); } @@ -5574,7 +5574,7 @@ TEST_F(ValidationErrorTest, Int32OptionValueOutOfNegativeRange) { " negative_int_value: -0x80000001 } " "}", - "foo.proto: foo.proto: OPTION_VALUE: Value out of range " + "foo.proto: foo.proto: OPTION_VALUE: Value out of range, -2,147,483,648 to 2,147,483,647, " "for int32 option \"foo\".\n"); } @@ -5590,7 +5590,7 @@ TEST_F(ValidationErrorTest, Int32OptionValueIsNotPositiveInt) { " is_extension: true } " " string_value: \"5\" } }", - "foo.proto: foo.proto: OPTION_VALUE: Value must be integer " + "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from -2,147,483,648 to 2,147,483,647, " "for int32 option \"foo\".\n"); } @@ -5608,7 +5608,7 @@ TEST_F(ValidationErrorTest, Int64OptionValueOutOfRange) { "} " "}", - "foo.proto: foo.proto: OPTION_VALUE: Value out of range " + "foo.proto: foo.proto: OPTION_VALUE: Value out of range, -9,223,372,036,854,775,808 to 9,223,372,036,854,775,807, " "for int64 option \"foo\".\n"); } @@ -5624,7 +5624,7 @@ TEST_F(ValidationErrorTest, Int64OptionValueIsNotPositiveInt) { " is_extension: true } " " identifier_value: \"5\" } }", - "foo.proto: foo.proto: OPTION_VALUE: Value must be integer " + "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from -9,223,372,036,854,775,808 to 9,223,372,036,854,775,807, " "for int64 option \"foo\".\n"); } @@ -5640,7 +5640,7 @@ TEST_F(ValidationErrorTest, UInt32OptionValueOutOfRange) { " is_extension: true } " " positive_int_value: 0x100000000 } }", - "foo.proto: foo.proto: OPTION_VALUE: Value out of range " + "foo.proto: foo.proto: OPTION_VALUE: Value out of range, 0 to 4,294,967,295, " "for uint32 option \"foo\".\n"); } @@ -5656,7 +5656,7 @@ TEST_F(ValidationErrorTest, UInt32OptionValueIsNotPositiveInt) { " is_extension: true } " " double_value: -5.6 } }", - "foo.proto: foo.proto: OPTION_VALUE: Value must be non-negative integer " + "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from 0 to 4,294,967,295, " "for uint32 option \"foo\".\n"); } @@ -5672,7 +5672,7 @@ TEST_F(ValidationErrorTest, UInt64OptionValueIsNotPositiveInt) { " is_extension: true } " " negative_int_value: -5 } }", - "foo.proto: foo.proto: OPTION_VALUE: Value must be non-negative integer " + "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from 0 to 18,446,744,073,709,551,615, " "for uint64 option \"foo\".\n"); } From 4e54ec20d12aaf6868638ceaab4d1c8c17f4d4e8 Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Wed, 14 Sep 2022 15:16:51 -0400 Subject: [PATCH 03/11] change format of int range in error message; use macro to make DRY --- src/google/protobuf/descriptor.cc | 35 +++++++++++----------- src/google/protobuf/descriptor_unittest.cc | 16 +++++----- 2 files changed, 25 insertions(+), 26 deletions(-) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 0e43bc3af6fc..cd40584b374a 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -58,6 +58,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/stubs/stringprintf.h" #include "absl/strings/str_join.h" #include "absl/strings/str_split.h" @@ -7675,6 +7676,14 @@ bool DescriptorBuilder::OptionInterpreter::ExamineIfOptionIsSet( return true; } +#define VALUE_OUT_OF_RANGE(T, NAME) absl::StrFormat( \ + "Value out of range, %d to %d, for " #T " option \"%s\".", \ + std::numeric_limits::min(), std::numeric_limits::max(), NAME) + +#define VALUE_MUST_BE_INT(T, NAME) absl::StrFormat( \ + "Value must be integer, from %d to %d, for " #T " option \"%s\".", \ + std::numeric_limits::min(), std::numeric_limits::max(), NAME) + bool DescriptorBuilder::OptionInterpreter::SetOptionValue( const FieldDescriptor* option_field, UnknownFieldSet* unknown_fields) { // We switch on the CppType to validate. @@ -7683,8 +7692,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( if (uninterpreted_option_->has_positive_int_value()) { if (uninterpreted_option_->positive_int_value() > static_cast(std::numeric_limits::max())) { - return AddValueError("Value out of range, -2,147,483,648 to 2,147,483,647, for int32 option \"" + - option_field->full_name() + "\"."); + return AddValueError(VALUE_OUT_OF_RANGE(int32, option_field->full_name())); } else { SetInt32(option_field->number(), uninterpreted_option_->positive_int_value(), @@ -7693,16 +7701,14 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( } else if (uninterpreted_option_->has_negative_int_value()) { if (uninterpreted_option_->negative_int_value() < static_cast(std::numeric_limits::min())) { - return AddValueError("Value out of range, -2,147,483,648 to 2,147,483,647, for int32 option \"" + - option_field->full_name() + "\"."); + return AddValueError(VALUE_OUT_OF_RANGE(int32, option_field->full_name())); } else { SetInt32(option_field->number(), uninterpreted_option_->negative_int_value(), option_field->type(), unknown_fields); } } else { - return AddValueError("Value must be integer, from -2,147,483,648 to 2,147,483,647, for int32 option \"" + - option_field->full_name() + "\"."); + return AddValueError(VALUE_MUST_BE_INT(int32, option_field->full_name())); } break; @@ -7710,8 +7716,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( if (uninterpreted_option_->has_positive_int_value()) { if (uninterpreted_option_->positive_int_value() > static_cast(std::numeric_limits::max())) { - return AddValueError("Value out of range, -9,223,372,036,854,775,808 to 9,223,372,036,854,775,807, for int64 option \"" + - option_field->full_name() + "\"."); + return AddValueError(VALUE_OUT_OF_RANGE(int64, option_field->full_name())); } else { SetInt64(option_field->number(), uninterpreted_option_->positive_int_value(), @@ -7722,8 +7727,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( uninterpreted_option_->negative_int_value(), option_field->type(), unknown_fields); } else { - return AddValueError("Value must be integer, from -9,223,372,036,854,775,808 to 9,223,372,036,854,775,807, for int64 option \"" + - option_field->full_name() + "\"."); + return AddValueError(VALUE_MUST_BE_INT(int64, option_field->full_name())); } break; @@ -7731,17 +7735,14 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( if (uninterpreted_option_->has_positive_int_value()) { if (uninterpreted_option_->positive_int_value() > std::numeric_limits::max()) { - return AddValueError("Value out of range, 0 to 4,294,967,295, for uint32 option \"" + - option_field->name() + "\"."); + return AddValueError(VALUE_OUT_OF_RANGE(uint32, option_field->full_name())); } else { SetUInt32(option_field->number(), uninterpreted_option_->positive_int_value(), option_field->type(), unknown_fields); } } else { - return AddValueError( - "Value must be integer, from 0 to 4,294,967,295, for uint32 option \"" + - option_field->full_name() + "\"."); + return AddValueError(VALUE_MUST_BE_INT(uint32, option_field->full_name())); } break; @@ -7751,9 +7752,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( uninterpreted_option_->positive_int_value(), option_field->type(), unknown_fields); } else { - return AddValueError( - "Value must be integer, from 0 to 18,446,744,073,709,551,615, for uint64 option \"" + - option_field->full_name() + "\"."); + return AddValueError(VALUE_MUST_BE_INT(uint64, option_field->full_name())); } break; diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index 33d4299fb647..8079a2689511 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -5557,7 +5557,7 @@ TEST_F(ValidationErrorTest, Int32OptionValueOutOfPositiveRange) { " positive_int_value: 0x80000000 } " "}", - "foo.proto: foo.proto: OPTION_VALUE: Value out of range, -2,147,483,648 to 2,147,483,647, " + "foo.proto: foo.proto: OPTION_VALUE: Value out of range, -2147483648 to 2147483647, " "for int32 option \"foo\".\n"); } @@ -5574,7 +5574,7 @@ TEST_F(ValidationErrorTest, Int32OptionValueOutOfNegativeRange) { " negative_int_value: -0x80000001 } " "}", - "foo.proto: foo.proto: OPTION_VALUE: Value out of range, -2,147,483,648 to 2,147,483,647, " + "foo.proto: foo.proto: OPTION_VALUE: Value out of range, -2147483648 to 2147483647, " "for int32 option \"foo\".\n"); } @@ -5590,7 +5590,7 @@ TEST_F(ValidationErrorTest, Int32OptionValueIsNotPositiveInt) { " is_extension: true } " " string_value: \"5\" } }", - "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from -2,147,483,648 to 2,147,483,647, " + "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from -2147483648 to 2147483647, " "for int32 option \"foo\".\n"); } @@ -5608,7 +5608,7 @@ TEST_F(ValidationErrorTest, Int64OptionValueOutOfRange) { "} " "}", - "foo.proto: foo.proto: OPTION_VALUE: Value out of range, -9,223,372,036,854,775,808 to 9,223,372,036,854,775,807, " + "foo.proto: foo.proto: OPTION_VALUE: Value out of range, -9223372036854775808 to 9223372036854775807, " "for int64 option \"foo\".\n"); } @@ -5624,7 +5624,7 @@ TEST_F(ValidationErrorTest, Int64OptionValueIsNotPositiveInt) { " is_extension: true } " " identifier_value: \"5\" } }", - "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from -9,223,372,036,854,775,808 to 9,223,372,036,854,775,807, " + "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from -9223372036854775808 to 9223372036854775807, " "for int64 option \"foo\".\n"); } @@ -5640,7 +5640,7 @@ TEST_F(ValidationErrorTest, UInt32OptionValueOutOfRange) { " is_extension: true } " " positive_int_value: 0x100000000 } }", - "foo.proto: foo.proto: OPTION_VALUE: Value out of range, 0 to 4,294,967,295, " + "foo.proto: foo.proto: OPTION_VALUE: Value out of range, 0 to 4294967295, " "for uint32 option \"foo\".\n"); } @@ -5656,7 +5656,7 @@ TEST_F(ValidationErrorTest, UInt32OptionValueIsNotPositiveInt) { " is_extension: true } " " double_value: -5.6 } }", - "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from 0 to 4,294,967,295, " + "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from 0 to 4294967295, " "for uint32 option \"foo\".\n"); } @@ -5672,7 +5672,7 @@ TEST_F(ValidationErrorTest, UInt64OptionValueIsNotPositiveInt) { " is_extension: true } " " negative_int_value: -5 } }", - "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from 0 to 18,446,744,073,709,551,615, " + "foo.proto: foo.proto: OPTION_VALUE: Value must be integer, from 0 to 18446744073709551615, " "for uint64 option \"foo\".\n"); } From 35dd193f4787983a69911cb5f0afd6a7763b820a Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Wed, 14 Sep 2022 16:25:14 -0400 Subject: [PATCH 04/11] add test to verify parsing of extremely large decimal integers to double values --- .../protobuf/compiler/parser_unittest.cc | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc index 3b3245191659..7685371c1415 100644 --- a/src/google/protobuf/compiler/parser_unittest.cc +++ b/src/google/protobuf/compiler/parser_unittest.cc @@ -592,6 +592,56 @@ TEST_F(ParseMessageTest, FieldOptions) { "}"); } +TEST_F(ParseMessageTest, FieldOptionsSupportLargeDecimalLiteral) { + // decimal integer literal > uint64 max + ExpectParsesTo( + "import \"google/protobuf/descriptor.proto\";\n" + "extend google.protobuf.FieldOptions {\n" + " optional double f = 10101;\n" + "}\n" + "message TestMessage {\n" + " optional double a = 1 [default = 18446744073709551616];\n" + " optional double b = 2 [default = -18446744073709551616];\n" + " optional double c = 3 [(f) = 18446744073709551616];\n" + " optional double d = 4 [(f) = -18446744073709551616];\n" + "}\n", + + "dependency: \"google/protobuf/descriptor.proto\"" + "extension {" + " name: \"f\" label: LABEL_OPTIONAL type: TYPE_DOUBLE number: 10101" + " extendee: \"google.protobuf.FieldOptions\"" + "}" + "message_type {" + " name: \"TestMessage\"" + " field {" + " name: \"a\" label: LABEL_OPTIONAL type: TYPE_DOUBLE number: 1" + " default_value: \"1.8446744073709552e+19\"" + " }" + " field {" + " name: \"b\" label: LABEL_OPTIONAL type: TYPE_DOUBLE number: 2" + " default_value: \"-1.8446744073709552e+19\"" + " }" + " field {" + " name: \"c\" label: LABEL_OPTIONAL type: TYPE_DOUBLE number: 3" + " options{" + " uninterpreted_option{" + " name{ name_part: \"f\" is_extension: true }" + " double_value: 1.8446744073709552e+19" + " }" + " }" + " }" + " field {" + " name: \"d\" label: LABEL_OPTIONAL type: TYPE_DOUBLE number: 4" + " options{" + " uninterpreted_option{" + " name{ name_part: \"f\" is_extension: true }" + " double_value: -1.8446744073709552e+19" + " }" + " }" + " }" + "}"); +} + TEST_F(ParseMessageTest, Oneof) { ExpectParsesTo( "message TestMessage {\n" From 4c69337faab5cd7a6bd040dbae841e21873004c0 Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Wed, 14 Sep 2022 20:11:19 -0400 Subject: [PATCH 05/11] use template instead of macro --- src/google/protobuf/descriptor.cc | 36 +++++++++++++++++++------------ 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index cd40584b374a..b2841ce6eff2 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -7676,13 +7676,21 @@ bool DescriptorBuilder::OptionInterpreter::ExamineIfOptionIsSet( return true; } -#define VALUE_OUT_OF_RANGE(T, NAME) absl::StrFormat( \ - "Value out of range, %d to %d, for " #T " option \"%s\".", \ - std::numeric_limits::min(), std::numeric_limits::max(), NAME) +template std::string ValueOutOfRange( + std::string type_name, std::string option_name) { + return absl::StrFormat( + "Value out of range, %d to %d, for %s option \"%s\".", \ + std::numeric_limits::min(), std::numeric_limits::max(), + type_name, option_name); +} -#define VALUE_MUST_BE_INT(T, NAME) absl::StrFormat( \ - "Value must be integer, from %d to %d, for " #T " option \"%s\".", \ - std::numeric_limits::min(), std::numeric_limits::max(), NAME) +template std::string ValueMustBeInt( + std::string type_name, std::string option_name) { + return absl::StrFormat( + "Value must be integer, from %d to %d, for %s option \"%s\".", \ + std::numeric_limits::min(), std::numeric_limits::max(), + type_name, option_name); +} bool DescriptorBuilder::OptionInterpreter::SetOptionValue( const FieldDescriptor* option_field, UnknownFieldSet* unknown_fields) { @@ -7692,7 +7700,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( if (uninterpreted_option_->has_positive_int_value()) { if (uninterpreted_option_->positive_int_value() > static_cast(std::numeric_limits::max())) { - return AddValueError(VALUE_OUT_OF_RANGE(int32, option_field->full_name())); + return AddValueError(ValueOutOfRange("int32", option_field->full_name())); } else { SetInt32(option_field->number(), uninterpreted_option_->positive_int_value(), @@ -7701,14 +7709,14 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( } else if (uninterpreted_option_->has_negative_int_value()) { if (uninterpreted_option_->negative_int_value() < static_cast(std::numeric_limits::min())) { - return AddValueError(VALUE_OUT_OF_RANGE(int32, option_field->full_name())); + return AddValueError(ValueOutOfRange("int32", option_field->full_name())); } else { SetInt32(option_field->number(), uninterpreted_option_->negative_int_value(), option_field->type(), unknown_fields); } } else { - return AddValueError(VALUE_MUST_BE_INT(int32, option_field->full_name())); + return AddValueError(ValueMustBeInt("int32", option_field->full_name())); } break; @@ -7716,7 +7724,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( if (uninterpreted_option_->has_positive_int_value()) { if (uninterpreted_option_->positive_int_value() > static_cast(std::numeric_limits::max())) { - return AddValueError(VALUE_OUT_OF_RANGE(int64, option_field->full_name())); + return AddValueError(ValueOutOfRange("int64", option_field->full_name())); } else { SetInt64(option_field->number(), uninterpreted_option_->positive_int_value(), @@ -7727,7 +7735,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( uninterpreted_option_->negative_int_value(), option_field->type(), unknown_fields); } else { - return AddValueError(VALUE_MUST_BE_INT(int64, option_field->full_name())); + return AddValueError(ValueMustBeInt("int64", option_field->full_name())); } break; @@ -7735,14 +7743,14 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( if (uninterpreted_option_->has_positive_int_value()) { if (uninterpreted_option_->positive_int_value() > std::numeric_limits::max()) { - return AddValueError(VALUE_OUT_OF_RANGE(uint32, option_field->full_name())); + return AddValueError(ValueOutOfRange("uint32", option_field->full_name())); } else { SetUInt32(option_field->number(), uninterpreted_option_->positive_int_value(), option_field->type(), unknown_fields); } } else { - return AddValueError(VALUE_MUST_BE_INT(uint32, option_field->full_name())); + return AddValueError(ValueMustBeInt("uint32", option_field->full_name())); } break; @@ -7752,7 +7760,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( uninterpreted_option_->positive_int_value(), option_field->type(), unknown_fields); } else { - return AddValueError(VALUE_MUST_BE_INT(uint64, option_field->full_name())); + return AddValueError(ValueMustBeInt("uint64", option_field->full_name())); } break; From 7702355b9cc9ab7a1fcc851ecaac372debbc25f6 Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Thu, 15 Sep 2022 09:44:15 -0400 Subject: [PATCH 06/11] address latest review comments --- src/google/protobuf/descriptor.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index b2841ce6eff2..ffc8cd628ebe 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -7677,7 +7677,7 @@ bool DescriptorBuilder::OptionInterpreter::ExamineIfOptionIsSet( } template std::string ValueOutOfRange( - std::string type_name, std::string option_name) { + 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::min(), std::numeric_limits::max(), @@ -7685,7 +7685,7 @@ template std::string ValueOutOfRange( } template std::string ValueMustBeInt( - std::string type_name, std::string option_name) { + 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::min(), std::numeric_limits::max(), @@ -7724,7 +7724,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( if (uninterpreted_option_->has_positive_int_value()) { if (uninterpreted_option_->positive_int_value() > static_cast(std::numeric_limits::max())) { - return AddValueError(ValueOutOfRange("int64", option_field->full_name())); + return AddValueError(ValueOutOfRange("int64", option_field->full_name())); } else { SetInt64(option_field->number(), uninterpreted_option_->positive_int_value(), @@ -7735,7 +7735,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( uninterpreted_option_->negative_int_value(), option_field->type(), unknown_fields); } else { - return AddValueError(ValueMustBeInt("int64", option_field->full_name())); + return AddValueError(ValueMustBeInt("int64", option_field->full_name())); } break; @@ -7743,14 +7743,14 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( if (uninterpreted_option_->has_positive_int_value()) { if (uninterpreted_option_->positive_int_value() > std::numeric_limits::max()) { - return AddValueError(ValueOutOfRange("uint32", option_field->full_name())); + return AddValueError(ValueOutOfRange("uint32", option_field->full_name())); } else { SetUInt32(option_field->number(), uninterpreted_option_->positive_int_value(), option_field->type(), unknown_fields); } } else { - return AddValueError(ValueMustBeInt("uint32", option_field->full_name())); + return AddValueError(ValueMustBeInt("uint32", option_field->full_name())); } break; @@ -7760,7 +7760,7 @@ bool DescriptorBuilder::OptionInterpreter::SetOptionValue( uninterpreted_option_->positive_int_value(), option_field->type(), unknown_fields); } else { - return AddValueError(ValueMustBeInt("uint64", option_field->full_name())); + return AddValueError(ValueMustBeInt("uint64", option_field->full_name())); } break; From 0bc90b189cb6d6f6f66f263dcd0afc84daf12e0d Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Thu, 15 Sep 2022 10:36:26 -0400 Subject: [PATCH 07/11] put helpers into anon namespace --- src/google/protobuf/descriptor.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index ffc8cd628ebe..81412ffc6cae 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -7676,6 +7676,9 @@ bool DescriptorBuilder::OptionInterpreter::ExamineIfOptionIsSet( return true; } +namespace { +// Helpers for method below + template std::string ValueOutOfRange( absl::string_view type_name, absl::string_view option_name) { return absl::StrFormat( @@ -7692,6 +7695,8 @@ template std::string ValueMustBeInt( type_name, option_name); } +} // namespace + bool DescriptorBuilder::OptionInterpreter::SetOptionValue( const FieldDescriptor* option_field, UnknownFieldSet* unknown_fields) { // We switch on the CppType to validate. From f82be688318afe5f46ea877a576a208d70d83d9d Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Thu, 15 Sep 2022 11:26:13 -0400 Subject: [PATCH 08/11] avoid possible exception; error if octal or hex literal that is too large --- src/google/protobuf/compiler/parser.cc | 11 ++++++++--- .../protobuf/compiler/parser_unittest.cc | 16 ++++++++++++++++ src/google/protobuf/io/tokenizer.cc | 19 ++++++++++++------- src/google/protobuf/io/tokenizer.h | 4 ++++ 4 files changed, 40 insertions(+), 10 deletions(-) diff --git a/src/google/protobuf/compiler/parser.cc b/src/google/protobuf/compiler/parser.cc index e5f42c45066f..438130762452 100644 --- a/src/google/protobuf/compiler/parser.cc +++ b/src/google/protobuf/compiler/parser.cc @@ -310,9 +310,14 @@ bool Parser::ConsumeNumber(double* output, const char* error) { std::numeric_limits::max(), &value)) { *output = value; - } else { - // out of int range, treat as double literal - *output = io::Tokenizer::ParseFloat(input_->current().text); + } else if (input_->current().text[0] == '0') { + // octal or hexadecimal; don't bother parsing as float + AddError("Integer out of range."); + // We still return true because we did, in fact, parse a number. + } else if (!io::Tokenizer::TryParseFloat(input_->current().text, output)) { + // out of int range, and not valid float? 🤷 + AddError("Integer out of range."); + // We still return true because we did, in fact, parse a number. } input_->Next(); return true; diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc index 7685371c1415..39d5b60744c4 100644 --- a/src/google/protobuf/compiler/parser_unittest.cc +++ b/src/google/protobuf/compiler/parser_unittest.cc @@ -1941,6 +1941,22 @@ TEST_F(ParserValidationErrorTest, FieldDefaultValueError) { "2:32: Enum type \"Baz\" has no value named \"NO_SUCH_VALUE\".\n"); } +TEST_F(ParserValidationErrorTest, FieldDefaultIntegerOutOfRange) { + ExpectHasErrors( + "message Foo {\n" + " optional double bar = 1 [default = 0x10000000000000000];\n" + "}\n", + "1:37: Integer out of range.\n"); +} + +TEST_F(ParserValidationErrorTest, FieldOptionOutOfRange) { + ExpectHasErrors( + "message Foo {\n" + " optional double bar = 1 [foo = 0x10000000000000000];\n" + "}\n", + "1:33: Integer out of range.\n"); +} + TEST_F(ParserValidationErrorTest, FileOptionNameError) { ExpectHasValidationErrors( "option foo = 5;", diff --git a/src/google/protobuf/io/tokenizer.cc b/src/google/protobuf/io/tokenizer.cc index 7bc0820dd85d..127fb5a40258 100644 --- a/src/google/protobuf/io/tokenizer.cc +++ b/src/google/protobuf/io/tokenizer.cc @@ -1002,9 +1002,19 @@ bool Tokenizer::ParseInteger(const std::string& text, uint64_t max_value, } double Tokenizer::ParseFloat(const std::string& text) { + double result; + GOOGLE_LOG_IF(DFATAL, + !TryParseFloat(text, &result)) + << " Tokenizer::ParseFloat() passed text that could not have been" + " tokenized as a float: " + << absl::CEscape(text); + return result; +} + +bool Tokenizer::TryParseFloat(const std::string& text, double* result) { const char* start = text.c_str(); char* end; - double result = NoLocaleStrtod(start, &end); + *result = NoLocaleStrtod(start, &end); // "1e" is not a valid float, but if the tokenizer reads it, it will // report an error but still return it as a valid token. We need to @@ -1020,12 +1030,7 @@ double Tokenizer::ParseFloat(const std::string& text) { ++end; } - GOOGLE_LOG_IF(DFATAL, - static_cast(end - start) != text.size() || *start == '-') - << " Tokenizer::ParseFloat() passed text that could not have been" - " tokenized as a float: " - << absl::CEscape(text); - return result; + return static_cast(end - start) == text.size() && *start != '-'; } // Helper to append a Unicode code point to a string as UTF8, without bringing diff --git a/src/google/protobuf/io/tokenizer.h b/src/google/protobuf/io/tokenizer.h index cab1faf917d1..73877ccc24ac 100644 --- a/src/google/protobuf/io/tokenizer.h +++ b/src/google/protobuf/io/tokenizer.h @@ -214,6 +214,10 @@ class PROTOBUF_EXPORT Tokenizer { // result is undefined (possibly an assert failure). static double ParseFloat(const std::string& text); + // Parses given text as if it were a TYPE_FLOAT token. Returns false if the + // given text is not actually a valid float literal. + static bool TryParseFloat(const std::string& text, double* result); + // Parses a TYPE_STRING token. This never fails, so long as the text actually // comes from a TYPE_STRING token parsed by Tokenizer. If it doesn't, the // result is undefined (possibly an assert failure). From d6acffba7bf67e8f928acf1cc676056919ad19e3 Mon Sep 17 00:00:00 2001 From: Josh Humphries Date: Thu, 15 Sep 2022 13:37:59 -0400 Subject: [PATCH 09/11] use normal conditional --- src/google/protobuf/io/tokenizer.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/google/protobuf/io/tokenizer.cc b/src/google/protobuf/io/tokenizer.cc index 127fb5a40258..d14c5a19268d 100644 --- a/src/google/protobuf/io/tokenizer.cc +++ b/src/google/protobuf/io/tokenizer.cc @@ -1003,11 +1003,12 @@ bool Tokenizer::ParseInteger(const std::string& text, uint64_t max_value, double Tokenizer::ParseFloat(const std::string& text) { double result; - GOOGLE_LOG_IF(DFATAL, - !TryParseFloat(text, &result)) - << " Tokenizer::ParseFloat() passed text that could not have been" - " tokenized as a float: " - << absl::CEscape(text); + if (!TryParseFloat(text, &result)) { + LOG(DFATAL) + << " Tokenizer::ParseFloat() passed text that could not have been" + " tokenized as a float: " + << absl::CEscape(text); + } return result; } From 0362a1204f269161fe20e53ff1184440bdeeb792 Mon Sep 17 00:00:00 2001 From: Joshua Humphries Date: Thu, 15 Sep 2022 14:01:43 -0400 Subject: [PATCH 10/11] initialize var to avoid undefined return val --- src/google/protobuf/io/tokenizer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/google/protobuf/io/tokenizer.cc b/src/google/protobuf/io/tokenizer.cc index d14c5a19268d..d7f100dbbd71 100644 --- a/src/google/protobuf/io/tokenizer.cc +++ b/src/google/protobuf/io/tokenizer.cc @@ -1002,7 +1002,7 @@ bool Tokenizer::ParseInteger(const std::string& text, uint64_t max_value, } double Tokenizer::ParseFloat(const std::string& text) { - double result; + double result = 0; if (!TryParseFloat(text, &result)) { LOG(DFATAL) << " Tokenizer::ParseFloat() passed text that could not have been" From 7e745c4910630f49b243c7f8d1db2050bdb4ff01 Mon Sep 17 00:00:00 2001 From: Joshua Humphries Date: Thu, 15 Sep 2022 15:24:48 -0400 Subject: [PATCH 11/11] oops, fix name: LOG -> GOOGLE_LOG --- src/google/protobuf/io/tokenizer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/google/protobuf/io/tokenizer.cc b/src/google/protobuf/io/tokenizer.cc index d7f100dbbd71..4fcf81b3dc39 100644 --- a/src/google/protobuf/io/tokenizer.cc +++ b/src/google/protobuf/io/tokenizer.cc @@ -1004,7 +1004,7 @@ bool Tokenizer::ParseInteger(const std::string& text, uint64_t max_value, double Tokenizer::ParseFloat(const std::string& text) { double result = 0; if (!TryParseFloat(text, &result)) { - LOG(DFATAL) + GOOGLE_LOG(DFATAL) << " Tokenizer::ParseFloat() passed text that could not have been" " tokenized as a float: " << absl::CEscape(text);