Search code examples
objective-cmacosstatic-analysiskeychain

API Misuse (Apple): Trying to free data which has not been allocated


An app I'm working on works with simple password items in the Login Keychain. I noticed there is a SecKeychainItemRef that is never freed. The official documentation on SecKeychainFindGenericPassword() reads:

On return, a pointer to the item object of the generic password. You are responsible for releasing your reference to this object.

After some quick changes, the static code analyzer claims:

  1. Assuming 'result' is equal to noErr
  2. Assuming 'item' is non-null
  3. Trying to free data which has not been allocated

I'm at a loss at how I'm assuming result is equal to noErr as there is an else clause. Not quite sure where I am assuming item is non-null nor how I am freeing data that is not allocated since it's checked (if (item)).

Screenshot of the warnings and code listing

Screenshot of the warning and code listing

Code Listings

This is a part of the Hermes project on GitHub.

Old

BOOL KeychainSetItem(NSString* username, NSString* password) {
  SecKeychainItemRef item;
  OSStatus result = SecKeychainFindGenericPassword(
    NULL,
    strlen(KEYCHAIN_SERVICE_NAME),
    KEYCHAIN_SERVICE_NAME,
    [username length],
    [username UTF8String],
    NULL,
    NULL,
    &item);

  if (result == noErr) {
    result = SecKeychainItemModifyContent(item, NULL, [password length],
                                          [password UTF8String]);
    return result == noErr;
  } else {
    result = SecKeychainAddGenericPassword(
      NULL,
      strlen(KEYCHAIN_SERVICE_NAME),
      KEYCHAIN_SERVICE_NAME,
      [username length],
      [username UTF8String],
      [password length],
      [password UTF8String],
      NULL);

    return result == noErr;
  }
}

New

BOOL KeychainSetItem(NSString* username, NSString* password) {
  SecKeychainItemRef item = nil;
  OSStatus result = SecKeychainFindGenericPassword(
    NULL,
    strlen(KEYCHAIN_SERVICE_NAME),
    KEYCHAIN_SERVICE_NAME,
    [username length],
    [username UTF8String],
    NULL,
    NULL,
    &item);

  if (result == noErr) {
    result = SecKeychainItemModifyContent(item, NULL, [password length],
                                          [password UTF8String]);
  } else {
    result = SecKeychainAddGenericPassword(
      NULL,
      strlen(KEYCHAIN_SERVICE_NAME),
      KEYCHAIN_SERVICE_NAME,
      [username length],
      [username UTF8String],
      [password length],
      [password UTF8String],
      NULL);
  }

  if (item) {
    SecKeychainItemFreeContent(NULL, item);
  }
  return result == noErr;
}

Solution

  • SecKeychainItemRef variables are CoreFoundation reference counted. From the static code analyzer's point of view, calling SecKeychainItemFreeContent() on a SecKeychainItemRef amounts to freeing unallocated data since it was not allocated by the SecKeychain functions.

    All the errors went away by calling CFRelease() on the SecKeychainItemRef variable (instead of SecKeychainItemFreeContent()).