PINDiskCache: +[moveItemAtURLToTrash:] races with +[emptyTrash]

[mjbshaw]

Proof of concept:

- (void)testDiskEmptyTrashRace
{
    NSString *cacheName = @"testDiskEmptyTrashRace";
    PINDiskCache *diskCache = [[PINDiskCache alloc] initWithName:cacheName];

    for (NSUInteger idx = 0; idx < 10000; idx++) {
        NSString *key = [@(idx) stringValue];
        NSData *tmpData = [key dataUsingEncoding:NSUTF8StringEncoding];
        [diskCache removeAllObjects];
        [diskCache setObject:tmpData forKey:key];
        XCTAssertNotNil([diskCache objectForKey:key]);
        [diskCache removeObjectForKey:key];
        XCTAssertNil([diskCache objectForKey:key]);
    }
}

The XCTAssertNil check fails eventually:

-[PINDiskCache removeAllObjects] calls +[PINDiskCache emptyTrash]. This calls dispatch_async(...), which schedules the block but doesn't execute it.

-[PINDiskCache removeObjectForKey:] (indirectly) calls +[PINDiskCache moveItemAtURLToTrash:]. This call is protected by _mutex, but not by sharedLock. This method then calls +[PINDiskCache sharedTrashURL] to obtain the shared trash URL.

However, in between the time when the URL is retrieved and the item is moved to the trash URL, the async block from +[PINDiskCache emptyTrash] executes. This block removes the shared trash directory (it also clears _sharedTrashURL but that's irrelevant because +[PINDiskCache moveItemAtURLToTrash:] has already obtained the old trash URL).

Now that the trash directory is deleted, +[PINDiskCache moveItemAtURLToTrash:] calls -[NSFileManager moveItemAtURL:toURL:error:] to attempt to move the item into the trash. This will fail because the trash directory no longer exists. The end result is that the item is never removed. This is a time-of-check-to-time-of-use race.

There are a couple ways this could be fixed.

• Option A: If the call to +[PINDiskCache moveItemAtURLToTrash:] fails, the cache could have a fallback and just directly call -[NSFileManager removeItemAtURL:error:] to delete the item.
• Option B: +[PINDiskCache moveItemAtURLToTrash:] could obtain the sharedLock and hold it while executing the following two lines of code:

  NSURL *uniqueTrashURL = [[PINDiskCache sharedTrashURL] URLByAppendingPathComponent:uniqueString isDirectory:NO];
  BOOL moved = [[NSFileManager defaultManager] moveItemAtURL:itemURL toURL:uniqueTrashURL error:&error];

  This requires some refactoring to either use a recursive lock or make it so the call to +[sharedTrashURL] doesn't deadlock.

Option A is probably the simplest, but Option B probably has less mental overhead because it fully resolves the race condition rather than trying to recover from it ad-hoc.

Personally, I like both options. I like that Option B fully resolves the race, and I like that Option A provides a fallback. If you're trying to delete a file, I don't think you should give up and keep the file just because you failed to create a new directory or rename something, since those actions are orthogonal to the original goal.