Debugging a malloc crash within AuthorizationExecuteWithPrivileges

[kainjow]

I've got this chunk of code that calls AuthorizationExecuteWithPrivileges(). So far I've seen 5 crashes all with the same stack trace, all running as 64-bit on 10.6.x, but unreproducible on my end:

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000001029e77fe
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   libSystem.B.dylib      0x00007fff8725cf6b small_malloc_from_free_list + 912
1   libSystem.B.dylib      0x00007fff87259741 szone_malloc_should_clear + 2070
2   libSystem.B.dylib      0x00007fff87258eea malloc_zone_malloc + 82
3   libSystem.B.dylib      0x00007fff872571e8 malloc + 44
4   libSystem.B.dylib      0x00007fff8726e7f2 _pthread_work_internal_init + 180
5   libSystem.B.dylib      0x00007fff87330231 pthread_workqueue_atfork_child + 39
6   libSystem.B.dylib      0x00007fff87318ebc _cthread_fork_child + 180
7   libSystem.B.dylib      0x00007fff872b8923 fork + 83
8   com.mycompany.myapp    0x000000010004d648 AuthorizationExecuteWithPrivileges
...

+ (OSStatus)launchAuthorizedTaskForPath:(NSString *)taskPath arguments:(NSArray *)arguments output:(NSString **)output
{
    if (!taskPath || ![[NSFileManager defaultManager] fileExistsAtPath:taskPath])
        return errAuthorizationToolExecuteFailure;
    
    AuthorizationRef authorizationRef = NULL;
    OSStatus status = AuthorizationCreate(NULL, kAuthorizationEmptyEnvironment, kAuthorizationFlagDefaults, &authorizationRef);
    if (status != errAuthorizationSuccess)
        return status;
    
    const char *path = [taskPath fileSystemRepresentation];    
    AuthorizationItem right = {kAuthorizationRightExecute, strlen(path), (void *)path, kAuthorizationFlagDefaults};
    AuthorizationRights rightSet = {1, &right};
    AuthorizationFlags flags = kAuthorizationFlagDefaults | kAuthorizationFlagPreAuthorize | kAuthorizationFlagInteractionAllowed | kAuthorizationFlagExtendRights;
    status = AuthorizationCopyRights(authorizationRef, &rightSet, kAuthorizationEmptyEnvironment, flags, NULL);
    if (status != errAuthorizationSuccess)
    {
        AuthorizationFree(authorizationRef, kAuthorizationFlagDestroyRights);
        return status;
    }
    
    char **args = NULL;
    if ((arguments != nil) && ([arguments count] > 0))
    {
        args = calloc(1, sizeof(char*) * ([arguments count] + 1));
        NSUInteger i=0;
        for (i=0; i<[arguments count]; i++)
        {
            NSString *arg = [arguments objectAtIndex:i];
            if ([arg isKindOfClass:[NSString class]])
                args[i] = (char *)[arg UTF8String];
        }
    }
    
    FILE *file = NULL;
    status = AuthorizationExecuteWithPrivileges(authorizationRef, path, kAuthorizationFlagDefaults, args, (output != NULL ? &file : NULL));
    ...

Now 5 crashes isn't really that much, since this code runs every single time on launch of the app, for every architecture supported on 10.4 and up, so I'm not overly concerned with it, but I'm guessing that there is some memory corruption going on.

I've been staring at this code all morning and am going a little crazy. The one idea I have is the call to calloc which *should* properly separate the count and size arguments instead of combining them like a call to malloc. However, would this really make a difference? The only thing I can think of is separating them might help prevent overflows but in this specific situation there is only 1 argument so that shouldn't apply.

Any other ideas?

[lee1210]

I'm probably not going to be able to be much help, but a few things:
Where do you free the pointer returned from calloc, assigned to args?
Have you written a small program that calls this method in a loop with quasi-random arguments and let it run for a few hours/days to see if you ever get a crash?
Have you run this for edge cases like 0 arguments?

Best i can think of for now, being wholly unfamiliar with the APIs you're using.

-Lee

[chown33]

First, is it running under GC or retain/release?

Second, calloc() zeros the memory it returns; malloc() doesn't. This is critical to the posted code, since you don't actually put a NULL in the last slot of args. So I would expect malloc() to fail in weird ways.

Third, I don't think this code does quite what you want:

        for (i=0; i<[arguments count]; i++)
        {
            NSString *arg = [arguments objectAtIndex:i];
            if ([arg isKindOfClass:[NSString class]])
                args[i] = (char *)[arg UTF8String];
        }

Imagine that arguments contains one NSObject followed by one NSString. What will the be result in args? Answer: null, utf8strPtr, null. The first null is wrong. I don't think it's relevant to the crash, but that doesn't make it any less wrong.

Finally, I might be suspicious of the lifetime of [arg UTF8String]'s returned pointer. It may be worthwhile to copy it to a calloc()'ed buffer whose lifetime you have more control over.

And are you sure AEWP will work with a NULL ptr passed in args? Again, I don't think it's relevant to the crash, but if arguments is nil or empty, that's what's passed in.

[Sydde]

Originally Posted by chown33

I might be suspicious of the lifetime of [arg UTF8String]'s returned pointer. It may be worthwhile to copy it to a calloc()'ed buffer whose lifetime you have more control over.

According to the docs, the block returned by -UTF8String will belong to the autorelease pool. I would not pass such a thing to an asynchronous process, and it not being a NSObject, there is no way to prevent its deallocation.

[kainjow]

Originally Posted by lee1210

Where do you free the pointer returned from calloc, assigned to args?

After the last line of code above.

Have you written a small program that calls this method in a loop with quasi-random arguments and let it run for a few hours/days to see if you ever get a crash?

Not yet but that's not a bad idea. However, AuthorizationCopyRights() pops up a dialog box asking for the user's username/password, so I'm not sure how doable that is.

Have you run this for edge cases like 0 arguments?

In the way it's used, arguments should always have 1 or more items in it, but I suppose I should make sure AEWP() still works in that case.

Best i can think of for now, being wholly unfamiliar with the APIs you're using.

Thanks. I've already added more checks, such as ensuring fileSystemRepresentation (which can throw an exception) and calloc don't return NULL, and making sure args is NULL terminated (although calloc *should* take care of that).

Originally Posted by chown33

First, is it running under GC or retain/release?

retain/release

Second, calloc() zeros the memory it returns; malloc() doesn't. This is critical to the posted code, since you don't actually put a NULL in the last slot of args. So I would expect malloc() to fail in weird ways.

As mentioned above I added this in to be explicit, but calloc should handle that, unless I'm misunderstanding something basic here...

Third, I don't think this code does quite what you want:
Imagine that arguments contains one NSObject followed by one NSString. What will the be result in args? Answer: null, utf8strPtr, null. The first null is wrong. I don't think it's relevant to the crash, but that doesn't make it any less wrong.

Agreed, I realized this looking it over but this shouldn't affect the crash. Worse case the new process just gets the arguments list truncated.

Finally, I might be suspicious of the lifetime of [arg UTF8String]'s returned pointer. It may be worthwhile to copy it to a calloc()'ed buffer whose lifetime you have more control over.

I'd be very surprised if AEWP was touching the autorelease pool to affect this.

And are you sure AEWP will work with a NULL ptr passed in args? Again, I don't think it's relevant to the crash, but if arguments is nil or empty, that's what's passed in.

After Lee mentioned it, I tested it and didn't see an issue. However, the docs don't mention that you can pass a NULL arguments as they often do (like they do for the last argument). Arguments should always be 1 or more, and I'd be amazed to find that AEWP doesn't handle a NULL arguments. Then again, NSTask's launchedTaskWithLaunchPath throws an exception if you pass a nil arguments. I'll change it to always pass a non-NULL pointer with at least the first element being NULL.

[chown33]

Originally Posted by kainjow

As mentioned above I added this in to be explicit, but calloc should handle that, unless I'm misunderstanding something basic here...

It was only in regards to your mention of malloc(). In short, a substitution of malloc(), with no other changes, would be incorrect, because the posted code was relying on calloc()'s zeroing.

I'd be very surprised if AEWP was touching the autorelease pool to affect this.

The suspicion was more an avenue for exploration: try it, see if anything changes. I expect it makes no difference, but debugging is often an exercise in challenged expectations.

[lee1210]

Originally Posted by kainjow

<snip>
Not yet but that's not a bad idea. However, AuthorizationCopyRights() pops up a dialog box asking for the user's username/password, so I'm not sure how doable that is.
<snip>

Maybe just make a version of this that gets the Authorization once, then generates various arguments and runs the rest of the code in a loop? I'm guessing the authorization token will go bad after some number of minutes, so you may only be able to do this N-minutes at a time, but you could likely get a few 10s of thousands of runs in during that time period. The reason I would try to get it crashing in some sort of reproducible way is that now if you make a change, you can't make it crash so you don't know if you've fixed it (unless you really catch the bug red-handed).

It seems like playing with arguments is going to be the key, because that seems to be what affects the behavior of the code at runtime. The idea of chown33 to get some memory for the arguments array and copy might be a good idea, too. If the char * returned from UTF8String has a lifetime of "sometime", that could certainly result in erroneous behavior "sometime".

-Lee