From b7285b6c695b22e49896cbc14d6e5a1252489d69 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Fri, 16 Apr 2021 12:59:54 -0700 Subject: [PATCH] Sync from Piper @368903491 PROTOBUF_SYNC_PIPER --- CHANGES.txt | 8 + conformance/failure_list_csharp.txt | 6 +- conformance/failure_list_java_lite.txt | 10 ++ conformance/failure_list_php.txt | 2 + conformance/failure_list_php_c.txt | 2 + conformance/failure_list_ruby.txt | 2 + .../text_format_failure_list_java_lite.txt | 5 + src/google/protobuf/map_test.cc | 15 ++ src/google/protobuf/message.h | 22 ++- src/google/protobuf/message_unittest.inc | 167 ++++++++++++++++++ src/google/protobuf/unittest.proto | 6 + src/google/protobuf/wire_format_unittest.cc | 62 ++++++- 12 files changed, 298 insertions(+), 9 deletions(-) create mode 100644 conformance/failure_list_java_lite.txt create mode 100644 conformance/text_format_failure_list_java_lite.txt diff --git a/CHANGES.txt b/CHANGES.txt index 9e6eb44885..90378673a7 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -9,6 +9,14 @@ Unreleased Changes (C++/Java/Python/PHP/Objective-C/C#/Ruby/JavaScript) Kotlin * Restrict extension setter and getter operators to non-nullable T. + Python + * Make JSON parsing match C++ and Java when multiple fields from the same + oneof are present and all but one is null. + + Conformance Tests + * Added a conformance test for the case of multiple fields from the same + oneof. + 3.16.0 (C++/Java/Python/PHP/Objective-C/C#/Ruby/JavaScript) C++ diff --git a/conformance/failure_list_csharp.txt b/conformance/failure_list_csharp.txt index 31bdf25ebf..bba295e7e2 100644 --- a/conformance/failure_list_csharp.txt +++ b/conformance/failure_list_csharp.txt @@ -1,3 +1,7 @@ +Recommended.Proto2.JsonInput.FieldNameExtension.Validator Recommended.Proto3.JsonInput.BytesFieldBase64Url.JsonOutput Recommended.Proto3.JsonInput.BytesFieldBase64Url.ProtobufOutput -Recommended.Proto2.JsonInput.FieldNameExtension.Validator +Required.Proto3.JsonInput.OneofFieldNullFirst.JsonOutput +Required.Proto3.JsonInput.OneofFieldNullFirst.ProtobufOutput +Required.Proto3.JsonInput.OneofFieldNullSecond.JsonOutput +Required.Proto3.JsonInput.OneofFieldNullSecond.ProtobufOutput diff --git a/conformance/failure_list_java_lite.txt b/conformance/failure_list_java_lite.txt new file mode 100644 index 0000000000..57a082e602 --- /dev/null +++ b/conformance/failure_list_java_lite.txt @@ -0,0 +1,10 @@ +# This is the list of conformance tests that are known to fail for the Java +# implementation right now. These should be fixed. +# +# By listing them here we can keep tabs on which ones are failing and be sure +# that we don't introduce regressions in other tests. + +Required.Proto3.ProtobufInput.PrematureEofInDelimitedDataForKnownNonRepeatedValue.MESSAGE +Required.Proto3.ProtobufInput.PrematureEofInDelimitedDataForKnownRepeatedValue.MESSAGE +Required.Proto2.ProtobufInput.PrematureEofInDelimitedDataForKnownNonRepeatedValue.MESSAGE +Required.Proto2.ProtobufInput.PrematureEofInDelimitedDataForKnownRepeatedValue.MESSAGE diff --git a/conformance/failure_list_php.txt b/conformance/failure_list_php.txt index d51a75dc36..667f80ca37 100644 --- a/conformance/failure_list_php.txt +++ b/conformance/failure_list_php.txt @@ -13,6 +13,8 @@ Required.Proto3.JsonInput.FloatFieldTooSmall Required.Proto3.JsonInput.Int32FieldNotInteger Required.Proto3.JsonInput.Int64FieldNotInteger Required.Proto3.JsonInput.OneofFieldDuplicate +Required.Proto3.JsonInput.OneofFieldNullSecond.JsonOutput +Required.Proto3.JsonInput.OneofFieldNullSecond.ProtobufOutput Required.Proto3.JsonInput.RepeatedFieldWrongElementTypeExpectingStringsGotInt Required.Proto3.JsonInput.RepeatedListValue.JsonOutput Required.Proto3.JsonInput.RepeatedListValue.ProtobufOutput diff --git a/conformance/failure_list_php_c.txt b/conformance/failure_list_php_c.txt index 63c7e8a024..1982029112 100644 --- a/conformance/failure_list_php_c.txt +++ b/conformance/failure_list_php_c.txt @@ -1,2 +1,4 @@ Recommended.Proto2.JsonInput.FieldNameExtension.Validator Required.Proto2.JsonInput.StoresDefaultPrimitive.Validator +Required.Proto3.JsonInput.OneofFieldNullSecond.JsonOutput +Required.Proto3.JsonInput.OneofFieldNullSecond.ProtobufOutput diff --git a/conformance/failure_list_ruby.txt b/conformance/failure_list_ruby.txt index 4938202ad7..ea5de36609 100644 --- a/conformance/failure_list_ruby.txt +++ b/conformance/failure_list_ruby.txt @@ -56,3 +56,5 @@ Recommended.Proto3.ProtobufInput.ValidDataRepeated.UINT32.PackedInput.UnpackedOu Recommended.Proto3.ProtobufInput.ValidDataRepeated.UINT32.UnpackedInput.UnpackedOutput.ProtobufOutput Recommended.Proto3.ProtobufInput.ValidDataRepeated.UINT64.PackedInput.UnpackedOutput.ProtobufOutput Recommended.Proto3.ProtobufInput.ValidDataRepeated.UINT64.UnpackedInput.UnpackedOutput.ProtobufOutput +Required.Proto3.JsonInput.OneofFieldNullSecond.JsonOutput +Required.Proto3.JsonInput.OneofFieldNullSecond.ProtobufOutput diff --git a/conformance/text_format_failure_list_java_lite.txt b/conformance/text_format_failure_list_java_lite.txt new file mode 100644 index 0000000000..61f1a964fe --- /dev/null +++ b/conformance/text_format_failure_list_java_lite.txt @@ -0,0 +1,5 @@ +# This is the list of conformance tests that are known to fail for the Java +# Lite TextFormat implementation right now. These should be fixed. +# +# By listing them here we can keep tabs on which ones are failing and be sure +# that we don't introduce regressions in other tests. diff --git a/src/google/protobuf/map_test.cc b/src/google/protobuf/map_test.cc index 335756eec9..3124f3bb60 100644 --- a/src/google/protobuf/map_test.cc +++ b/src/google/protobuf/map_test.cc @@ -2415,6 +2415,21 @@ TEST(GeneratedMapFieldTest, SerializationToStream) { MapTestUtil::ExpectMapFieldsSet(message2); } +TEST(GeneratedMapFieldTest, ParseFailsIfMalformed) { + unittest::TestMapSubmessage o, p; + auto m = o.mutable_test_map()->mutable_map_int32_foreign_message(); + (*m)[0].set_c(-1); + std::string serialized; + EXPECT_TRUE(o.SerializeToString(&serialized)); + + // Should parse correctly. + EXPECT_TRUE(p.ParseFromString(serialized)); + + // Overwriting the last byte to 0xFF results in malformed wire. + serialized[serialized.size() - 1] = 0xFF; + EXPECT_FALSE(p.ParseFromString(serialized)); +} + TEST(GeneratedMapFieldTest, SameTypeMaps) { const Descriptor* map1 = unittest::TestSameTypeMap::descriptor() diff --git a/src/google/protobuf/message.h b/src/google/protobuf/message.h index c9db4e8403..9cfa5e171d 100644 --- a/src/google/protobuf/message.h +++ b/src/google/protobuf/message.h @@ -592,12 +592,20 @@ class PROTOBUF_EXPORT Reflection final { // the compiled-in class for this type, NOT DynamicMessage. Message* MutableMessage(Message* message, const FieldDescriptor* field, MessageFactory* factory = nullptr) const; + // Replaces the message specified by 'field' with the already-allocated object // sub_message, passing ownership to the message. If the field contained a // message, that message is deleted. If sub_message is nullptr, the field is // cleared. void SetAllocatedMessage(Message* message, Message* sub_message, const FieldDescriptor* field) const; + + // Similar to `SetAllocatedMessage`, but omits all internal safety and + // ownership checks. This method should only be used when the objects are on + // the same arena or paired with a call to `UnsafeArenaReleaseMessage`. + void UnsafeArenaSetAllocatedMessage(Message* message, Message* sub_message, + const FieldDescriptor* field) const; + // Releases the message specified by 'field' and returns the pointer, // ReleaseMessage() will return the message the message object if it exists. // Otherwise, it may or may not return nullptr. In any case, if the return @@ -609,6 +617,13 @@ class PROTOBUF_EXPORT Reflection final { Message* message, const FieldDescriptor* field, MessageFactory* factory = nullptr) const; + // Similar to `ReleaseMessage`, but omits all internal safety and ownership + // checks. This method should only be used when the objects are on the same + // arena or paired with a call to `UnsafeArenaSetAllocatedMessage`. + Message* UnsafeArenaReleaseMessage(Message* message, + const FieldDescriptor* field, + MessageFactory* factory = nullptr) const; + // Repeated field getters ------------------------------------------ // These get the value of one element of a repeated field. @@ -1123,13 +1138,6 @@ class PROTOBUF_EXPORT Reflection final { void AddEnumValueInternal(Message* message, const FieldDescriptor* field, int value) const; - Message* UnsafeArenaReleaseMessage(Message* message, - const FieldDescriptor* field, - MessageFactory* factory = nullptr) const; - - void UnsafeArenaSetAllocatedMessage(Message* message, Message* sub_message, - const FieldDescriptor* field) const; - friend inline // inline so nobody can call this function. void RegisterAllTypesInternal(const Metadata* file_level_metadata, int size); diff --git a/src/google/protobuf/message_unittest.inc b/src/google/protobuf/message_unittest.inc index 78a12972ef..dc307505e7 100644 --- a/src/google/protobuf/message_unittest.inc +++ b/src/google/protobuf/message_unittest.inc @@ -56,9 +56,11 @@ #include #include #include +#include #include #include #include +#include #include @@ -208,6 +210,171 @@ TEST(MESSAGE_TEST_NAME, ParseFailsIfNotInitialized) { errors[0]); } +TEST(MESSAGE_TEST_NAME, ParseFailsIfSubmessageNotInitialized) { + UNITTEST::TestRequiredForeign source, message; + source.mutable_optional_message()->set_dummy2(100); + std::string serialized = source.SerializePartialAsString(); + + EXPECT_TRUE(message.ParsePartialFromString(serialized)); + EXPECT_FALSE(message.IsInitialized()); + + std::vector errors; + { + ScopedMemoryLog log; + EXPECT_FALSE(message.ParseFromString(source.SerializePartialAsString())); + errors = log.GetMessages(ERROR); + } + + EXPECT_THAT( + errors, + testing::ElementsAre( + "Can't parse message of type \"" + + std::string(UNITTEST_PACKAGE_NAME) + + ".TestRequiredForeign\" because it is missing required fields: " + "optional_message.a, optional_message.b, optional_message.c")); +} + +TEST(MESSAGE_TEST_NAME, ParseFailsIfExtensionNotInitialized) { + UNITTEST::TestChildExtension source, message; + auto* r = source.mutable_optional_extension()->MutableExtension( + UNITTEST::TestRequired::single); + r->set_dummy2(100); + std::string serialized = source.SerializePartialAsString(); + + EXPECT_TRUE(message.ParsePartialFromString(serialized)); + EXPECT_FALSE(message.IsInitialized()); + + std::vector errors; + { + ScopedMemoryLog log; + EXPECT_FALSE(message.ParseFromString(source.SerializePartialAsString())); + errors = log.GetMessages(ERROR); + } + + EXPECT_THAT(errors, + testing::ElementsAre(strings::Substitute( + "Can't parse message of type \"$0.TestChildExtension\" " + "because it is missing required fields: " + "optional_extension.($0.TestRequired.single).a, " + "optional_extension.($0.TestRequired.single).b, " + "optional_extension.($0.TestRequired.single).c", + UNITTEST_PACKAGE_NAME))); +} + +TEST(MESSAGE_TEST_NAME, ParseFailsIfSubmessageTruncated) { + UNITTEST::NestedTestAllTypes o, p; + constexpr int kDepth = 5; + auto* child = o.mutable_child(); + for (int i = 0; i < kDepth; i++) { + child = child->mutable_child(); + } + TestUtil::SetAllFields(child->mutable_payload()); + + std::string serialized; + EXPECT_TRUE(o.SerializeToString(&serialized)); + + // Should parse correctly. + EXPECT_TRUE(p.ParseFromString(serialized)); + + constexpr int kMaxTruncate = 50; + ASSERT_GT(serialized.size(), kMaxTruncate); + + for (int i = 1; i < kMaxTruncate; i += 3) { + EXPECT_FALSE( + p.ParseFromString(serialized.substr(0, serialized.size() - i))); + } +} + +TEST(MESSAGE_TEST_NAME, ParseFailsIfWireMalformed) { + UNITTEST::NestedTestAllTypes o, p; + constexpr int kDepth = 5; + auto* child = o.mutable_child(); + for (int i = 0; i < kDepth; i++) { + child = child->mutable_child(); + } + // -1 becomes \xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x1 + child->mutable_payload()->set_optional_int32(-1); + + std::string serialized; + EXPECT_TRUE(o.SerializeToString(&serialized)); + + // Should parse correctly. + EXPECT_TRUE(p.ParseFromString(serialized)); + + // Overwriting the last byte to 0xFF results in malformed wire. + serialized[serialized.size() - 1] = 0xFF; + EXPECT_FALSE(p.ParseFromString(serialized)); +} + +TEST(MESSAGE_TEST_NAME, ParseFailsIfOneofWireMalformed) { + UNITTEST::NestedTestAllTypes o, p; + constexpr int kDepth = 5; + auto* child = o.mutable_child(); + for (int i = 0; i < kDepth; i++) { + child = child->mutable_child(); + } + // -1 becomes \xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x1 + child->mutable_payload()->mutable_oneof_nested_message()->set_bb(-1); + + std::string serialized; + EXPECT_TRUE(o.SerializeToString(&serialized)); + + // Should parse correctly. + EXPECT_TRUE(p.ParseFromString(serialized)); + + // Overwriting the last byte to 0xFF results in malformed wire. + serialized[serialized.size() - 1] = 0xFF; + EXPECT_FALSE(p.ParseFromString(serialized)); +} + +TEST(MESSAGE_TEST_NAME, ParseFailsIfExtensionWireMalformed) { + UNITTEST::TestChildExtension o, p; + auto* m = o.mutable_optional_extension()->MutableExtension( + UNITTEST::optional_nested_message_extension); + + // -1 becomes \xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x1 + m->set_bb(-1); + + std::string serialized; + EXPECT_TRUE(o.SerializeToString(&serialized)); + + // Should parse correctly. + EXPECT_TRUE(p.ParseFromString(serialized)); + + // Overwriting the last byte to 0xFF results in malformed wire. + serialized[serialized.size() - 1] = 0xFF; + EXPECT_FALSE(p.ParseFromString(serialized)); +} + +TEST(MESSAGE_TEST_NAME, Swap) { + UNITTEST::NestedTestAllTypes o; + constexpr int kDepth = 5; + auto* child = o.mutable_child(); + for (int i = 0; i < kDepth; i++) { + child = child->mutable_child(); + } + TestUtil::SetAllFields(child->mutable_payload()); + + std::string serialized; + EXPECT_TRUE(o.SerializeToString(&serialized)); + + { + Arena arena; + UNITTEST::NestedTestAllTypes* p1 = + Arena::CreateMessage(&arena); + + // Should parse correctly. + EXPECT_TRUE(p1->ParseFromString(serialized)); + + UNITTEST::NestedTestAllTypes* p2 = + Arena::CreateMessage(&arena); + + p1->Swap(p2); + + EXPECT_EQ(o.SerializeAsString(), p2->SerializeAsString()); + } +} + TEST(MESSAGE_TEST_NAME, BypassInitializationCheckOnParse) { UNITTEST::TestRequired message; io::ArrayInputStream raw_input(nullptr, 0); diff --git a/src/google/protobuf/unittest.proto b/src/google/protobuf/unittest.proto index 75b65f0249..032a880524 100644 --- a/src/google/protobuf/unittest.proto +++ b/src/google/protobuf/unittest.proto @@ -364,6 +364,12 @@ message TestNestedExtension { } } +message TestChildExtension { + optional string a = 1; + optional string b = 2; + optional TestAllExtensions optional_extension = 3; +} + // We have separate messages for testing required fields because it's // annoying to have to fill in required fields in TestProto in order to // do anything with it. Note that we don't need to test every type of diff --git a/src/google/protobuf/wire_format_unittest.cc b/src/google/protobuf/wire_format_unittest.cc index e75fc316f8..68b42e0454 100644 --- a/src/google/protobuf/wire_format_unittest.cc +++ b/src/google/protobuf/wire_format_unittest.cc @@ -641,7 +641,7 @@ void SerializeReverseOrder(const unittest::TestMessageSetExtension1& message, io::CodedOutputStream* coded_output) { WireFormatLite::WriteTag(15, // i WireFormatLite::WIRETYPE_VARINT, coded_output); - coded_output->WriteVarint32(message.i()); + coded_output->WriteVarint64(message.i()); WireFormatLite::WriteTag(16, // recursive WireFormatLite::WIRETYPE_LENGTH_DELIMITED, coded_output); @@ -692,6 +692,66 @@ TEST(WireFormatTest, ParseMessageSetWithDeepRecReverseOrder) { EXPECT_FALSE(message_set.ParseFromString(data)); } +TEST(WireFormatTest, ParseFailMalformedMessageSet) { + constexpr int kDepth = 5; + std::string data; + { + proto2_wireformat_unittest::TestMessageSet message_set; + proto2_wireformat_unittest::TestMessageSet* mset = &message_set; + for (int i = 0; i < kDepth; i++) { + auto m = mset->MutableExtension( + unittest::TestMessageSetExtension1::message_set_extension); + m->set_i(i); + mset = m->mutable_recursive(); + } + auto m = mset->MutableExtension( + unittest::TestMessageSetExtension1::message_set_extension); + // -1 becomes \xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x1 + m->set_i(-1); + + EXPECT_TRUE(message_set.SerializeToString(&data)); + // Make the proto mal-formed. + data[data.size() - 2 - kDepth] = 0xFF; + } + + proto2_wireformat_unittest::TestMessageSet message_set; + EXPECT_FALSE(message_set.ParseFromString(data)); +} + +TEST(WireFormatTest, ParseFailMalformedMessageSetReverseOrder) { + constexpr int kDepth = 5; + std::string data; + { + proto2_wireformat_unittest::TestMessageSet message_set; + proto2_wireformat_unittest::TestMessageSet* mset = &message_set; + for (int i = 0; i < kDepth; i++) { + auto m = mset->MutableExtension( + unittest::TestMessageSetExtension1::message_set_extension); + m->set_i(i); + mset = m->mutable_recursive(); + } + auto m = mset->MutableExtension( + unittest::TestMessageSetExtension1::message_set_extension); + // -1 becomes \xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\x1 + m->set_i(-1); + // SerializeReverseOrder() assumes "recursive" is always present. + m->mutable_recursive(); + + message_set.ByteSizeLong(); + + // Serialize with reverse payload tag order + io::StringOutputStream output_stream(&data); + io::CodedOutputStream coded_output(&output_stream); + SerializeReverseOrder(message_set, &coded_output); + } + + // Make varint for -1 malformed. + data[data.size() - 5 * (kDepth + 1) - 4] = 0xFF; + + proto2_wireformat_unittest::TestMessageSet message_set; + EXPECT_FALSE(message_set.ParseFromString(data)); +} + TEST(WireFormatTest, ParseBrokenMessageSet) { proto2_wireformat_unittest::TestMessageSet message_set; std::string input("goodbye"); // Invalid wire format data.