Skip to content

Commit

Permalink
[ObjC] Fix GPBUnknownField/GPBUnknownFields copy.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
thomasvl committed Aug 16, 2024
1 parent 35bd2be commit b3b9888
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 14 deletions.
10 changes: 9 additions & 1 deletion objectivec/GPBUnknownField.m
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
5 changes: 2 additions & 3 deletions objectivec/GPBUnknownFields.m
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
25 changes: 15 additions & 10 deletions objectivec/Tests/GPBUnknownFieldsTest.m
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit b3b9888

Please sign in to comment.