From c46340eb36e7b8a2477b99062f0dc6816c9a0bcc Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Fri, 2 Aug 2024 07:21:07 -0700 Subject: [PATCH] [ObjC] Support errors when merging unknown fields to a message. PiperOrigin-RevId: 658782615 --- objectivec/GPBMessage.h | 18 ++++++++++-- objectivec/GPBMessage.m | 14 ++++----- objectivec/GPBUnknownFields.h | 2 +- objectivec/Tests/GPBMessageTests.m | 16 ++++++---- objectivec/Tests/GPBUnknownFieldsTest.m | 39 +++++++++++++++++++++---- objectivec/Tests/GPBUtilitiesTests.m | 5 ++-- objectivec/Tests/GPBWireFormatTests.m | 4 ++- 7 files changed, 71 insertions(+), 27 deletions(-) diff --git a/objectivec/GPBMessage.h b/objectivec/GPBMessage.h index e9889925ac76..90d6991de275 100644 --- a/objectivec/GPBMessage.h +++ b/objectivec/GPBMessage.h @@ -507,13 +507,25 @@ CF_EXTERN_C_END * Merges in the data from an `GPBUnknownFields`, meaning the data from the unknown fields gets * re-parsed so any known fields will be propertly set. * - * If the intent is to replace the message's unknown fields, call `-clearUnknownFields` first. + * If the intent is to *replace* the message's unknown fields, call `-clearUnknownFields` first. + * + * Since the data from the GPBUnknownFields will always be well formed, this call will almost never + * fail. What could cause it to fail is if the GPBUnknownFields contains a field values it is + * and error for the message's schema - i.e.: if it contains a length delimited field where the + * field number for the message is defined to be a _string_ field, however the length delimited + * data provide is not a valid UTF8 string. * * @param unknownFields The unknown fields to merge the data from. * @param extensionRegistry The extension registry to use to look up extensions, can be `nil`. + * @param errorPtr An optional error pointer to fill in with a failure + * reason if the data can not be parsed. Will only be + * filled in if the data failed to be parsed. + * + * @return Boolean indicating success. errorPtr will only be fill in on failure. **/ -- (void)mergeUnknownFields:(GPBUnknownFields *)unknownFields - extensionRegistry:(nullable id)extensionRegistry; +- (BOOL)mergeUnknownFields:(GPBUnknownFields *)unknownFields + extensionRegistry:(nullable id)extensionRegistry + error:(NSError **)errorPtr; @end diff --git a/objectivec/GPBMessage.m b/objectivec/GPBMessage.m index 652c791ef0ea..384bf5b0d86c 100644 --- a/objectivec/GPBMessage.m +++ b/objectivec/GPBMessage.m @@ -1484,14 +1484,12 @@ - (void)clearUnknownFields { self.unknownFields = nil; } -- (void)mergeUnknownFields:(GPBUnknownFields *)unknownFields - extensionRegistry:(nullable id)extensionRegistry { - NSData *data = [unknownFields serializeAsData]; - if (![self mergeFromData:data extensionRegistry:extensionRegistry error:NULL]) { -#if defined(DEBUG) && DEBUG - NSAssert(0, @"Internal error within the library, failed to parse data from unknown fields."); -#endif - }; +- (BOOL)mergeUnknownFields:(GPBUnknownFields *)unknownFields + extensionRegistry:(nullable id)extensionRegistry + error:(NSError **)errorPtr { + return [self mergeFromData:[unknownFields serializeAsData] + extensionRegistry:extensionRegistry + error:errorPtr]; } - (BOOL)isInitialized { diff --git a/objectivec/GPBUnknownFields.h b/objectivec/GPBUnknownFields.h index 4f05c2b9725c..07a7af5f5107 100644 --- a/objectivec/GPBUnknownFields.h +++ b/objectivec/GPBUnknownFields.h @@ -33,7 +33,7 @@ __attribute__((objc_subclassing_restricted)) * * Note: The instance is not linked to the message, any change will not be * reflected on the message the changes have to be pushed back to the message - * with `-[GPBMessage mergeUnknownFields:error:]`. + * with `-[GPBMessage mergeUnknownFields:extensionRegistry:error:]`. **/ - (instancetype)initFromMessage:(nonnull GPBMessage *)message; diff --git a/objectivec/Tests/GPBMessageTests.m b/objectivec/Tests/GPBMessageTests.m index f3272dbaf2e2..ab5109156464 100644 --- a/objectivec/Tests/GPBMessageTests.m +++ b/objectivec/Tests/GPBMessageTests.m @@ -506,7 +506,7 @@ - (void)testDescription { GPBUnknownFields *ufs = [[[GPBUnknownFields alloc] init] autorelease]; [ufs addFieldNumber:1234 fixed32:1234]; [ufs addFieldNumber:2345 varint:54321]; - [message mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([message mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); NSString *description = [message description]; XCTAssertGreaterThan([description length], 0U); @@ -997,13 +997,17 @@ - (void)testAutocreatedUnknownFields { XCTAssertFalse([message hasOptionalNestedMessage]); GPBUnknownFields *ufs = [[[GPBUnknownFields alloc] init] autorelease]; [ufs addFieldNumber:1 varint:1]; - [message.optionalNestedMessage mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([message.optionalNestedMessage mergeUnknownFields:ufs + extensionRegistry:nil + error:NULL]); XCTAssertTrue([message hasOptionalNestedMessage]); message.optionalNestedMessage = nil; XCTAssertFalse([message hasOptionalNestedMessage]); [ufs clear]; // Also make sure merging zero length forces it to become visible. - [message.optionalNestedMessage mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([message.optionalNestedMessage mergeUnknownFields:ufs + extensionRegistry:nil + error:NULL]); XCTAssertTrue([message hasOptionalNestedMessage]); } @@ -1887,7 +1891,7 @@ - (void)testGenerateAndParseUnknownMessage { [message setUnknownFields:unknowns]; GPBUnknownFields *ufs = [[[GPBUnknownFields alloc] init] autorelease]; [ufs addFieldNumber:1234 varint:5678]; - [message mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([message mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); NSData *data = [message data]; GPBMessage *message2 = [GPBMessage parseFromData:data extensionRegistry:nil error:NULL]; XCTAssertEqualObjects(message, message2); @@ -1900,7 +1904,7 @@ - (void)testDelimitedWriteAndParseMultipleMessages { [message1 setUnknownFields:unknowns1]; GPBUnknownFields *ufs1 = [[[GPBUnknownFields alloc] init] autorelease]; [ufs1 addFieldNumber:1234 varint:5678]; - [message1 mergeUnknownFields:ufs1 extensionRegistry:nil]; + XCTAssertTrue([message1 mergeUnknownFields:ufs1 extensionRegistry:nil error:NULL]); GPBUnknownFieldSet *unknowns2 = [[[GPBUnknownFieldSet alloc] init] autorelease]; [unknowns2 mergeVarintField:789 value:987]; @@ -1910,7 +1914,7 @@ - (void)testDelimitedWriteAndParseMultipleMessages { GPBUnknownFields *ufs2 = [[[GPBUnknownFields alloc] init] autorelease]; [ufs2 addFieldNumber:2345 fixed32:6789]; [ufs2 addFieldNumber:3456 fixed32:7890]; - [message2 mergeUnknownFields:ufs2 extensionRegistry:nil]; + XCTAssertTrue([message2 mergeUnknownFields:ufs2 extensionRegistry:nil error:NULL]); NSMutableData *delimitedData = [NSMutableData data]; [delimitedData appendData:[message1 delimitedData]]; diff --git a/objectivec/Tests/GPBUnknownFieldsTest.m b/objectivec/Tests/GPBUnknownFieldsTest.m index 6c6481d480eb..c1e5e10634a8 100644 --- a/objectivec/Tests/GPBUnknownFieldsTest.m +++ b/objectivec/Tests/GPBUnknownFieldsTest.m @@ -811,7 +811,7 @@ - (void)testMessageMergeUnknowns { [group addFieldNumber:123456 varint:5432]; TestAllTypes* msg = [TestAllTypes message]; - [msg mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([msg mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); XCTAssertEqual(msg.optionalInt64, 100); XCTAssertEqual(msg.optionalFixed32, 200); XCTAssertEqual(msg.optionalFixed64, 300); @@ -829,7 +829,7 @@ - (void)testMessageMergeUnknowns { XCTAssertEqual(varint, 5432); TestEmptyMessage* emptyMessage = [TestEmptyMessage message]; - [emptyMessage mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([emptyMessage mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); GPBUnknownFields* ufs3 = [[[GPBUnknownFields alloc] initFromMessage:emptyMessage] autorelease]; XCTAssertEqualObjects(ufs3, ufs); // Round trip through an empty message got us same fields back. XCTAssertTrue(ufs3 != ufs); // But they are different objects. @@ -843,7 +843,7 @@ - (void)testRoundTripLotsOfFields { TestEmptyMessage* emptyMessage = [TestEmptyMessage parseFromData:allFieldsData error:NULL]; GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] initFromMessage:emptyMessage] autorelease]; TestAllTypes* allFields2 = [TestAllTypes message]; - [allFields2 mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([allFields2 mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); XCTAssertEqualObjects(allFields2, allFields); // Confirm that the they still all end up in unknowns when parsed into a message with extensions @@ -891,7 +891,7 @@ - (void)testMismatchedFieldTypes { // unknown fields again. { TestAllTypes* msg = [TestAllTypes message]; - [msg mergeUnknownFields:ufsWrongTypes extensionRegistry:nil]; + XCTAssertTrue([msg mergeUnknownFields:ufsWrongTypes extensionRegistry:nil error:NULL]); GPBUnknownFields* ufs2 = [[[GPBUnknownFields alloc] initFromMessage:msg] autorelease]; XCTAssertFalse(ufs2.empty); XCTAssertEqualObjects(ufs2, ufsWrongTypes); // All back as unknown fields. @@ -901,19 +901,46 @@ - (void)testMismatchedFieldTypes { // into unknown fields. { TestAllExtensions* msg = [TestAllExtensions message]; - [msg mergeUnknownFields:ufsWrongTypes extensionRegistry:[UnittestRoot extensionRegistry]]; + XCTAssertTrue([msg mergeUnknownFields:ufsWrongTypes + extensionRegistry:[UnittestRoot extensionRegistry] + error:NULL]); GPBUnknownFields* ufs2 = [[[GPBUnknownFields alloc] initFromMessage:msg] autorelease]; XCTAssertFalse(ufs2.empty); XCTAssertEqualObjects(ufs2, ufsWrongTypes); // All back as unknown fields. } } +- (void)testMergeFailures { + // Valid data, pushes to the string just fine. + { + GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] init] autorelease]; + [ufs addFieldNumber:TestAllTypes_FieldNumber_OptionalString + lengthDelimited:DataFromCStr("abc")]; + TestAllTypes* msg = [TestAllTypes message]; + NSError* error = nil; + XCTAssertTrue([msg mergeUnknownFields:ufs extensionRegistry:nil error:&error]); + XCTAssertNil(error); + XCTAssertEqualObjects(msg.optionalString, @"abc"); + } + + // Invalid UTF-8 causes a failure when pushed to the message. + { + GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] init] autorelease]; + [ufs addFieldNumber:TestAllTypes_FieldNumber_OptionalString + lengthDelimited:DataFromBytes(0xC2, 0xF2, 0x0, 0x0, 0x0)]; + TestAllTypes* msg = [TestAllTypes message]; + NSError* error = nil; + XCTAssertFalse([msg mergeUnknownFields:ufs extensionRegistry:nil error:&error]); + XCTAssertNotNil(error); + } +} + - (void)testLargeVarint { GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] init] autorelease]; [ufs addFieldNumber:1 varint:0x7FFFFFFFFFFFFFFFL]; TestEmptyMessage* emptyMessage = [TestEmptyMessage message]; - [emptyMessage mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([emptyMessage mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); GPBUnknownFields* ufsParsed = [[[GPBUnknownFields alloc] initFromMessage:emptyMessage] autorelease]; diff --git a/objectivec/Tests/GPBUtilitiesTests.m b/objectivec/Tests/GPBUtilitiesTests.m index fb0175ecca33..6db22a3d37bc 100644 --- a/objectivec/Tests/GPBUtilitiesTests.m +++ b/objectivec/Tests/GPBUtilitiesTests.m @@ -188,7 +188,7 @@ - (void)testTextFormatUnknownFields { [group addFieldNumber:3 fixed32:0x3]; [group addFieldNumber:2 fixed64:0x2]; TestEmptyMessage *message = [TestEmptyMessage message]; - [message mergeUnknownFields:ufs extensionRegistry:nil]; + XCTAssertTrue([message mergeUnknownFields:ufs extensionRegistry:nil error:NULL]); NSString *expected = @"# --- Unknown fields ---\n" @"10: 1\n" @@ -255,7 +255,8 @@ - (void)testSetRepeatedFields { static void AddUnknownFields(GPBMessage *message, int num) { GPBUnknownFields *ufs = [[GPBUnknownFields alloc] init]; [ufs addFieldNumber:num varint:num]; - [message mergeUnknownFields:ufs extensionRegistry:nil]; + // Can't fail since it is a varint. + [message mergeUnknownFields:ufs extensionRegistry:nil error:NULL]; [ufs release]; } diff --git a/objectivec/Tests/GPBWireFormatTests.m b/objectivec/Tests/GPBWireFormatTests.m index 3d32112f17e5..2df9f2d6b669 100644 --- a/objectivec/Tests/GPBWireFormatTests.m +++ b/objectivec/Tests/GPBWireFormatTests.m @@ -131,7 +131,9 @@ - (void)testSerializeMessageSet { GPBUnknownFields* group = [ufs addGroupWithFieldNumber:GPBWireFormatMessageSetItem]; [group addFieldNumber:GPBWireFormatMessageSetTypeId varint:kUnknownTypeId2]; [group addFieldNumber:GPBWireFormatMessageSetMessage lengthDelimited:DataFromCStr("baz")]; - [message_set mergeUnknownFields:ufs extensionRegistry:[MSetUnittestMsetRoot extensionRegistry]]; + XCTAssertTrue([message_set mergeUnknownFields:ufs + extensionRegistry:[MSetUnittestMsetRoot extensionRegistry] + error:NULL]); NSData* data = [message_set data];