Merge pull request protocolbuffers#8444 from perezd/sync-stage

Integrate from Piper for C++, Java, and Python

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.

csharp/src/Google.Protobuf/Reflection/Descriptor.cs

@@ -4796,11 +4796,11 @@ public void ClearJavaPackage() {
 
     private string javaOuterClassname_;
     /// <summary>
-    /// 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.
     /// </summary>
     [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
     public string JavaOuterClassname {
@@ -4826,10 +4826,10 @@ public void ClearJavaOuterClassname() {
 
     private bool javaMultipleFiles_;
     /// <summary>
-    /// 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.
     /// </summary>

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();

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.
+    }
+  }
 }

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];

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")

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,

src/google/protobuf/util/message_differencer.cc

@@ -148,18 +148,16 @@ class MessageDifferencer::MultipleFieldsMapKeyComparator
     const FieldDescriptor* field = key_field_path[path_index];
     std::vector<SpecificField> current_parent_fields(parent_fields);
     if (path_index == static_cast<int64_t>(key_field_path.size() - 1)) {
-      if (field->is_repeated()) {
-        if (!message_differencer_->CompareRepeatedField(
-                message1, message2, field, &current_parent_fields)) {
-          return false;
-        }
+      if (field->is_map()) {
+        return message_differencer_->CompareMapField(message1, message2, field,
+                                                     &current_parent_fields);
+      } else if (field->is_repeated()) {
+        return message_differencer_->CompareRepeatedField(
+            message1, message2, field, &current_parent_fields);
       } else {
-        if (!message_differencer_->CompareFieldValueUsingParentFields(
-                message1, message2, field, -1, -1, &current_parent_fields)) {
-          return false;
-        }
+        return message_differencer_->CompareFieldValueUsingParentFields(
+            message1, message2, field, -1, -1, &current_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<SpecificField>* 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<SpecificField>* 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<SpecificField>* 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);