0

I recently migrated a huge library to ARC, and a tool-free section is giving a headache. Here is the code:

+ (NSString *)getKeychainItem:(NSString *)identifier
{
    NSString *fullIdentifier = [NSString stringWithFormat:@"%@%@", kIdentifierPrefix, identifier];

    NSMutableDictionary *queryKeychain;
    OSStatus status = noErr;

    queryKeychain = [NSMutableDictionary dictionary];

    // Set the public key query dictionary.
    [queryKeychain setObject:(__bridge id)kSecClassGenericPassword
                      forKey:(__bridge id)kSecClass];

    // Get the key.
    CFDataRef data;
    CFDictionaryRef queryKeychainCF = (__bridge CFDictionaryRef)queryKeychain;
    status = SecItemCopyMatching(queryKeychainCF, (CFTypeRef *)&data);

    NSData *passwordData = (__bridge_transfer NSData *)data;

    NSString *password;

    if (status == noErr)
    {
        password = [[NSString alloc] initWithBytes:[passwordData bytes]
                                            length:[passwordData length]
                                          encoding:NSUTF8StringEncoding];

    }
    else if (status != errSecItemNotFound)
    {
        NSLog(@"Error getting keychain item %@ -- OSStatus: %lu", identifier, status);
    }

    return password;
}

This should be pretty straight forward, however, the passwordData object is being overreleased and I have no idea why, the stack trace is this. If I just set passwordData to nil and don't do the __bridge__transfer, it doesn't crash. Any Idea on why?

Thanks a lot!

Pierluigi Cifani
  • 224
  • 2
  • 14

3 Answers3

2

I haven't used __bridge_transfer myself, but if you change the "passwordData" to this:

NSData *passwordData = (NSData *)data;

XCode gives you two recommendations.

Don't transfer ownership (core foundation has to release it):

NSData *passwordData = (__bridge NSData *)data;

Transfer ownership (ARC takes over):

NSData *passwordData = (NSData *)CFBridgingRelease(data);

__bridge_transfer might be the same thing, but I've had no trouble using the CFBridgingRelease call, which is what XCode recommends.

Setting anything to nil won't actually release anything, unless ARC is managing the memory. You never want to set a Core Foundation object to nil unless you've explicitly released it with Core Foundation, or transfered ownership to ARC.

Another option you have is do CFRelease(data) just before your return and just use a normal __bridge.

This is all based on the assumption that SecItemCopyMatching is giving you a copy of the data and excepting you to release it. New and Copy are keywords that usually indicate this. You can debug further by using CFGetRetainCount(data) at different points to verify the count.

I've also noticed that fullIdentifier isn't being used. Is this the whole function?

You can also use initWithData:encoding: instead of initWithBytes:length:encoding.

Luke
  • 13,678
  • 7
  • 45
  • 79
  • I'm afraid that `CFBridgingRelease` is also failing: http://i.imgur.com/8Wi4gB7.png . I'm starting to think that `SecItemCopyMatching` smells... – Pierluigi Cifani Mar 13 '13 at 17:11
  • Try doing a regular `__bridge` and place NSLog(@"%d", CFGetRetainCount(data)) in various places. NSString might be assuming ownership of the memory. – Luke Mar 13 '13 at 17:13
  • Thanks luke, but the problem was another one, related to SecItemCopyMatching behaviour – Pierluigi Cifani Mar 13 '13 at 18:29
0

You need to cast CFDataRef to NSData try using

 NSData *passwordData = (NSData *)data;

or just use the bridge without transferring the ownership

NSData* passwordData = (__bridge NSData*) data;
nsgulliver
  • 12,655
  • 23
  • 43
  • 64
0

Turns out the problem was on another object altogether, attaching the correct code:

CFDictionaryRef queryKeychainCF = (__bridge_retained CFDictionaryRef)queryKeychain;
status = SecItemCopyMatching(queryKeychainCF, (CFTypeRef *)&data);   
NSData *passwordData = (__bridge_transfer NSData *)data;

After trying all solutions, started reading errors for the SecItemCopyMatching method under ARC and got to this answer.

Community
  • 1
  • 1
Pierluigi Cifani
  • 224
  • 2
  • 14