2

I have a crash, very likely related to memory management that I can not spot.

Crash never happened on me, I only know it's occurring because of crash reports I have received.
This also means the only current way I have to confirm crash is fixed is to ship the app and wait for crash reports to come in (bad news) or not (happy me!).

Extract of crash report:

Exception Type:  SIGSEGV
Exception Codes: SEGV_ACCERR at 0x9
Crashed Thread:  0

Thread 0 Crashed:
0   CoreFoundation                      0x375f29e8 0x375e4000 + 59880
1   MyApp                            0x000bf22f -[UIViewController(AddressPicker) fullNameAndAddressFromPerson:identifier:] (UIViewController+AddressPicker.m:108)
2   MyApp                            0x000bf3f9 -[UIViewController(AddressPicker) guessedUserAddress] (UIViewController+AddressPicker.m:161)
3   MyApp                            0x0009cc99 -[RecipientsViewController loadUserAddressFromMyAppSender] (RecipientsViewController.m:190)
4   MyApp                            0x0009c189 -[RecipientsViewController viewDidLoad] (RecipientsViewController.m:78)
5   UIKit                               0x31a5e541 0x31a3c000 + 140609

Original Code:

- (NSString *) fullNameAndAddressFromPerson:(ABRecordRef) person identifier:(ABMultiValueIdentifier) identifier {
    NSMutableString *address = [NSMutableString new];

    // Get and add first and last name
    CFStringRef cfFirstName = ABRecordCopyValue(person, kABPersonFirstNameProperty);
    NSString *firstName = (NSString *)CFBridgingRelease(cfFirstName);

    CFStringRef cfLastName = ABRecordCopyValue(person, kABPersonLastNameProperty);
    NSString *lastName = (NSString *)CFBridgingRelease(cfLastName);

    [address appendFormat:@"%@ %@\n", firstName ?: @"", lastName ?: @""];


    // Get and add address
    ABMultiValueRef addressMultiValue = ABRecordCopyValue(person, kABPersonAddressProperty);

    CFTypeRef addressRef = ABMultiValueCopyValueAtIndex(addressMultiValue, ABMultiValueGetIndexForIdentifier(addressMultiValue, identifier)); // This is line 108
    CFRelease(addressMultiValue);

    NSDictionary *addressDictionary = (NSDictionary *) (CFBridgingRelease(addressRef));

    if ([addressDictionary isKindOfClass:NSDictionary.class]) {
        [address appendString:ABCreateStringWithAddressDictionary(addressDictionary, YES)];
    }

    return [address stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceCharacterSet]];
}

Updated code, including checks suggested by Nirav:

- (NSString *) fullNameAndAddressFromPerson:(ABRecordRef) person identifier:(ABMultiValueIdentifier) identifier {
    NSMutableString *address = [NSMutableString new];
    NSString *firstName, *lastName;

    // Get and add first and last name
    if (person) {
        CFStringRef cfFirstName = ABRecordCopyValue(person, kABPersonFirstNameProperty);
        if (cfFirstName) {
            firstName = (NSString *)CFBridgingRelease(cfFirstName);
        }

        CFStringRef cfLastName = ABRecordCopyValue(person, kABPersonLastNameProperty);

        if (cfLastName) {
            lastName = (NSString *)CFBridgingRelease(cfLastName);
        }
    }

    [address appendFormat:@"%@ %@\n", firstName ?: @"", lastName ?: @""];


    // Get and add address
    ABMultiValueRef addressMultiValue = ABRecordCopyValue(person, kABPersonAddressProperty);

    CFTypeRef addressRef;
    if (addressMultiValue && identifier) {
        addressRef = ABMultiValueCopyValueAtIndex(addressMultiValue, ABMultiValueGetIndexForIdentifier(addressMultiValue, identifier));
        CFRelease(addressMultiValue);
    }

    NSDictionary *addressDictionary;

    if (addressRef) {
        addressDictionary = (NSDictionary *) (CFBridgingRelease(addressRef));

        if ([addressDictionary isKindOfClass:NSDictionary.class]) {
            [address appendString:ABCreateStringWithAddressDictionary(addressDictionary, YES)];
        }
    }

    return [address stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceCharacterSet]];
}

I guess I'm missing the obvious, so instead of me just replying "duh, you are right", I will accept the answer that likely fix the bug and point out ways this code can be made safer and improved.

jv42
  • 8,521
  • 5
  • 40
  • 64
Guillaume
  • 21,685
  • 6
  • 63
  • 95
  • BTW - it shows the error is on line 108 on the file this code is posted from. – Nirav Bhatt Feb 01 '13 at 22:02
  • @NiravBhatt Yep, that's why I added a comment stating "This is line 108" – Guillaume Feb 01 '13 at 22:16
  • Then it is. addressMultiValue or identifier, one of them got to be null ie identifier maybe out of range or addressMultiValue is nil. – Nirav Bhatt Feb 01 '13 at 22:17
  • I believe instead of (addressMultiValue && identifier) - safer thing would be ((addressMultiValue != nil) && (identifier != nil)). Reason - Hex address values may make it nonzero so if (identifier) will evaluate to true. – Nirav Bhatt Feb 01 '13 at 23:05
  • Sorry, I disagree with last comment. You can argue your version is easier to read (more explicit), and depending of context, I sometimes use it. But nil is a pointer to the address 0x0. – Guillaume Feb 01 '13 at 23:19
  • You are correct - http://stackoverflow.com/questions/4130325/nil-in-gdb-is-not-defined-as-0x0. Don't know what made me think of this, probably some old bad compiler woes from other language :-( BTW does this make a case for marking my answer as correct? – Nirav Bhatt Feb 01 '13 at 23:50
  • What line is 108 in your original code? – Christopher Pickslay Feb 02 '13 at 22:27
  • @ChristopherPickslay the one ending with '// This is line 108' :-p – Guillaume Feb 02 '13 at 23:01
  • Is it possible the `identifier` passed into this method is NULL or invalid? – Christopher Pickslay Feb 04 '13 at 18:10
  • `identifier` is a ABMultiValueIdentifier, which is typedefined to a int32_t. 0 is a possible and acceptable value. – Guillaume Feb 05 '13 at 10:07

1 Answers1

1

I faced similar crash doing similar thing and here is what I did:

  • Do null check for every CFStringRef before using them - here: cfFirstName, cfLastName
  • Do null check for every ABMultiValueRef being returned - here: addressMultiValue

And so on. The gist is to perform null check before bridging macros.

This is to handle phonebook records that don't have certain fields populated, that's it.

Nirav Bhatt
  • 6,940
  • 5
  • 45
  • 89
  • 1
    Good point. Did you check only against nil, or can you get other invalid value (NSNull ?) – Guillaume Feb 01 '13 at 22:22
  • Did you find a way to create a phonebook record that show this problem? – Guillaume Feb 01 '13 at 22:23
  • Just create the entry that has missing address first line. When you supply identifier = 0 (or equivalent, I don't remember exactly) you will see this issue. Ditto for firstname,lastname etc. – Nirav Bhatt Feb 01 '13 at 22:24
  • "entry that has missing address first line" => did not crash either :( – Guillaume Feb 01 '13 at 22:34
  • It depends on the id that gets passed. addressMultiValue acts like an array trying to locate identifier entry. See this: http://blog.slaunchaman.com/2009/01/21/cocoa-touch-tutorial-extract-address-book-address-values-on-iphone-os/ – Nirav Bhatt Feb 01 '13 at 22:40