Memory Leak in iOS KeychainItemWrapper

Question

I'm using the KeyChainItemWrapper from Apple's sample code to store user password for authentication, but when I call it to set the password:

   [keychain setObject:passwordField.text forKey:(id)kSecValueData];

It dribbles memory leaks all over my shirt. The problem apparently traces back to line 274 in KeyChainItemWrapper.m, which is this:

if (SecItemCopyMatching((CFDictionaryRef)genericPasswordQuery, (CFTypeRef *)&attributes) == noErr)
{

How would I fix this, and should I be more careful when working with Apple sample code in the future?

Note: I could post more code, but I've narrowed the problem down to this line using Instruments and the full sample code is readily available to any developer.

Kadner

Answer 1

Looking at the code for KeyChainItemWrapper, I'd agree that this line is a memory leak. They missed the [attributes release] at the end of writeToKeychain. See all the other calls to SecItemCopyMatching() in this file for examples of how they correctly release the returned-by-reference object.

I would use the "It's good, but..." link at the bottom of this page to note the error.

Bibulous

That was exactly where the problem was. Just add [attributes release]; to the end of the function plugs that link. This bug has been reported to Apple as you suggested. – Kadner

Wow - as far as sample code goes, I think KeychainItemWrapper is some of the worst out there! autoreleases everywhere, at least 2 known memory leaks, including the above, and another one initialising keychainItemData ... – Squalor

npellow, what's wrong with autoreleases? They just release at some point. Apple uses them frequently. In fact, its how they want us to program – Rebba

Answer 2

Static analysis is reporting a potential leak of on object in method resetKeychainItem, line 191, of KeyChainItemWrapper.m. Surprisingly, it does not report a potential leak in the area addressed above, although I did add release of the object as suggested, and for correctness.

Here is the code with the leak being reported:

- (void)resetKeychainItem
{
    ...
    // Default attributes for keychain item.
    [keychainItemData setObject:@"" forKey:(id)kSecAttrAccount]; // <-- Potential leak of an object
    [keychainItemData setObject:@"" forKey:(id)kSecAttrLabel];
    [keychainItemData setObject:@"" forKey:(id)kSecAttrDescription];

    // Default data for keychain item.
    [keychainItemData setObject:@"" forKey:(id)kSecValueData];
}

This issue is being reported on the empty string @"". I tried a variety of code implementations to try and "fix" this issue, but nothing appears to work.

Is this is a false positive?

Update: Right after posting I realized that I can double-click on the warning to trace the error.

This warning is attributed to the line above it for allocating the dictionary:

if (!keychainItemData)
{
    self.keychainItemData = [[NSMutableDictionary alloc] init];
}

I changed the code to the following:

if (!keychainItemData)
{
    self.keychainItemData = [[[NSMutableDictionary alloc] init] autorelease];
}

The analyzer warning is no longer present.

Perineum

Answer 3

I found another one leak in - (void) resetKeychainItem.

It should be

self.pKeychainItemData = [[[NSMutableDictionary alloc] init] autorelease];.

Dinar

why not self.pKeychainItemData = [NSMutableDictionary dictionary];? So much cleaner... – Rebba