mikeash.com: just this guy, you know?

Posted at 2009-11-13 20:38 | RSS feed (Full text feed) | Blog Index
Next article: Deconstructing Apple's Copyright and Trademark Guidelines
Previous article: Friday Q&A 2009-11-06: Linking and Install Names
Tags: cocoa dangerous threading
Friday Q&A 2009-11-13: Dangerous Cocoa Calls
by Mike Ash  

It's another Friday, and so time for another Friday Q&A. This week, Quentin Carnicelli (one of the guys who signs my paychecks) suggested that I talk about dangerous API calls in Cocoa.

First, let me clarify what I mean by a dangerous call. I don't mean something that is obviously dangerous, like -[NSFileManager removeFileAtPath:handler:]. That call is dangerous because you could use it to wipe the user's entire hard drive, but it's not interesting, because it's obvious that it can do this. Instead, I'm going to cover calls which are subtly dangerous, things which you might easily use in an innocent manner, only to discover later that bad things can occur.

Without further ado, here are my dangerous calls:

-[NSTask launch]
This call is dangerous because it throws exceptions when it fails, and it can fail for completely mundane problems, like trying to execute a file that doesn't have the executable bit set. Because the Cocoa tradition is to only throw exceptions for programmer errors, this can cause trouble for the unwary programmer who doesn't wrap this in a @try block.

-[NSTask waitUntilExit]
This one looks tremendously innocent. The problem is that, if the task is still running, it will run the runloop to wait for the task to exit. Even that would not be a problem, except that it runs the runloop in the default mode. Because of this, all timers, callbacks, etc., that are installed in the default mode will continue to run, but will be running in a context they did not expect. This call looks like a simple blocking call, but it can potentially lead to the invocation of random timers or other callbacks. This in turn can easily lead to deadlocks or data corruption.

+[NSTimer scheduledTimerWithTimeInterval:target:selector:userInfo:repeats:]
This returns a timer whose memory management is difficult to get right. Many people simply stash the return value in an instance variable. This is legitimate, because the runloop is documented to retain the timer. However, the runloop will release it as soon as it's invalidated, which will happen once a non-repeating timer fires, and also upon explicit invalidation. It's easy to get this wrong and end up with a dangling reference, and a lot of code has this problem. To make things even more fun, the timer retains its target, so if you retain the timer, you're likely to set up a retain cycle.

NSHost
Yes, this entire class is dangerous, and should not be used. Why? It's an unfortunate confluence of two otherwise unrelated properties of the class.

The first property is that NSHost has a blocking API. It accesses network resources, and as such any call to it can take an indefinite amount of time. This is fine by itself.

The second property is that NSHost returns shared objects, but NSHost is not thread safe. This means that it can only be safely used on the main thread.

Put the two together, and you have a class which can block any time you use it, but which can only be used on the main thread. Since it's unacceptable to indefinitely block the main thread of an application, this basically means that you can't use NSHost.

NSBundle
This one has half of the problems of NSHost. NSBundle returns shared objects, but is not thread safe, so it's main-thread only. It's still safe to use from the main thread. The reason I mark it as dangerous is because the fact that it's unsafe to use from secondary threads is not really documented, but rather has to be inferred from the fact that it's not thread safe and the fact that the instances are shared, and it can be tempting to use it from other threads.

-[NSBundle unload]
Because it's very easy to make references to memory within a bundle's code (e.g. by creating instances of an Objective-C class defined within the bundle, or by referring to literal strings defined within the bundle), and extremely difficult to make sure that all such references are gone, it's generally not safe to unload code on Mac OS X. If any such dangling references remain, your application will crash.

-[NSImage imageNamed:], followed by mutation
This call is very convenient, but it returns a shared object. A lot of code will make this call, and then immediately start resizing or drawing into the object that it returns. This can mess up the shared image for any other code that uses it. If you're going to modify the image, then you need to copy it first, but this isn't immediately obvious.

-[NSDate timeIntervalSinceReferenceDate] used for elapsed time
How many of you have written code like this?

    BOOL success;
    NSTimeInterval start = [NSDate timeIntervalSinceReferenceDate];
    do
        success = [self trySomething];
    while(!success && [NSDate timeIntervalSinceReferenceDate] - start <= kTimeoutInterval);
I'm guilty of it myself. It's a pretty natural way to write a timeout loop. There are many other circumstances where you might use this sort of thing too, not with an explicit loop, but in something more asynchronous.

So what's the problem? -timeIntervalSinceReferenceDate uses the system clock, which can be changed! Imagine that your program enters this loop, but before it can exit it, the system clock is set back by a day. Your timeout has now gone up to a day! Or imagine the opposite, that the system clock is suddenly set forward. Your timeout will suddenly become zero.

Mac OS X tries to make time adjustments smooth. For example, if the NTP client discovers that your clock is a few seconds out of whack, it will gradually pull it back in rather than making the adjustment all at once. However, sudden adjustments are still possible, even if rare, and your code should be robust against them. Even the gradual adjustments will cause your timeouts to change duration, albeit not by a huge amount.

The bad news is that Cocoa doesn't make it easy to do this right. You want to use a consistent, non-adjustable for elapsed time, but Cocoa has no APIs for such a thing. The OS does, so you just have to drop down a level.

My preferred API for this task is mach_absolute_time. However, this returns a value that's in terms of arbitrary timebase units, not something comfortable like seconds or nanoseconds. It's not hard to convert, but requires some work. Apple has two examples of how to convert the return value into something sensible.

Two other reasonable alternatives are the Carbon calls Microseconds and UpTime.

Distributed Objects
I've said it before, but Distributed Objects need special care and attention. The trouble is that DO looks like a completely transparent way to access objects in other processes, but it doesn't entirely succeed at this. It can still be a good way to do IPC, but it requires more care than you might think from reading through the documentation on it. For details on why that's so, my original post on the subject covers it all.

Conclusion
No, that's not a dangerous Cocoa call, it just means that we're done for the week. Come back in seven days for another exciting edition. In the meanwhile, keep those topic suggestions flowing. Friday Q&A is driven by your ideas, so if you'd like to see a topic covered here, let me know.

Did you enjoy this article? I'm selling whole books full of them! Volumes II and III are now out! They're available as ePub, PDF, print, and on iBooks and Kindle. Click here for more information.

Comments:

Crud. I use referenceDates all the time - and NSBundles from threads. Time to rethink.

There a lot of classes that will throw exceptions when you least expect. The "only throw exceptions for programmer errors" rule is stretched too thin I think.

Again, great post.
Do you have an example of the "lot of classes"? I don't wrap my code in exception handlers normally, nor ensure that the code is safe to throw through, but I seem to get away with it. I might be living dangerously by doing this, certainly, but in my experience most Cocoa classes only throw if you screw something up, like trying to insert a nil value into a collection.
I'm struggling right now with CoreData issues that involve exceptions being throw by the managed object context when I try to merge changes between contexts. CoreData is generally very good about returning an NSError - but this one API call throws instead.

That's the first API that comes to mind immediately. But I've been hit by exceptions before. Programmer error can be a very ambiguous term for example the NSTask execute flag error could be called programmer error with a bit of a stretch…
Wonderful article, as usual, Mike. Thanks, very appreciated.

At first when I saw your tidbit about NSDate, I thought "Too bad we can't use NSDate for this, it's nice to use something high-level when we can" but of course I didn't stop there. You've inspired me to write and add this to my library of OSS:


#import <mach/mach_time.h>

@interface SDTiming : NSObject {
    uint64_t start;
}

- (void) mark;
- (uint64_t) elapsed;

@end


@implementation SDTiming

- (void) mark {
    start = mach_absolute_time();
}

- (uint64_t) elapsed {
    uint64_t end = mach_absolute_time();
    uint64_t elapsed = end - start;
    
    Nanoseconds elapsed_nano = AbsoluteToNanoseconds(*(AbsoluteTime*) &elapsed);
    return *(uint64_t*) &elapsed_nano;
}

@end


P.S. I really hope the
tag worked, because I seem to vaguely remember that
doesn't format code on your blog properly.
Nice post Mike. We just had an iPhone app rejected because of NSHost's complete crapitude (app talks to a desktop app, timed out waiting for NSHost to do reverse lookups, followed by tears and the dreaded "license agreement section 3.3.5" e-mail).

NSHost is indeed the devil. Speak not its name.
Great post. Another specific reason Distributed Objects is dangerous is because remote proxy objects can throw exceptions on nearly ANY call to them, if something goes wrong at the DO layer (socket breaks, remote process crashes, etc.)

This means any code working with potentially-remote objects needs to be exception-safe. But you can pass remote objects to Cocoa APIs, which can be hit by those exceptions internally when calling the object ... bad news.

(An easy way to get that problem is to pass a URL as a parameter to a DO method. NSURL is for some reason proxied instead of copied, so any usage of that URL on the other side results in a DO call and potentially an exception.)
Uh oh...I use NSBundle and NSLocalizedString all the time from threads, too, so maybe it's time to check out CFBundle instead. I think NSPipe and/or NSFileHandle will also throw exceptions for things that aren't necessarily a programmer error.
Jonathan Wight: Sounds troublesome. I've never done anything with CoreData so I wouldn't have seen that. IMO any such Cocoa API is broken, at least unless it comes with big flashing red warning signs.

Steven Degutis: I've now made the code tag public, so that'll fix code for the future. Example:

while(1)
    destroy();

Adam Maxwell: Note that CFBundle is safe, although it's difficult to tweeze this out of the documentation. As I recall, it's not on the master list of thread safe CF classes, but there's a section buried somewhere in the CF threading docs that says that any CF API which vends shared instances will make sure that they're safe to use from multiple threads.

As for NSPipe and NSFileHandle, I've seen NSFileHandle throw exceptions for any error returned by the POSIX layer, including EINTER, which is crazy, and it was my intent to include it in my list here. However, I couldn't reproduce that no matter how hard I tried. I speculate that this may be fixed on 10.6.
Re: CFBundle thread safety, I checked the source for CF-550 and it looks like access is indeed protected with a spinlock. That agrees with the CF thread safety summary at http://developer.apple.com/Mac/library/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html which says "Even central shared objects that might be queried from more than one thread are reliably thread-safe."

It'd be nice if NSFileHandle didn't throw exceptions for errors, although it's still documented to do that in 10.6. The -[NSTask launch] behavior you mention is worse, though.
Yep, that's the thread safety document I was thinking of. As for NSFileHandle, for some reason it didn't even occur to me to check for other errors, I only checked to see if it threw spuriously for EINTR. It could very well still throw for other errors.
I'd add NSTimer's initWithFireDate:interval:target:selector:userInfo:repeats: too. Let's say you want to create a timer that goes off at some particular user-specified time, say midnight. You might try using this method. But of course the system clock can and does change (ex: Daylight Savings Time, changing time zones). The docs say "...it will fire at date...", which is vague at best. The method's implementation seems to take the given date/time, subtract the current date/time, and fire a timer that many seconds later. So you need to register for system clock change notifications like so:


    EventTypeSpec eventType;
    eventType.eventClass = kEventClassSystem;
    eventType.eventKind = kEventSystemTimeDateChanged;

    InstallApplicationEventHandler(...)


or on 10.6 using NSSystemClockDidChangeNotification.
[NSDate date] has the same problem as timeIntervalSinceReferenceDate. That problem didn't occur to me—thanks for this post.
That thread-safety document lists NSHost as being safe to call on multiple threads so long as you only call it on one at a time. Did you find out that this is not true the hard way, or is it something that changed recently?
10.6 adds -[NSProcessInfo systemUptime]: “An NSTimeInterval indicating how long system the computer has been restarted.”
Jim G: Unless you can guarantee that nothing in Cocoa (and nothing in third-party code which might happen to get loaded into your process) will ever touch NSHost, you have no way to enforce that "only call it on one at a time" contract.

Jens Ayton: Good catch, I didn't notice that one. Of course, NSProcessInfo is, guess what, not thread safe, and is a shared object, so that's only useful if you need that information on the main thread.
Ah, I see what you mean now.

But then what about +setHostCacheEnabled and +flushHostCache? It seems like this would disable the shared object behaviour that makes it unsafe to use from different threads.

Also, there is this comment in NSHost.h (10.4 through 10.6 SDKs):
// NSHost does not implement any caching any longer

So now I'm very confused! :)
OK, scratch that, it looks like the comment means that control over the caching is not implemented, because isHostCacheEnabled just returns NULL.
Has anyone filed bugs for these issues?
Great post, as usual! I’m sure there are many more hidden pitfalls in Cocoa. To avoid flooding your articles’s comments I started a discussion on Stack Overflow on this topic: http://stackoverflow.com/questions/1734386/what-are-common-cocoa-pitfalls
HS: What good is it to file bugs on these? With people still running Panther in the installed base, it will be 5 years minimum before you'll have to stop coding around these issues.
Jim G: Even if it worked to disable caching, you still couldn't use it safely, because some other code could just go in and re-enable it, oblivious to the fact that this will screw you up.

originalgeek: That's an incredibly short-sighted view. In the best case, bugs would get fixed for 10.7, which you could reasonably start requiring in only another 2-3 years, depending on when it ships and your own personal taste. Very few people are still coding to Panther. My employer recently moved everything to requiring Leopard, and I haven't written Panther-capable code in several years. Even if you do require Panther yourself, it's still a very short-sighted view: would you like it if Panther still had all the bugs that 10.0 did? Progress benefits you, even if you're slow to follow it.

I'm not real big on filing bugs myself, because Apple's process is fairly broken. But the idea that it's pointless because it'll take too long for you to take advantage of a fix is just not a good reason.
FYI the dispatch timers with dispatch_time are safe to use even if someone changes the clock. If you want to have the clock change reflected by the timer use dispatch_walltime.

That way you don't need to mess with Mach timers directly.
This kind of information is so much more helpful than the numerous books which only describe the right way of doing things.
Strips: Are you sure about that? From looking at the libdispatch source code, it would appear that a timer set using dispatch_walltime will fire whenever the clock time exceeds its fire time and the dispatch manager wakes up, but I don't see anything that watches for clock changes to make sure that the dispatch manager actually wakes up earlier if the clock has been changed after the timer is scheduled. I could easily have missed something, though.
Would there be any way to either outfit clang or some similar analyzer to warn you that you're doing some of these things? That would be so useful, especially when learning a new API!
No, the manager queue isn't specifically woken up for clock changes. It tends to get woken up often enough to pick up changes, but yes there are no guarantees. Well, this would do it:

  notify_register_dispatch(kNotifyTimeZoneChange, &notify_token, dispatch_get_global_queue(0, 0), ^{});

...however I was actually trying to point at dispatch_time (not dispatch_walltime) as being suitable for timeouts in a way timeIntervalSinceReferenceDate isn't.
I don't think that dispatch_time is suitable for timeouts simply because it's an opaque type and the API doesn't have enough functionality to actually use it for comparisons and the like. In other words, you can query it for things like the current time, and a time that's a certain time into the future, but there's no way to see if the one is greater than the other.
Have you actually checked that NSHost is returning shared objects? A simple if ([NSHost currentHost] == [NSHost currentHost]) should do the trick.
It doesn't matter what NSHost does now. All that matters is the API contract. Unless the contract says that it's guaranteed not to return shared objects, you can't count on that. Even if it never returns shared objects on 10.6, you can't know how the implementation might change later on.
"I don't think that dispatch_time is suitable for timeouts simply because it's an opaque type and the API doesn't have enough functionality to actually use it for comparisons and the like."


I didn't have a compare in mind:


    BOOL success;
    __block volatile BOOL give_up = NO;
    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, kTimeoutInterval), dispatch_get_global_queue(0, 0), ^{ give_up = YES;});

    do
        success = [self trySomething];
    while(!success && !give_up);


That would be better as a function that takes a block and timeout (unless you want uniform timeouts) and runs the block until it returns YES or until the timeout expires...
Obviously I had a failure of imagination here. That is indeed a nice way to trigger the timeout. Efficiency may not be so good, but that's unlikely to matter. Thanks for elaborating!
Well, yeah dispatch_after is going to call malloc a few times vs. getting a mach time stamp on each try. You would have to make a LOT of tries before the dispatch version has a chance of breaking even.

You could "even out the odds" by not doing dispatch_after until after the first try (if and only if it fails), but then the code is getting kind of large...

So yes, it isn't appropriate to use in (most) places absolute performance is required. I _would_ use it to get a block interface like:

runWithTimeout(kTimeoutInterval, block)

since that sort of clarity is worth the modest performance cost...then again that doesn't require use of dispatch_time, you can use straight mach time calls and still pass in a block to execute (and blocks are performance competitive with function pointers as long as you don't need to Block_copy, which this function wouldn't need to).
Darrin Cardani: There was a discussion on clang-dev mailing list in January about providing a plugin API for clang to make it possible to add additional checks, just like these ones (see [1]). However, currently the only option is to code these checks manually (in C++) into clang itself. It's not too hard to do (I've done some basic checks in short amount of time), and clang community accepts well-made patches pretty easily so it might get into the release build as well. Check [2] to get started.

[1] http://lists.cs.uiuc.edu/pipermail/cfe-dev/2009-January/004001.html
[2] http://clang.llvm.org/get_started.html
To quote NSHost.h:

// NSHost does not implement any caching any longer
I don't see that comment as being a guarantee of future behavior, but simply a description of present behavior. Even if it were a guarantee, caching is not the only way that you could get a shared object. They could easily have something that simply gives you an extant NSHost object for the name/address in question if it hasn't been deallocated yet.
Ouch! I've got multithreaded application with lots of waitUntilExit!

What's the alternative? Should I run it on main thread (and hope for nested runloops or something?)

Can I make it a just-blocking-call?
Because runloops are per-thread, waitUntilExit is typically safe on secondary threads. It would be dangerous only if you're actively running a runloop on that secondary thread and are scheduling things in NSDefaultRunLoopMode that shouldn't be running during the call to waitUntilExit. Otherwise, it will not be a problem. In fact, one way to make this call safe would be to proxy it over to another thread where you know that these things aren't being done, and wait for it to complete there before proceeding.
This came up in my feed reader right after I posted a somewhat similar article (http://www.doomstick.com/blog/?p=157) though much smaller in scope and with fewer examples. For the sake of completeness I will list mine here as well.

NSPipe documentation explicitly states that you don't need to do anything special to close it out ("You don’t need to send closeFile to this object or explicitly release the object after you have finished using it.").

However, if you're using a lot of pipes, and garbage collection doesn't kick in early enough you will get an exception when you run out of file handles. Closing them with closeFile is therefore something that I've learned the hard way to always do.
Thanks for this great post.
Question... if +[NSTimer scheduledTimerWithTimeInterval:target:selector:userInfo:repeats:]
 is a dangerous call. What would you suggest using instead?
There is no better alternative. All you can do is take care when using it to ensure that your memory management is correct. This usually means retaining the returned time, and either having the target be a different object than the one that holds the timer reference, or setting up an explicit "stop" method so the timer can be destroyed and the reference cycle broken. All of this is reasonable. The reason this call is dangerous is not because it's intractable, but because it looks much simpler to use than it is, so people often don't take the necessary steps.
Apple already provided a great guide to the safety of their APIs in the naming conventions

NS* == Not Safe
Fear not folks. There's no way that NSDate can alter the system clock. Especially, given the code example used in the post. Read the NSDate Class Reference (which is why they were written) and you'll see why. Better yet, use the code presented and let it run for as long as you like. You'll see no change in the system clock.
Dan: You completely misunderstand the nature of the problem. The problem is not that NSDate can alter the system clock. As you say it can't. The problem is that something else can alter the system clock (like ntpd) while you're in the loop, causing NSDate to return results that your code will not expect.
And apparently I don't read too well. I of course meant to type Don.
Lesson learned. My apologies for "skimming" and not "reading". Looks like I'll have look more deeply into mach_absolute_time.
Does the problem with NSHost apply to CFHost as well?
The CFHost reference makes numerous mentions of thread safety, so I should think it's fine in this respect.
My favorite is -[NSMutableArray removeObject]. It removes *all* objects that satisfy isEqual, not just one object that has the same pointer value.
The day after I read this, I got 40 crashlogs with the NSTask launch exception (all from the same person, their system had got into a state where it could not create processes). Nice timing!

Another NSTask method that can unexpectedly throw an exception is terminate. I think it happens if the launch initially succeeds, but then fails for some later reason, you then call terminate and it throws a "not launched" exception.
I discovered that the link changed for the mach_absolute_time article.

The new link is at:

https://developer.apple.com/library/mac/qa/qa1398/_index.html

Comments RSS feed for this page

Add your thoughts, post a comment:

Spam and off-topic posts will be deleted without notice. Culprits may be publicly humiliated at my sole discretion.

Name:
The Answer to the Ultimate Question of Life, the Universe, and Everything?
Comment:
Formatting: <i> <b> <blockquote> <code>.
NOTE: Due to an increase in spam, URLs are forbidden! Please provide search terms or fragment your URLs so they don't look like URLs.
Code syntax highlighting thanks to Pygments.
Hosted at DigitalOcean.