diff --git a/CHANGES.txt b/CHANGES.txt index 05a15edbf562..f4c9eafe45b5 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -22,8 +22,11 @@ Unreleased Changes (C++/Java/Python/PHP/Objective-C/C#/Ruby/JavaScript) on an error path. * Avoid expensive inlined code space for encoding message length for messages >= 128 bytes and instead do a procedure call to a shared out-of-line routine. + * util::DefaultFieldComparator will be final in a future version of protobuf. + Subclasses should inherit from SimpleFieldComparator instead. Java: + * Detect invalid overflow of byteLimit and return InvalidProtocolBufferException as documented. * Exceptions thrown while reading from an InputStream in parseFrom are now included as causes. * Support potentially more efficient proto parsing from RopeByteStrings. diff --git a/csharp/src/Google.Protobuf.Test/testprotos.pb b/csharp/src/Google.Protobuf.Test/testprotos.pb index 06a17c1038c0..42ecd3adf8e3 100644 Binary files a/csharp/src/Google.Protobuf.Test/testprotos.pb and b/csharp/src/Google.Protobuf.Test/testprotos.pb differ diff --git a/csharp/src/Google.Protobuf/Reflection/Descriptor.cs b/csharp/src/Google.Protobuf/Reflection/Descriptor.cs index 5c5b2ffaf4f9..a1ad559bee77 100644 --- a/csharp/src/Google.Protobuf/Reflection/Descriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/Descriptor.cs @@ -4796,11 +4796,11 @@ public void ClearJavaPackage() { private string javaOuterClassname_; /// - /// If set, all the classes from the .proto file are wrapped in a single - /// outer class with the given name. This applies to both Proto1 - /// (equivalent to the old "--one_java_file" option) and Proto2 (where - /// a .proto always translates to a single class, but you may want to - /// explicitly choose the class name). + /// Controls the name of the wrapper Java class generated for the .proto file. + /// That class will always contain the .proto file's getDescriptor() method as + /// well as any top-level extensions defined in the .proto file. + /// If java_multiple_files is disabled, then all the other classes from the + /// .proto file will be nested inside the single wrapper outer class. /// [global::System.Diagnostics.DebuggerNonUserCodeAttribute] public string JavaOuterClassname { @@ -4826,10 +4826,10 @@ public void ClearJavaOuterClassname() { private bool javaMultipleFiles_; /// - /// If set true, then the Java code generator will generate a separate .java + /// If enabled, then the Java code generator will generate a separate .java /// file for each top-level message, enum, and service defined in the .proto - /// file. Thus, these types will *not* be nested inside the outer class - /// named by java_outer_classname. However, the outer class will still be + /// file. Thus, these types will *not* be nested inside the wrapper class + /// named by java_outer_classname. However, the wrapper class will still be /// generated to contain the file's getDescriptor() method as well as any /// top-level extensions defined in the file. /// diff --git a/java/core/src/main/java/com/google/protobuf/CodedInputStream.java b/java/core/src/main/java/com/google/protobuf/CodedInputStream.java index 6098a9ad8e79..37b986d7a5df 100644 --- a/java/core/src/main/java/com/google/protobuf/CodedInputStream.java +++ b/java/core/src/main/java/com/google/protobuf/CodedInputStream.java @@ -1185,6 +1185,9 @@ public int pushLimit(int byteLimit) throws InvalidProtocolBufferException { throw InvalidProtocolBufferException.negativeSize(); } byteLimit += getTotalBytesRead(); + if (byteLimit < 0) { + throw InvalidProtocolBufferException.parseFailure(); + } final int oldLimit = currentLimit; if (byteLimit > oldLimit) { throw InvalidProtocolBufferException.truncatedMessage(); diff --git a/java/core/src/test/java/com/google/protobuf/CodedInputStreamTest.java b/java/core/src/test/java/com/google/protobuf/CodedInputStreamTest.java index 4de5f5b12d2d..10d156ead2ca 100644 --- a/java/core/src/test/java/com/google/protobuf/CodedInputStreamTest.java +++ b/java/core/src/test/java/com/google/protobuf/CodedInputStreamTest.java @@ -1284,4 +1284,33 @@ public synchronized int read(byte[] b, int off, int len) { maliciousCapture.get(1)[0] = 0x9; assertEquals(0x9, byteArray[0]); // MODIFICATION! Should we fix? } + + public void testInvalidInputYieldsInvalidProtocolBufferException_readTag() throws Exception { + byte[] input = new byte[] {0x0a, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, 0x77}; + CodedInputStream inputStream = CodedInputStream.newInstance(input); + try { + inputStream.readTag(); + int size = inputStream.readRawVarint32(); + inputStream.pushLimit(size); + inputStream.readTag(); + fail(); + } catch (InvalidProtocolBufferException ex) { + // Expected. + } + } + + public void testInvalidInputYieldsInvalidProtocolBufferException_readBytes() throws Exception { + byte[] input = + new byte[] {0x0a, (byte) 0xff, (byte) 0xff, (byte) 0xff, (byte) 0xff, 0x67, 0x1a, 0x1a}; + CodedInputStream inputStream = CodedInputStream.newInstance(input); + try { + inputStream.readTag(); + int size = inputStream.readRawVarint32(); + inputStream.pushLimit(size); + inputStream.readBytes(); + fail(); + } catch (InvalidProtocolBufferException ex) { + // Expected. + } + } } diff --git a/php/src/Google/Protobuf/Internal/FileOptions.php b/php/src/Google/Protobuf/Internal/FileOptions.php index 6283b2ad7a2b..3f2c3a4d4ccf 100644 --- a/php/src/Google/Protobuf/Internal/FileOptions.php +++ b/php/src/Google/Protobuf/Internal/FileOptions.php @@ -25,20 +25,20 @@ class FileOptions extends \Google\Protobuf\Internal\Message */ protected $java_package = null; /** - * If set, all the classes from the .proto file are wrapped in a single - * outer class with the given name. This applies to both Proto1 - * (equivalent to the old "--one_java_file" option) and Proto2 (where - * a .proto always translates to a single class, but you may want to - * explicitly choose the class name). + * Controls the name of the wrapper Java class generated for the .proto file. + * That class will always contain the .proto file's getDescriptor() method as + * well as any top-level extensions defined in the .proto file. + * If java_multiple_files is disabled, then all the other classes from the + * .proto file will be nested inside the single wrapper outer class. * * Generated from protobuf field optional string java_outer_classname = 8; */ protected $java_outer_classname = null; /** - * If set true, then the Java code generator will generate a separate .java + * If enabled, then the Java code generator will generate a separate .java * file for each top-level message, enum, and service defined in the .proto - * file. Thus, these types will *not* be nested inside the outer class - * named by java_outer_classname. However, the outer class will still be + * file. Thus, these types will *not* be nested inside the wrapper class + * named by java_outer_classname. However, the wrapper class will still be * generated to contain the file's getDescriptor() method as well as any * top-level extensions defined in the file. * @@ -192,16 +192,16 @@ class FileOptions extends \Google\Protobuf\Internal\Message * inappropriate because proto packages do not normally start with backwards * domain names. * @type string $java_outer_classname - * If set, all the classes from the .proto file are wrapped in a single - * outer class with the given name. This applies to both Proto1 - * (equivalent to the old "--one_java_file" option) and Proto2 (where - * a .proto always translates to a single class, but you may want to - * explicitly choose the class name). + * Controls the name of the wrapper Java class generated for the .proto file. + * That class will always contain the .proto file's getDescriptor() method as + * well as any top-level extensions defined in the .proto file. + * If java_multiple_files is disabled, then all the other classes from the + * .proto file will be nested inside the single wrapper outer class. * @type bool $java_multiple_files - * If set true, then the Java code generator will generate a separate .java + * If enabled, then the Java code generator will generate a separate .java * file for each top-level message, enum, and service defined in the .proto - * file. Thus, these types will *not* be nested inside the outer class - * named by java_outer_classname. However, the outer class will still be + * file. Thus, these types will *not* be nested inside the wrapper class + * named by java_outer_classname. However, the wrapper class will still be * generated to contain the file's getDescriptor() method as well as any * top-level extensions defined in the file. * @type bool $java_generate_equals_and_hash @@ -319,11 +319,11 @@ public function setJavaPackage($var) } /** - * If set, all the classes from the .proto file are wrapped in a single - * outer class with the given name. This applies to both Proto1 - * (equivalent to the old "--one_java_file" option) and Proto2 (where - * a .proto always translates to a single class, but you may want to - * explicitly choose the class name). + * Controls the name of the wrapper Java class generated for the .proto file. + * That class will always contain the .proto file's getDescriptor() method as + * well as any top-level extensions defined in the .proto file. + * If java_multiple_files is disabled, then all the other classes from the + * .proto file will be nested inside the single wrapper outer class. * * Generated from protobuf field optional string java_outer_classname = 8; * @return string @@ -344,11 +344,11 @@ public function clearJavaOuterClassname() } /** - * If set, all the classes from the .proto file are wrapped in a single - * outer class with the given name. This applies to both Proto1 - * (equivalent to the old "--one_java_file" option) and Proto2 (where - * a .proto always translates to a single class, but you may want to - * explicitly choose the class name). + * Controls the name of the wrapper Java class generated for the .proto file. + * That class will always contain the .proto file's getDescriptor() method as + * well as any top-level extensions defined in the .proto file. + * If java_multiple_files is disabled, then all the other classes from the + * .proto file will be nested inside the single wrapper outer class. * * Generated from protobuf field optional string java_outer_classname = 8; * @param string $var @@ -363,10 +363,10 @@ public function setJavaOuterClassname($var) } /** - * If set true, then the Java code generator will generate a separate .java + * If enabled, then the Java code generator will generate a separate .java * file for each top-level message, enum, and service defined in the .proto - * file. Thus, these types will *not* be nested inside the outer class - * named by java_outer_classname. However, the outer class will still be + * file. Thus, these types will *not* be nested inside the wrapper class + * named by java_outer_classname. However, the wrapper class will still be * generated to contain the file's getDescriptor() method as well as any * top-level extensions defined in the file. * @@ -389,10 +389,10 @@ public function clearJavaMultipleFiles() } /** - * If set true, then the Java code generator will generate a separate .java + * If enabled, then the Java code generator will generate a separate .java * file for each top-level message, enum, and service defined in the .proto - * file. Thus, these types will *not* be nested inside the outer class - * named by java_outer_classname. However, the outer class will still be + * file. Thus, these types will *not* be nested inside the wrapper class + * named by java_outer_classname. However, the wrapper class will still be * generated to contain the file's getDescriptor() method as well as any * top-level extensions defined in the file. * diff --git a/src/google/protobuf/descriptor.proto b/src/google/protobuf/descriptor.proto index 69c75208eeb0..156e410ae10f 100644 --- a/src/google/protobuf/descriptor.proto +++ b/src/google/protobuf/descriptor.proto @@ -348,17 +348,17 @@ message FileOptions { optional string java_package = 1; - // If set, all the classes from the .proto file are wrapped in a single - // outer class with the given name. This applies to both Proto1 - // (equivalent to the old "--one_java_file" option) and Proto2 (where - // a .proto always translates to a single class, but you may want to - // explicitly choose the class name). + // Controls the name of the wrapper Java class generated for the .proto file. + // That class will always contain the .proto file's getDescriptor() method as + // well as any top-level extensions defined in the .proto file. + // If java_multiple_files is disabled, then all the other classes from the + // .proto file will be nested inside the single wrapper outer class. optional string java_outer_classname = 8; - // If set true, then the Java code generator will generate a separate .java + // If enabled, then the Java code generator will generate a separate .java // file for each top-level message, enum, and service defined in the .proto - // file. Thus, these types will *not* be nested inside the outer class - // named by java_outer_classname. However, the outer class will still be + // file. Thus, these types will *not* be nested inside the wrapper class + // named by java_outer_classname. However, the wrapper class will still be // generated to contain the file's getDescriptor() method as well as any // top-level extensions defined in the file. optional bool java_multiple_files = 10 [default = false]; diff --git a/src/google/protobuf/port_undef.inc b/src/google/protobuf/port_undef.inc index daef09bc45af..85917621d460 100644 --- a/src/google/protobuf/port_undef.inc +++ b/src/google/protobuf/port_undef.inc @@ -79,6 +79,10 @@ #undef PROTOBUF_ATTRIBUTE_INIT_PRIORITY #undef PROTOBUF_PRAGMA_INIT_SEG +#ifdef PROTOBUF_FUTURE_BREAKING_CHANGES +#undef PROTOBUF_FUTURE_BREAKING_CHANGES +#endif + // Restore macro that may have been #undef'd in port_def.inc. #ifdef _MSC_VER #pragma pop_macro("CREATE_NEW") diff --git a/src/google/protobuf/util/field_comparator.h b/src/google/protobuf/util/field_comparator.h index 4bd5395e56e3..dd1a48699672 100644 --- a/src/google/protobuf/util/field_comparator.h +++ b/src/google/protobuf/util/field_comparator.h @@ -146,15 +146,6 @@ class PROTOBUF_EXPORT SimpleFieldComparator : public FieldComparator { void SetDefaultFractionAndMargin(double fraction, double margin); protected: - // NOTE: this will go away. - ComparisonResult Compare(const Message& message_1, const Message& message_2, - const FieldDescriptor* field, int index_1, - int index_2, - const util::FieldContext* field_context) override { - return SimpleCompare(message_1, message_2, field, index_1, index_2, - field_context); - } - // Returns the comparison result for the given field in two messages. // // This function is called directly by DefaultFieldComparator::Compare. @@ -268,7 +259,13 @@ class PROTOBUF_EXPORT SimpleFieldComparator : public FieldComparator { }; // Default field comparison: use the basic implementation of FieldComparator. -class PROTOBUF_EXPORT DefaultFieldComparator : public SimpleFieldComparator { +#ifdef PROTOBUF_FUTURE_BREAKING_CHANGES +class PROTOBUF_EXPORT DefaultFieldComparator final + : public SimpleFieldComparator +#else // PROTOBUF_FUTURE_BREAKING_CHANGES +class PROTOBUF_EXPORT DefaultFieldComparator : public SimpleFieldComparator +#endif // PROTOBUF_FUTURE_BREAKING_CHANGES +{ public: ComparisonResult Compare(const Message& message_1, const Message& message_2, const FieldDescriptor* field, int index_1, diff --git a/src/google/protobuf/util/message_differencer.cc b/src/google/protobuf/util/message_differencer.cc index 10fdc021a258..4815178cd401 100644 --- a/src/google/protobuf/util/message_differencer.cc +++ b/src/google/protobuf/util/message_differencer.cc @@ -148,18 +148,16 @@ class MessageDifferencer::MultipleFieldsMapKeyComparator const FieldDescriptor* field = key_field_path[path_index]; std::vector current_parent_fields(parent_fields); if (path_index == static_cast(key_field_path.size() - 1)) { - if (field->is_repeated()) { - if (!message_differencer_->CompareRepeatedField( - message1, message2, field, ¤t_parent_fields)) { - return false; - } + if (field->is_map()) { + return message_differencer_->CompareMapField(message1, message2, field, + ¤t_parent_fields); + } else if (field->is_repeated()) { + return message_differencer_->CompareRepeatedField( + message1, message2, field, ¤t_parent_fields); } else { - if (!message_differencer_->CompareFieldValueUsingParentFields( - message1, message2, field, -1, -1, ¤t_parent_fields)) { - return false; - } + return message_differencer_->CompareFieldValueUsingParentFields( + message1, message2, field, -1, -1, ¤t_parent_fields); } - return true; } else { const Reflection* reflection1 = message1.GetReflection(); const Reflection* reflection2 = message2.GetReflection(); @@ -830,24 +828,17 @@ bool MessageDifferencer::CompareWithFieldsInternal( bool fieldDifferent = false; assert(field1 != NULL); - if (field1->is_repeated()) { + if (field1->is_map()) { + fieldDifferent = + !CompareMapField(message1, message2, field1, parent_fields); + } else if (field1->is_repeated()) { fieldDifferent = !CompareRepeatedField(message1, message2, field1, parent_fields); - if (fieldDifferent) { - if (reporter_ == NULL) return false; - isDifferent = true; - } } else { fieldDifferent = !CompareFieldValueUsingParentFields( message1, message2, field1, -1, -1, parent_fields); - // If we have found differences, either report them or terminate if - // no reporter is present. - if (fieldDifferent && reporter_ == NULL) { - return false; - } - - if (reporter_ != NULL) { + if (reporter_ != nullptr) { SpecificField specific_field; specific_field.field = field1; parent_fields->push_back(specific_field); @@ -860,6 +851,10 @@ bool MessageDifferencer::CompareWithFieldsInternal( parent_fields->pop_back(); } } + if (fieldDifferent) { + if (reporter_ == nullptr) return false; + isDifferent = true; + } // Increment the field indices. ++field_index1; ++field_index2; @@ -1002,17 +997,19 @@ bool MessageDifferencer::CompareMapFieldByMapReflection( return true; } -bool MessageDifferencer::CompareRepeatedField( +bool MessageDifferencer::CompareMapField( const Message& message1, const Message& message2, const FieldDescriptor* repeated_field, std::vector* parent_fields) { + GOOGLE_DCHECK(repeated_field->is_map()); + // the input FieldDescriptor is guaranteed to be repeated field. const Reflection* reflection1 = message1.GetReflection(); const Reflection* reflection2 = message2.GetReflection(); // When both map fields are on map, do not sync to repeated field. // TODO(jieluo): Add support for reporter - if (repeated_field->is_map() && reporter_ == nullptr && + if (reporter_ == nullptr && // Users didn't set custom map field key comparator map_field_key_comparator_.find(repeated_field) == map_field_key_comparator_.end() && @@ -1052,6 +1049,26 @@ bool MessageDifferencer::CompareRepeatedField( } } + return CompareRepeatedRep(message1, message2, repeated_field, parent_fields); +} + +bool MessageDifferencer::CompareRepeatedField( + const Message& message1, const Message& message2, + const FieldDescriptor* repeated_field, + std::vector* parent_fields) { + GOOGLE_DCHECK(!repeated_field->is_map()); + return CompareRepeatedRep(message1, message2, repeated_field, parent_fields); +} + +bool MessageDifferencer::CompareRepeatedRep( + const Message& message1, const Message& message2, + const FieldDescriptor* repeated_field, + std::vector* parent_fields) { + // the input FieldDescriptor is guaranteed to be repeated field. + GOOGLE_DCHECK(repeated_field->is_repeated()); + const Reflection* reflection1 = message1.GetReflection(); + const Reflection* reflection2 = message2.GetReflection(); + const int count1 = reflection1->FieldSize(message1, repeated_field); const int count2 = reflection2->FieldSize(message2, repeated_field); const bool treated_as_subset = IsTreatedAsSubset(repeated_field); diff --git a/src/google/protobuf/util/message_differencer.h b/src/google/protobuf/util/message_differencer.h index 57588b9e3df2..943a0db2d3e8 100644 --- a/src/google/protobuf/util/message_differencer.h +++ b/src/google/protobuf/util/message_differencer.h @@ -779,7 +779,20 @@ class PROTOBUF_EXPORT MessageDifferencer { const FieldDescriptor* field, std::vector* parent_fields); - // Compare the map fields using map reflection instead of sync to repeated. + // Compares map fields, and report the error. + bool CompareMapField(const Message& message1, const Message& message2, + const FieldDescriptor* field, + std::vector* parent_fields); + + // Helper for CompareRepeatedField and CompareMapField: compares and reports + // differences element-wise. This is the implementation for non-map fields, + // and can also compare map fields by using the underlying representation. + bool CompareRepeatedRep(const Message& message1, const Message& message2, + const FieldDescriptor* field, + std::vector* parent_fields); + + // Helper for CompareMapField: compare the map fields using map reflection + // instead of sync to repeated. bool CompareMapFieldByMapReflection(const Message& message1, const Message& message2, const FieldDescriptor* field,