From 35bd2be8286c736303083d7c2230d11ab3648672 Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Wed, 14 Aug 2024 08:49:00 -0700 Subject: [PATCH 1/3] [ObjC] Raise an exception for a nil message. The api is annotated that it isn't valid, but incase someone calling code isn't annotated correct and someone returns nil for another nonnull api, it could happen, so make it an explicit failure just to be safe. PiperOrigin-RevId: 662935009 --- objectivec/GPBUnknownFields.m | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/objectivec/GPBUnknownFields.m b/objectivec/GPBUnknownFields.m index 2672d10b15b15..87e62b9a84b0a 100644 --- a/objectivec/GPBUnknownFields.m +++ b/objectivec/GPBUnknownFields.m @@ -197,6 +197,12 @@ - (instancetype)initFromMessage:(nonnull GPBMessage *)message { self = [super init]; if (self) { fields_ = [[NSMutableArray alloc] init]; + // This shouldn't happen with the annotations, but just incase something claiming nonnull + // does return nil, block it. + if (!message) { + [self release]; + [NSException raise:NSInvalidArgumentException format:@"Message cannot be nil"]; + } NSData *data = GPBMessageUnknownFieldsData(message); if (data) { GPBCodedInputStream *input = [[GPBCodedInputStream alloc] initWithData:data]; From b3b988885d91f31c9cdc058fd5249efc18a91d11 Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Thu, 15 Aug 2024 08:58:29 -0700 Subject: [PATCH 2/3] [ObjC] Fix GPBUnknownField/GPBUnknownFields copy. Since a group GPBUnknownField uses a GPBUnknownFields and that is mutable, it needs to be copied so two instances aren't linked. PiperOrigin-RevId: 663323972 --- objectivec/GPBUnknownField.m | 10 +++++++++- objectivec/GPBUnknownFields.m | 5 ++--- objectivec/Tests/GPBUnknownFieldsTest.m | 25 +++++++++++++++---------- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/objectivec/GPBUnknownField.m b/objectivec/GPBUnknownField.m index ba70dfe599304..f574acf7e062e 100644 --- a/objectivec/GPBUnknownField.m +++ b/objectivec/GPBUnknownField.m @@ -165,9 +165,17 @@ - (id)copyWithZone:(NSZone *)zone { case GPBUnknownFieldTypeFixed32: case GPBUnknownFieldTypeFixed64: case GPBUnknownFieldTypeLengthDelimited: - case GPBUnknownFieldTypeGroup: // In these modes, the object isn't mutable, so just return self. return [self retain]; + case GPBUnknownFieldTypeGroup: { + // The `GPBUnknownFields` for the group is mutable, so a new instance of this object and + // the group is needed. + GPBUnknownFields *copyGroup = [storage_.group copyWithZone:zone]; + GPBUnknownField *copy = [[GPBUnknownField allocWithZone:zone] initWithNumber:number_ + group:copyGroup]; + [copyGroup release]; + return copy; + } case GPBUnknownFieldTypeLegacy: { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wdeprecated-declarations" diff --git a/objectivec/GPBUnknownFields.m b/objectivec/GPBUnknownFields.m index 87e62b9a84b0a..890be4286ed18 100644 --- a/objectivec/GPBUnknownFields.m +++ b/objectivec/GPBUnknownFields.m @@ -228,9 +228,8 @@ - (instancetype)init { } - (id)copyWithZone:(NSZone *)zone { - GPBUnknownFields *copy = [[GPBUnknownFields alloc] init]; - // Fields are r/o in this api, so just copy the array. - copy->fields_ = [fields_ mutableCopyWithZone:zone]; + GPBUnknownFields *copy = [[GPBUnknownFields allocWithZone:zone] init]; + copy->fields_ = [[NSMutableArray allocWithZone:zone] initWithArray:fields_ copyItems:YES]; return copy; } diff --git a/objectivec/Tests/GPBUnknownFieldsTest.m b/objectivec/Tests/GPBUnknownFieldsTest.m index 42c3f7dc87e6d..c73e0649c1118 100644 --- a/objectivec/Tests/GPBUnknownFieldsTest.m +++ b/objectivec/Tests/GPBUnknownFieldsTest.m @@ -683,25 +683,30 @@ - (void)testCopy { [ufs addFieldNumber:3 fixed64:3]; [ufs addFieldNumber:4 lengthDelimited:DataFromCStr("foo")]; GPBUnknownFields* group = [ufs addGroupWithFieldNumber:5]; + [group addFieldNumber:10 varint:10]; + GPBUnknownFields* subGroup = [group addGroupWithFieldNumber:100]; + [subGroup addFieldNumber:20 varint:20]; GPBUnknownFields* ufs2 = [[ufs copy] autorelease]; XCTAssertTrue(ufs != ufs2); // Different objects XCTAssertEqualObjects(ufs, ufs2); // Equal contents - // All the actual field objects should be the same since they are immutable. + // All field objects but the group should be the same since they are immutable. XCTAssertTrue([[ufs fields:1] firstObject] == [[ufs2 fields:1] firstObject]); // Same object XCTAssertTrue([[ufs fields:2] firstObject] == [[ufs2 fields:2] firstObject]); // Same object XCTAssertTrue([[ufs fields:3] firstObject] == [[ufs2 fields:3] firstObject]); // Same object XCTAssertTrue([[ufs fields:4] firstObject] == [[ufs2 fields:4] firstObject]); // Same object XCTAssertTrue([[ufs fields:4] firstObject].lengthDelimited == - [[ufs2 fields:4] firstObject].lengthDelimited); // Same object - XCTAssertTrue([[ufs fields:5] firstObject] == [[ufs2 fields:5] firstObject]); // Same object - XCTAssertTrue(group == [[ufs2 fields:5] firstObject].group); // Same object - - // Now force copies on the fields to confirm that is not making new objects either. - for (GPBUnknownField* field in ufs) { - GPBUnknownField* field2 = [[field copy] autorelease]; - XCTAssertTrue(field == field2); // Same object (since they aren't mutable). - } + [[ufs2 fields:4] firstObject].lengthDelimited); // Same object + // Since the group holds another `GPBUnknownFields` object (which is mutable), it will be a + // different object. + XCTAssertTrue([[ufs fields:5] firstObject] != [[ufs2 fields:5] firstObject]); + XCTAssertTrue(group != [[ufs2 fields:5] firstObject].group); + XCTAssertEqualObjects(group, [[ufs2 fields:5] firstObject].group); + // And confirm that copy went deep so the nested group also is a different object. + GPBUnknownFields* groupCopied = [[ufs2 fields:5] firstObject].group; + XCTAssertTrue([[group fields:100] firstObject] != [[groupCopied fields:100] firstObject]); + XCTAssertTrue(subGroup != [[groupCopied fields:100] firstObject].group); + XCTAssertEqualObjects(subGroup, [[groupCopied fields:100] firstObject].group); } - (void)testInvalidFieldNumbers { From 0790ab4d7a771d4ca79fa795cc96febfbb43ebb6 Mon Sep 17 00:00:00 2001 From: Thomas Van Lenten Date: Thu, 15 Aug 2024 13:14:15 -0700 Subject: [PATCH 3/3] [ObjC] Add api to add a field to another collection of unknown fields. PiperOrigin-RevId: 663424215 --- objectivec/GPBUnknownFields.h | 17 ++++++++++++++ objectivec/GPBUnknownFields.m | 10 ++++++++ objectivec/Tests/GPBUnknownFieldsTest.m | 31 +++++++++++++++++++++++++ 3 files changed, 58 insertions(+) diff --git a/objectivec/GPBUnknownFields.h b/objectivec/GPBUnknownFields.h index 07a7af5f51078..859614b6453a1 100644 --- a/objectivec/GPBUnknownFields.h +++ b/objectivec/GPBUnknownFields.h @@ -109,6 +109,23 @@ __attribute__((objc_subclassing_restricted)) **/ - (nonnull GPBUnknownFields *)addGroupWithFieldNumber:(int32_t)fieldNumber; +/** + * Add the copy of the given unknown field. + * + * This can be useful from processing one `GPBUnknownFields` to create another. + * + * NOTE: If the field being copied is an Group, this instance added is new and thus + * the `.group` of that result is also new, so if you intent is to modify the group + * it *must* be fetched out of the result. + * + * It is a programming error to call this when the `type` is a legacy field. + * + * @param field The field to add. + * + * @return The autoreleased field that was added. + **/ +- (GPBUnknownField *)addCopyOfField:(nonnull GPBUnknownField *)field; + /** * Removes the given field from the set. * diff --git a/objectivec/GPBUnknownFields.m b/objectivec/GPBUnknownFields.m index 890be4286ed18..dc004b3fb8999 100644 --- a/objectivec/GPBUnknownFields.m +++ b/objectivec/GPBUnknownFields.m @@ -323,6 +323,16 @@ - (GPBUnknownFields *)addGroupWithFieldNumber:(int32_t)fieldNumber { return [group autorelease]; } +- (GPBUnknownField *)addCopyOfField:(nonnull GPBUnknownField *)field { + if (field->type_ == GPBUnknownFieldTypeLegacy) { + [NSException raise:NSInternalInconsistencyException + format:@"GPBUnknownField is the wrong type"]; + } + GPBUnknownField *result = [field copy]; + [fields_ addObject:result]; + return [result autorelease]; +} + - (void)removeField:(nonnull GPBUnknownField *)field { NSUInteger count = fields_.count; [fields_ removeObjectIdenticalTo:field]; diff --git a/objectivec/Tests/GPBUnknownFieldsTest.m b/objectivec/Tests/GPBUnknownFieldsTest.m index c73e0649c1118..2b8a085072e03 100644 --- a/objectivec/Tests/GPBUnknownFieldsTest.m +++ b/objectivec/Tests/GPBUnknownFieldsTest.m @@ -662,6 +662,37 @@ - (void)testFastEnumeration { XCTAssertEqual(loop, 10); } +- (void)testAddCopyOfField { + GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] init] autorelease]; + [ufs addFieldNumber:1 varint:10]; + [ufs addFieldNumber:2 fixed32:11]; + [ufs addFieldNumber:3 fixed64:12]; + [ufs addFieldNumber:4 lengthDelimited:DataFromCStr("foo")]; + GPBUnknownFields* group = [ufs addGroupWithFieldNumber:5]; + [group addFieldNumber:10 varint:100]; + GPBUnknownFields* subGroup = [group addGroupWithFieldNumber:100]; + [subGroup addFieldNumber:50 varint:50]; + + GPBUnknownFields* ufs2 = [[[GPBUnknownFields alloc] init] autorelease]; + for (GPBUnknownField* field in ufs) { + GPBUnknownField* field2 = [ufs2 addCopyOfField:field]; + XCTAssertEqualObjects(field, field2); + if (field.type == GPBUnknownFieldTypeGroup) { + // Group does a copy because the `.group` value is mutable. + XCTAssertTrue(field != field2); // Pointer comparison. + XCTAssertTrue(group != field2.group); // Pointer comparison. + XCTAssertEqualObjects(group, field2.group); + GPBUnknownFields* subGroupAdded = [field2.group firstGroup:100]; + XCTAssertTrue(subGroupAdded != subGroup); // Pointer comparison. + XCTAssertEqualObjects(subGroupAdded, subGroup); + } else { + // All other types are immutable, so they use the same object. + XCTAssertTrue(field == field2); // Pointer comparision. + } + } + XCTAssertEqualObjects(ufs, ufs2); +} + - (void)testDescriptions { // Exercise description for completeness. GPBUnknownFields* ufs = [[[GPBUnknownFields alloc] init] autorelease];