ARC `BAD_ACCESS` when using category method of NSString

Question

I call a utility method of mine like so:

NSDateFormatter *dateFormat = [[NSDateFormatter alloc] init];
[dateFormat setDateFormat:@"dd.MM.yy HH:mm"];
NSString *dateString = [dateFormat stringFromDate:[NSDate date]];

return [[Environment sharedInstance].versionLabelFormat replaceTokensWithStrings:
     @"VERSION", APP_VERSION, 
     @"BUILD", APP_BULD_NUMBER, 
     @"DATETIME" , dateString, 
     nil ];

This is the NSString category method

-(NSString *)replaceTokensWithStrings:(NSString *)firstKey, ... NS_REQUIRES_NIL_TERMINATION{

    NSString *result = self;

        va_list _arguments;
        va_start(_arguments, firstKey);

        for (NSString *key = firstKey; key != nil; key = va_arg(_arguments, NSString*)) {

            // The value has to be copied to prevent crashes
            NSString *value = [(NSString *)(va_arg(_arguments, NSString*))copy];

            if(!value){
                // Every key has to have a value pair otherwise the replacement is invalid and nil is returned

                NSLog(@"Premature occurence of nil. Each token must be accompanied by a value: %@", result);
                return nil;
            }

            result = [result replaceToken:key withString:value];
        }
        va_end(_arguments);

    // Check if there are any tokens which were not yet replaced (for example if one value was nil)

    if([result rangeOfString:@"{"].location == NSNotFound){
        return result;
    } else {
        NSLog(@"Failed to replace tokens failed string still contains tokens: %@", result);
        return nil;
    }
}

No on the following line I had to add a copy statement otherwise there would be a Zombie with the dateString:

NSString *value = [(NSString *)(va_arg(_arguments, NSString*))copy];

To be more specific the Zombie Report told me this:

 1 Malloc       NSDateFormatter stringForObjectValue:
   Autorelease  NSDateFormatter stringForObjectValue:
 2 CFRetain     MyClass versionString:
 3 CFRetain     replaceToken:withString:
 2 CFRelease    replaceToken:withString:
 1 CFRelease    replaceTokensWithStrings:   ( One release too much!)
 0 CFRelease    MyClass versionString:
-1 Zombie       GSEventRunModal

Although the copy statement seems to fix the problem I would like to understand what is not ARC-complient with the code so that the BAD_ACCESS would occur without the copy for the value string.

Answer 1

As others have stated the problem lies in the way you retrieve objects from the variable argument list which might not be compatible with ARC.

va_arg has a funny way of how to return a value of a specific type which ARC is probably not aware of. I'm not sure if this is a bug in clang or if it is the intended behavior for ARC. I'll clarify this issue and will update the post accordingly.

As a workaround just avoid the problem by using void pointers in the argument handling and convert them to objects properly in an ARC safe way:

for (NSString *key = firstKey; key != nil; key = (__bridge NSString *)va_arg(_arguments, void *)) {
    NSString *value = (__bridge NSString *)va_arg(_arguments, void *);
    NSAssert(value != NULL, @"Premature occurence of nil.");
    result = [result stringByReplacingToken:key
                                 withString:value];
}

Edit: The __bridge cast tells ARC to not do something about ownership. It just expects the object to be alive and does not transfer or give up ownership. Nevertheless the key and value variables maintain strong references to the objects while in use.

Second Edit: It seems that clang/ARC should be aware of the type in va_arg and either warn or just do the right thing (see this, for example).

I tried to reproduce your problem without success. Everything works for me on:

$ clang --version
> Apple clang version 4.0 (tags/Apple/clang-421.10.48) (based on LLVM 3.1svn)

Which Xcode version do you use?

Answer 2

It's a bug in clang.

It should work

ARC is compatible with variable argument lists. If it wasn't you would get an error from the compiler.

The variable value is a strong reference whereas the result of va_arg(_arguments, NSString *) is an unsafe unretained reference: you may write va_arg(_arguments, __unsafed_unretained NSString *) and get the exact same compiled assembly but trying with another ownership-qualifier will get you a compiler error as it's not supported.

So, when storing the value in value and assuming the variable is actually used, the compiler should emit a call to objc_retain and balance it with a call to objc_release when the variable is destructed. According to the report, the second call is emitted whereas the first one is missing.

This lead to a crash with the string returned by stringWithDate: (and only this one) because it's the only string that isn't constant. All the other parameters are constant strings generated by the compiler, which simply ignore any memory management method, because they persist in memory as long as the executable is loaded.

So, we need to understand why the compiler emit a release without the corresponding retain. As you don't perform any manual memory management and don't trick the ownership rules by casting with __bridge_transfer or __bridge_retained, we can assume that the issue comes from the compiler.

Undefined behavior isn't the reason

There are two reasons that may cause the compiler to emit invalid assembly. Either you code contains an undefined behavior either there is a bug in the compiler.

Undefined behavior occurs when your code try to perform something that is not defined by the C Standard: when the compiler meets an undefined behavior, it is entitled to do whatever it wants. Undefined behaviors result to programs that may or may not crash. Most of the time the issue occurs at the same place as the undefined behavior but sometimes it may seem unrelated because the compilers expect undefined behaviors not to occur and rely on that expectation to perform some optimizations.

So let's see if your code contains an undefined behavior. Yes it does, as the method replaceTokensWithStrings: may call va_start and return without va_end being called (return nil inside the for loop). The C Standard explicitly states (in section 7.15.1.3) that doing so is undefined behavior.

If we replace return nil with break, your code is now valid. However that doesn't solve the problem.

Blame the compiler

Now that we eliminated all the other possible reasons, we need to face the reality. There is a bug in clang. We can see this by performing many subtle changes that produce valid compiled assembly:

• If we compile with -O0 instead of -Os, it works.
• If we compile with clang 4.1, it works.
• If we send a message to the object before the if condition, it works.
• If we replace va_arg(_arguments, NSString *) with va_arg(_arguments, id), it works. I suggest you use this workaround.