Sparkle runs thread-unsafe code on secondary threads

[andymatuschak]

See bug #388793 for a consequence of this bug, though this does not even
touch the real problem. As the problem is more widespread and lies
deeper I open a new more comprehensive bug.

Sparkle runs code that can only be used on the main thread on secondary
threads. This happens (for sure) in SUPlainInstaller,
SUDiskImageUnarchiver, and SUPipedArchiver.

The clearest problem is the use of shared instances of thread-unsafe
classes, such as NSFileManager, NSWorkspace, and NSBundle, either
directly or indirectly. You can use these instances ONLY on the main
thread, because they're the same objects that are already used for sure
on the main thread.

I can only see three ways to fix this (may depend on the particular instance):

1. Run this code synchronously.
2. Run the unsafe code synchronously before spawning the secondary thread.
3. Replace the unsafe Cocoa methods by thread-safe Carbon functions. For example, the Carbon file manager is generally thread safe.

Migrated from Launchpad: https://bugs.launchpad.net/sparkle/+bug/389869
Originally reported by Hofman (cmhofman) at Sat, 20 Jun 2009 12:05:18 -0000.

[andymatuschak]

As a partial fix for the most critical part, here's a thread-safe
function to copy a file. The rest could probably be handled using option
2.

BOOL SUCopyObjectAtURLToDirectoryAtURL(NSURL _srcURL, NSURL *dstURL, NSError *_error)
{
FSRef srcFileRef, dstDirRef;
BOOL success;

success = CFURLGetFSRef((CFURLRef)srcURL, &srcFileRef);
if (success)
    success = CFURLGetFSRef((CFURLRef)dstURL, &dstDirRef);

OSErr err = noErr;
FSRef newObjectRef;

if (success)
    err = FSCopyObjectSync(&srcFileRef, &dstDirRef, NULL, &newObjectRef, 0);

if (NO == success && error != NULL) {
    NSString *errorMessage = nil;
    if (noErr != err)
        errorMessage = [NSString stringWithUTF8String:GetMacOSStatusCommentString(err)];
    if (nil == errorMessage)
        errorMessage = NSLocalizedString(@"Unable to copy file.", @"Error description");
    *error = [NSError errorWithDomain:NSOSStatusErrorDomain code:err userInfo:[NSDictionary dictionaryWithObject:errorMessage forKey:NSLocalizedDescriptionKey]];
}

return success;

}

Originally posted on Launchpad by Hofman (cmhofman) at Sat Jun 20 05:44:40 -0700 2009

[andymatuschak]

NSFileManager is only thread-unsafe if you're relying on the delegate to
receive callbacks. In that case you can create a custom NSFileManager.
Most code doesn't actually set/use the NSFileManager delegates, and is
therefore safe from any thread.

Originally posted on Launchpad by Paul Marks (shadowofged) at Sun Jun 21 01:20:03 -0700 2009

[andymatuschak]

Where did you read that? Because I did not see that anywhere in the
docs. The threading guide says that NSFileManager is thread unsafe.
Period. The NSFileManager docs don't say any pf the methods is thread
safe. Unless Apple says that a class or method is thread safe, it's
thread unsafe. That's a fact. Moreover the thread unsafety of
NSFileManager has been discussed on many lists, also by Apple engineers.

Apart from that, the most critical methods I'm talking about DO take a
delegate. But that's, as I said, irrelevant.

And using a second NSFileManager is only supported since 10.5, while
Sparkle supports 10.4. So that's not an option. Moreover, it's probably
overkill to produce a heavy object like NSFileManager for just one call
on a secondary thread (there is probably a reason they use a singleton
rather than class methods).

Originally posted on Launchpad by Hofman (cmhofman) at Sun Jun 21 02:07:27 -0700 2009

[andymatuschak]

Some more hints how to solve this. I'm hesitant to fix this in my copy,
because I'd like to do it the same as in the main branch.

Basically all file copies can be replaced by FSCopyObjectSync, and the
file moves can be replaced by FSMoveObjectSync and
FSMoveObjectToTrashSync, see my sample above how to do this.

To check for write access use access([path fileSystemRepresentation],
W_OK), similar for read access checks.

To create a temporary directory (is this really necessary on the
background thread?) you could use FSCreateDirectoryUnicode.

To delete a temporary directory, you can check the sample code in
MoreFilesX. As this is not really slow and normally not used, you can
also just perform this on the main thread using NSFileManager.

To check whether a file exists, you can simply use FSPathMakeRef, if it
returns noErr the file exists.

To enumerate a directory, use FSOpenIterator, the docs have a generic
sample for this.

You may decide to simply do the releaseFromQuarantine using
performSelectorOnMainThread:..., otherwise you need to iterate inside
directories yourself. This is not really a slow process.

And what's the NSFileSize check in SUPipedUnarchiver for? The returned
file size is never used.

Originally posted on Launchpad by Hofman (cmhofman) at Sat Jun 27 03:08:13 -0700 2009

[andymatuschak]

Ugh, this is disgusting.

I wonder how bad it would be to just run it synchronously. I'm not sure
the concurrency is worth the hideousness.

Originally posted on Launchpad by Andy Matuschak (andymatuschak) at Mon Jul 06 23:36:45 -0700 2009

[andymatuschak]

Concurrency, at least as far as the file access is concerned, should not
be a problem. But these objects are simply not thread safe, and you
don't know what goes on behind the scenes.

I was also wondering if it could run synchronously, it's also something
that does not happen too frequently. I myself have disabled running
SUPlainInstaller async, also because that was the simple solution and I
was about to release and because of the (even more serious) callback
issue.

However copying a whole (potentially big) bundle could be slow, remember
of the progress dialog Finder shows. Installation is less an issue
because the user at that point already knows he's going to lose the app
at any moment. However I think copying can best be done async (though
perhaps not the quarantine thing). Moreover, I don't think the Carbon
calls are that bad, not worse than the NSTask stuff. The most
complicated is the enumerated copying in SUDiskImageUnarchiver, but in
fact Carbon is in that respect way faster than Cocoa (because Cocoa
fetches any irrelevant file attributes).

Originally posted on Launchpad by Hofman (cmhofman) at Tue Jul 07 02:32:33 -0700 2009

[andymatuschak]

I've attached a thread safe version of SUDiskImageUnarchiver.m, using
Carbon file manager methods. It's not much longer.

I think this is the most important one that you may want to leave async.

Note that I removed the lines creating a folder at the mount point. That
is totally unnecessary, because hdiutil overwrites it anyway.

And note that you need to link CoreServices (or Carbon) to use this.

Originally posted on Launchpad by Hofman (cmhofman) at Tue Jul 07 05:59:19 -0700 2009

[andymatuschak]

Attached another version of SUDiskImageUnarchiver.m. Forgot to assign
the returned error in one call.

Originally posted on Launchpad by Hofman (cmhofman) at Tue Jul 07 11:06:06 -0700 2009

[andymatuschak]

Attached a thread safe version of SUPlainInstallerInternals.m using
Carbon file manager functions.

Originally posted on Launchpad by Hofman (cmhofman) at Tue Jul 07 13:01:21 -0700 2009

[andymatuschak]

And a thread safe version of SUPlainInstaller.m, making sure the
callback takes place on the main thread.

The only other change necessary apart from these three is
SUPipedArchiver.m, just remove the 2 lines for the file size check.

Originally posted on Launchpad by Hofman (cmhofman) at Tue Jul 07 14:25:20 -0700 2009

[andymatuschak]

Here's an even better solution for SUPlainInstaller. It uses the old
temporary filename, but gets it first before spawning a thread. Also
avoids getting the same FSRef twice, and fixes the method name.

Originally posted on Launchpad by Hofman (cmhofman) at Wed Jul 08 06:08:56 -0700 2009

[andymatuschak]

Oops, moved the wrong file. Also another effort duplication removed.

Originally posted on Launchpad by Hofman (cmhofman) at Wed Jul 08 07:13:54 -0700 2009

[andymatuschak]

Here are all the necessary changes combined into a patch

Originally posted on Launchpad by Hofman (cmhofman) at Wed Jul 08 14:15:23 -0700 2009

[andymatuschak]

Wow, in fact FSCopyObjectSync, unlike NSFileManager, does allow copying
the volume root as a whole, so the whole enumeration in
SUDiskImageUnarchiver is not necessary. New version attached. Tested on
10.5.7.

Small correction in SUPlainInstaller: in my version nil values can be
passed to -setObject:forKey: (for the error), so instead
-setValue:forKey: should be used.

Sorry for the many versions.

Originally posted on Launchpad by Hofman (cmhofman) at Thu Jul 09 10:54:37 -0700 2009

[andymatuschak]

Hofman, don't be sorry—thanks for doing the legwork on this one! It may
be a little time before I can review it in detail, but this is high
priority for me.

Originally posted on Launchpad by Andy Matuschak (andymatuschak) at Thu Jul 09 11:26:22 -0700 2009

[andymatuschak]

Just noticed that FSMoveObjectToTrashSync() is Leopard-only. That could
be replaced by a performSelectorOnMainThread:withObject:waitUntilDone:
calling the old code, possibly conditionally on the OS version. That
would not be a problem, as a move on the same volume is fast (unlike
copy).

Originally posted on Launchpad by Hofman (cmhofman) at Mon Jul 13 09:55:49 -0700 2009