Skip to content

[objc] String/Data Copies #5215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 2, 2018
Merged

[objc] String/Data Copies #5215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
366b0fb
Remove stray 'return'.
thomasvl Oct 2, 2018
b10df78
Turn off ALWAYS_SEARCH_USER_PATHS.
thomasvl Oct 2, 2018
268f0b4
Copy the value when setting message/data fields.
thomasvl Oct 2, 2018
d1ee299
Add tests to confirm strings/bytes are copied.
thomasvl Oct 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions objectivec/GPBMessage.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3084,6 +3084,14 @@ static void ResolveIvarSet(__unsafe_unretained GPBFieldDescriptor *field,
result->encodingSelector = @selector(set##NAME:); \
break; \
}
#define CASE_SET_COPY(NAME) \
case GPBDataType##NAME: { \
result->impToAdd = imp_implementationWithBlock(^(id obj, id value) { \
return GPBSetRetainedObjectIvarWithFieldInternal(obj, field, [value copy], syntax); \
}); \
result->encodingSelector = @selector(set##NAME:); \
break; \
}
CASE_SET(Bool, BOOL, Bool)
CASE_SET(Fixed32, uint32_t, UInt32)
CASE_SET(SFixed32, int32_t, Int32)
Expand All @@ -3097,8 +3105,8 @@ static void ResolveIvarSet(__unsafe_unretained GPBFieldDescriptor *field,
CASE_SET(SInt64, int64_t, Int64)
CASE_SET(UInt32, uint32_t, UInt32)
CASE_SET(UInt64, uint64_t, UInt64)
CASE_SET(Bytes, id, Object)
CASE_SET(String, id, Object)
CASE_SET_COPY(Bytes)
CASE_SET_COPY(String)
CASE_SET(Message, id, Object)
CASE_SET(Group, id, Object)
CASE_SET(Enum, int32_t, Enum)
Expand Down Expand Up @@ -3178,7 +3186,7 @@ + (BOOL)resolveInstanceMethod:(SEL)sel {
// full lookup.
const GPBFileSyntax syntax = descriptor.file.syntax;
result.impToAdd = imp_implementationWithBlock(^(id obj, id value) {
return GPBSetObjectIvarWithFieldInternal(obj, field, value, syntax);
GPBSetObjectIvarWithFieldInternal(obj, field, value, syntax);
});
result.encodingSelector = @selector(setArray:);
break;
Expand Down
56 changes: 52 additions & 4 deletions objectivec/GPBUtilities.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,36 @@ void GPBMaybeClearOneof(GPBMessage *self, GPBOneofDescriptor *oneof,
//% GPBSetObjectIvarWithField(self, field, (id)value);
//%}
//%
//%PDDM-DEFINE IVAR_ALIAS_DEFN_COPY_OBJECT(NAME, TYPE)
//%// Only exists for public api, no core code should use this.
//%TYPE *GPBGetMessage##NAME##Field(GPBMessage *self,
//% TYPE$S NAME$S GPBFieldDescriptor *field) {
//%#if defined(DEBUG) && DEBUG
//% NSCAssert(DataTypesEquivalent(GPBGetFieldDataType(field),
//% GPBDataType##NAME),
//% @"Attempting to get value of TYPE from field %@ "
//% @"of %@ which is of type %@.",
//% [self class], field.name,
//% TypeToString(GPBGetFieldDataType(field)));
//%#endif
//% return (TYPE *)GPBGetObjectIvarWithField(self, field);
//%}
//%
//%// Only exists for public api, no core code should use this.
//%void GPBSetMessage##NAME##Field(GPBMessage *self,
//% NAME$S GPBFieldDescriptor *field,
//% NAME$S TYPE *value) {
//%#if defined(DEBUG) && DEBUG
//% NSCAssert(DataTypesEquivalent(GPBGetFieldDataType(field),
//% GPBDataType##NAME),
//% @"Attempting to set field %@ of %@ which is of type %@ with "
//% @"value of type TYPE.",
//% [self class], field.name,
//% TypeToString(GPBGetFieldDataType(field)));
//%#endif
//% GPBSetCopyObjectIvarWithField(self, field, (id)value);
//%}
//%

// Object types are handled slightly differently, they need to be released
// and retained.
Expand Down Expand Up @@ -483,6 +513,24 @@ static void GPBSetObjectIvarWithField(GPBMessage *self,
syntax);
}

static void GPBSetCopyObjectIvarWithField(GPBMessage *self,
GPBFieldDescriptor *field, id value);

// GPBSetCopyObjectIvarWithField is blocked from the analyzer because it flags
// a leak for the -copy even though GPBSetRetainedObjectIvarWithFieldInternal
// is marked as consuming the value. Note: For some reason this doesn't happen
// with the -retain in GPBSetObjectIvarWithField.
#if !defined(__clang_analyzer__)
// This exists only for briging some aliased types, nothing else should use it.
static void GPBSetCopyObjectIvarWithField(GPBMessage *self,
GPBFieldDescriptor *field, id value) {
if (self == nil || field == nil) return;
GPBFileSyntax syntax = [self descriptor].file.syntax;
GPBSetRetainedObjectIvarWithFieldInternal(self, field, [value copy],
syntax);
}
#endif // !defined(__clang_analyzer__)

void GPBSetObjectIvarWithFieldInternal(GPBMessage *self,
GPBFieldDescriptor *field, id value,
GPBFileSyntax syntax) {
Expand Down Expand Up @@ -1168,7 +1216,7 @@ void GPBSetDoubleIvarWithFieldInternal(GPBMessage *self,

// Aliases are function calls that are virtually the same.

//%PDDM-EXPAND IVAR_ALIAS_DEFN_OBJECT(String, NSString)
//%PDDM-EXPAND IVAR_ALIAS_DEFN_COPY_OBJECT(String, NSString)
// This block of code is generated, do not edit it directly.

// Only exists for public api, no core code should use this.
Expand Down Expand Up @@ -1197,10 +1245,10 @@ void GPBSetMessageStringField(GPBMessage *self,
[self class], field.name,
TypeToString(GPBGetFieldDataType(field)));
#endif
GPBSetObjectIvarWithField(self, field, (id)value);
GPBSetCopyObjectIvarWithField(self, field, (id)value);
}

//%PDDM-EXPAND IVAR_ALIAS_DEFN_OBJECT(Bytes, NSData)
//%PDDM-EXPAND IVAR_ALIAS_DEFN_COPY_OBJECT(Bytes, NSData)
// This block of code is generated, do not edit it directly.

// Only exists for public api, no core code should use this.
Expand Down Expand Up @@ -1229,7 +1277,7 @@ void GPBSetMessageBytesField(GPBMessage *self,
[self class], field.name,
TypeToString(GPBGetFieldDataType(field)));
#endif
GPBSetObjectIvarWithField(self, field, (id)value);
GPBSetCopyObjectIvarWithField(self, field, (id)value);
}

//%PDDM-EXPAND IVAR_ALIAS_DEFN_OBJECT(Message, GPBMessage)
Expand Down
4 changes: 2 additions & 2 deletions objectivec/ProtocolBuffers_OSX.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -891,7 +891,7 @@
C01FCF4F08A954540054247B /* Debug */ = {
isa = XCBuildConfiguration;
buildSettings = {
ALWAYS_SEARCH_USER_PATHS = YES;
ALWAYS_SEARCH_USER_PATHS = NO;
CLANG_ANALYZER_NONNULL = YES;
CLANG_ANALYZER_SECURITY_FLOATLOOPCOUNTER = YES;
CLANG_ANALYZER_SECURITY_INSECUREAPI_RAND = YES;
Expand Down Expand Up @@ -961,7 +961,7 @@
C01FCF5008A954540054247B /* Release */ = {
isa = XCBuildConfiguration;
buildSettings = {
ALWAYS_SEARCH_USER_PATHS = YES;
ALWAYS_SEARCH_USER_PATHS = NO;
CLANG_ANALYZER_NONNULL = YES;
CLANG_ANALYZER_SECURITY_FLOATLOOPCOUNTER = YES;
CLANG_ANALYZER_SECURITY_INSECUREAPI_RAND = YES;
Expand Down
4 changes: 2 additions & 2 deletions objectivec/ProtocolBuffers_iOS.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -914,7 +914,7 @@
C01FCF4F08A954540054247B /* Debug */ = {
isa = XCBuildConfiguration;
buildSettings = {
ALWAYS_SEARCH_USER_PATHS = YES;
ALWAYS_SEARCH_USER_PATHS = NO;
CLANG_ANALYZER_NONNULL = YES;
CLANG_ANALYZER_SECURITY_FLOATLOOPCOUNTER = YES;
CLANG_ANALYZER_SECURITY_INSECUREAPI_RAND = YES;
Expand Down Expand Up @@ -985,7 +985,7 @@
C01FCF5008A954540054247B /* Release */ = {
isa = XCBuildConfiguration;
buildSettings = {
ALWAYS_SEARCH_USER_PATHS = YES;
ALWAYS_SEARCH_USER_PATHS = NO;
CLANG_ANALYZER_NONNULL = YES;
CLANG_ANALYZER_SECURITY_FLOATLOOPCOUNTER = YES;
CLANG_ANALYZER_SECURITY_INSECUREAPI_RAND = YES;
Expand Down
66 changes: 66 additions & 0 deletions objectivec/Tests/GPBMessageTests+Runtime.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2491,6 +2491,72 @@ - (void)test_GPBSetMessageMapField {
XCTAssertEqualObjects(@"bar", message.mapStringString[@"foo"]);
}

- (void)test_StringFieldsCopy {
// ObjC conventions call for NSString properites to be copy, ensure
// that is done correctly and the string isn't simply retained.

Message2 *msg1 = [Message2 message];
Message2 *msg2 = [Message2 message];

GPBFieldDescriptor *fieldDesc =
[[Message2 descriptor] fieldWithNumber:Message2_FieldNumber_OptionalString];
NSMutableString *mutableStr = [NSMutableString stringWithString:@"foo"];

msg1.optionalString = mutableStr;
GPBSetMessageStringField(msg2, fieldDesc, mutableStr);

XCTAssertEqualObjects(msg1.optionalString, mutableStr);
XCTAssertEqualObjects(msg1.optionalString, @"foo");
XCTAssertTrue(msg1.optionalString != mutableStr); // Ptr comparision.

XCTAssertEqualObjects(msg2.optionalString, mutableStr);
XCTAssertEqualObjects(msg2.optionalString, @"foo");
XCTAssertTrue(msg2.optionalString != mutableStr); // Ptr comparision.

[mutableStr appendString:@"bar"];

XCTAssertNotEqualObjects(msg1.optionalString, mutableStr);
XCTAssertEqualObjects(msg1.optionalString, @"foo");
XCTAssertTrue(msg1.optionalString != mutableStr); // Ptr comparision.

XCTAssertNotEqualObjects(msg2.optionalString, mutableStr);
XCTAssertEqualObjects(msg2.optionalString, @"foo");
XCTAssertTrue(msg2.optionalString != mutableStr); // Ptr comparision.
}

- (void)test_BytesFieldsCopy {
// ObjC conventions call for NSData properites to be copy, ensure
// that is done correctly and the data isn't simply retained.

Message2 *msg1 = [Message2 message];
Message2 *msg2 = [Message2 message];

GPBFieldDescriptor *fieldDesc =
[[Message2 descriptor] fieldWithNumber:Message2_FieldNumber_OptionalBytes];
NSMutableData *mutableData = [NSMutableData dataWithData:DataFromCStr("abc")];

msg1.optionalBytes = mutableData;
GPBSetMessageBytesField(msg2, fieldDesc, mutableData);

XCTAssertEqualObjects(msg1.optionalBytes, mutableData);
XCTAssertEqualObjects(msg1.optionalBytes, DataFromCStr("abc"));
XCTAssertTrue(msg1.optionalBytes != mutableData); // Ptr comparision.

XCTAssertEqualObjects(msg2.optionalBytes, mutableData);
XCTAssertEqualObjects(msg2.optionalBytes, DataFromCStr("abc"));
XCTAssertTrue(msg2.optionalBytes != mutableData); // Ptr comparision.

[mutableData appendData:DataFromCStr("123")];

XCTAssertNotEqualObjects(msg1.optionalBytes, mutableData);
XCTAssertEqualObjects(msg1.optionalBytes, DataFromCStr("abc"));
XCTAssertTrue(msg1.optionalBytes != mutableData); // Ptr comparision.

XCTAssertNotEqualObjects(msg2.optionalBytes, mutableData);
XCTAssertEqualObjects(msg2.optionalBytes, DataFromCStr("abc"));
XCTAssertTrue(msg2.optionalBytes != mutableData); // Ptr comparision.
}

#pragma mark - Subset from from map_tests.cc

// TEST(GeneratedMapFieldTest, IsInitialized)
Expand Down