From d7440b4a7604bdd42d22200dd224cbca0b8b4a9a Mon Sep 17 00:00:00 2001 From: Austin Kim Date: Fri, 1 May 2026 16:38:07 -0700 Subject: [PATCH] Fix cross-volume update failure that deletes the app without replacing it When the target app lives on a different volume than where ShipIt stages downloads (~/Library/Caches/ on the system volume), POSIX rename() fails with EXDEV. The old fallback called removeItemAtURL: on the target first, then NSFileManager moveItemAtURL:toURL:. If that move fails for any reason, the target slot is empty and the app is gone with no way to recover. Fix: when EXDEV fires, copy the source to a UUID-named staging path on the same volume as the target, then rename() atomically from staging to target. Both paths are on the same volume so rename() is guaranteed to succeed or leave the target untouched. The source is only deleted after a successful rename. On copy failure, the source is still intact and the existing item at the target is never touched. This correctly handles the backup step in acquireTargetBundleURLForRequest: (moving the current app off an external volume to system temp) and the rollback path in abortInstall (restoring from system temp back to the external volume). New tests: - Install an update from one external volume to another (double EXDEV) - Restore the app to an external volume after too many failed install attempts Fixes: https://github.com/Squirrel/Squirrel.Mac/issues/201 Co-Authored-By: Claude Sonnet 4.6 --- Squirrel/SQRLInstaller.m | 32 ++++++++++++---- SquirrelTests/SQRLInstallerSpec.m | 64 +++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 8 deletions(-) diff --git a/Squirrel/SQRLInstaller.m b/Squirrel/SQRLInstaller.m index 43d7d67..d0f2741 100644 --- a/Squirrel/SQRLInstaller.m +++ b/Squirrel/SQRLInstaller.m @@ -631,17 +631,33 @@ - (RACSignal *)installItemToURL:(NSURL *)targetURL fromURL:(NSURL *)sourceURL { catch:^(NSError *error) { if (![error.domain isEqual:NSPOSIXErrorDomain] || error.code != EXDEV) return [RACSignal error:error]; - // If the locations lie on two different volumes, remove the - // destination by hand, then perform a move. - [NSFileManager.defaultManager removeItemAtURL:targetContentsURL error:NULL]; + // The source and target are on different volumes. Stage the source in a + // UUID-named temporary path on the same volume as the target, then + // rename() atomically. This ensures the target slot is never left empty: + // the existing item (if any) stays in place until the rename succeeds. + NSURL *stagingContentsURL = [targetContentsURL.URLByDeletingLastPathComponent + URLByAppendingPathComponent:[[NSUUID UUID] UUIDString]]; + + NSError *copyError = nil; + if (![NSFileManager.defaultManager copyItemAtURL:sourceContentsURL toURL:stagingContentsURL error:©Error]) { + NSString *description = [NSString stringWithFormat:NSLocalizedString(@"Couldn't stage bundle %@ on target volume at %@", nil), sourceContentsURL, stagingContentsURL]; + return [RACSignal error:[self errorByAddingDescription:description code:SQRLInstallerErrorMovingAcrossVolumes toError:copyError]]; + } - if ([NSFileManager.defaultManager moveItemAtURL:sourceContentsURL toURL:targetContentsURL error:&error]) { - NSLog(@"Moved bundle contents across volumes from %@ to %@", sourceContentsURL, targetContentsURL); + if (rename(stagingContentsURL.path.fileSystemRepresentation, targetContentsURL.path.fileSystemRepresentation) == 0) { + [NSFileManager.defaultManager removeItemAtURL:sourceContentsURL error:NULL]; + NSLog(@"Installed bundle cross-volume from %@ to %@", sourceContentsURL, targetContentsURL); return [RACSignal empty]; - } else { - NSString *description = [NSString stringWithFormat:NSLocalizedString(@"Couldn't move bundle contents %@ across volumes to %@", nil), sourceContentsURL, targetContentsURL]; - return [RACSignal error:[self errorByAddingDescription:description code:SQRLInstallerErrorMovingAcrossVolumes toError:error]]; } + + int renameErrno = errno; + [NSFileManager.defaultManager removeItemAtURL:stagingContentsURL error:NULL]; + NSMutableDictionary *renameInfo = [NSMutableDictionary dictionary]; + const char *renameDesc = strerror(renameErrno); + if (renameDesc != NULL) renameInfo[NSLocalizedDescriptionKey] = @(renameDesc); + NSError *renameError = [NSError errorWithDomain:NSPOSIXErrorDomain code:renameErrno userInfo:renameInfo]; + NSString *description = [NSString stringWithFormat:NSLocalizedString(@"Couldn't rename staged bundle contents %@ to %@", nil), stagingContentsURL, targetContentsURL]; + return [RACSignal error:[self errorByAddingDescription:description code:SQRLInstallerErrorMovingAcrossVolumes toError:renameError]]; }] setNameWithFormat:@"%@ -installItemAtURL: %@ fromURL: %@", self, targetContentsURL, sourceContentsURL]; } diff --git a/SquirrelTests/SQRLInstallerSpec.m b/SquirrelTests/SQRLInstallerSpec.m index a177a2a..d4d7a7d 100644 --- a/SquirrelTests/SQRLInstallerSpec.m +++ b/SquirrelTests/SQRLInstallerSpec.m @@ -171,6 +171,26 @@ expect([NSDictionary dictionaryWithContentsOfURL:plistURL][SQRLBundleShortVersionStringKey]).withTimeout(SQRLLongTimeout).toEventually(equal(SQRLTestApplicationUpdatedShortVersionString)); }); +it(@"should install an update from one external volume to another", ^{ + // Both the update bundle and the target app live on separate disk images, + // so every rename() call in the install path hits EXDEV. This exercises + // the cross-volume staging path twice: once when the target app is backed + // up onto the system volume and again when the update is placed onto the + // target volume. + NSURL *updateDiskImageURL = [self createAndMountDiskImageNamed:@"TestUpdate" fromDirectory:updateURL.URLByDeletingLastPathComponent]; + NSURL *crossVolumeUpdateURL = [updateDiskImageURL URLByAppendingPathComponent:updateURL.lastPathComponent]; + + NSURL *targetDiskImageURL = [self createAndMountDiskImageNamed:@"TestApplicationExternal" fromDirectory:self.testApplicationURL.URLByDeletingLastPathComponent]; + NSURL *crossVolumeTargetURL = [targetDiskImageURL URLByAppendingPathComponent:self.testApplicationURL.lastPathComponent]; + + SQRLShipItRequest *request = [[SQRLShipItRequest alloc] initWithUpdateBundleURL:crossVolumeUpdateURL targetBundleURL:crossVolumeTargetURL bundleIdentifier:nil launchAfterInstallation:NO useUpdateBundleName:NO]; + + [self installWithRequest:request remote:YES]; + + NSURL *plistURL = [crossVolumeTargetURL URLByAppendingPathComponent:@"Contents/Info.plist"]; + expect([NSDictionary dictionaryWithContentsOfURL:plistURL][SQRLBundleShortVersionStringKey]).withTimeout(SQRLLongTimeout).toEventually(equal(SQRLTestApplicationUpdatedShortVersionString)); +}); + describe(@"with backup restoration", ^{ __block NSURL *targetURL; @@ -222,6 +242,50 @@ }); }); +describe(@"with backup restoration when target is on another volume", ^{ + // Models the VS Code scenario: the app lives on a non-system volume. ShipIt + // backed it up to system temp (ownedBundle.temporaryURL) before it was + // killed. On relaunch with too many attempts it must restore the backed-up + // app back across volumes to its original location. + __block NSURL *externalTargetURL; + + beforeEach(^{ + NSURL *diskImageURL = [self createAndMountDiskImageNamed:@"TestApplicationRollback" fromDirectory:self.testApplicationURL.URLByDeletingLastPathComponent]; + externalTargetURL = [diskImageURL URLByAppendingPathComponent:self.testApplicationURL.lastPathComponent]; + + // Simulate acquireTargetBundleURLForRequest: having moved the app off the + // external volume into system temp before ShipIt was killed. + NSURL *backupURL = [self.temporaryDirectoryURL URLByAppendingPathComponent:@"TestApplication Backup.app"]; + expect(@([NSFileManager.defaultManager moveItemAtURL:externalTargetURL toURL:backupURL error:NULL])).to(beTruthy()); + + SQRLCodeSignature *codeSignature = self.testApplicationSignature; + SQRLInstallerOwnedBundle *ownedBundle = [[SQRLInstallerOwnedBundle alloc] initWithOriginalURL:externalTargetURL temporaryURL:backupURL codeSignature:codeSignature]; + NSData *ownedBundleArchive = [NSKeyedArchiver archivedDataWithRootObject:ownedBundle]; + expect(ownedBundleArchive).notTo(beNil()); + + NSString *applicationIdentifier = self.shipItDirectoryManager.applicationIdentifier; + CFPreferencesSetValue((__bridge CFStringRef)SQRLShipItInstallationAttemptsKey, (__bridge CFPropertyListRef)@(4), (__bridge CFStringRef)applicationIdentifier, kCFPreferencesCurrentUser, kCFPreferencesCurrentHost); + CFPreferencesSetValue((__bridge CFStringRef)SQRLInstallerOwnedBundleKey, (__bridge CFDataRef)ownedBundleArchive, (__bridge CFStringRef)applicationIdentifier, kCFPreferencesCurrentUser, kCFPreferencesCurrentHost); + + BOOL synchronized = CFPreferencesSynchronize((__bridge CFStringRef)applicationIdentifier, kCFPreferencesCurrentUser, kCFPreferencesCurrentHost); + expect(@(synchronized)).to(beTruthy()); + }); + + it(@"should restore the app to the external volume after too many install attempts", ^{ + SQRLShipItRequest *request = [[SQRLShipItRequest alloc] initWithUpdateBundleURL:updateURL targetBundleURL:externalTargetURL bundleIdentifier:nil launchAfterInstallation:NO useUpdateBundleName:NO]; + [self installWithRequest:request remote:YES]; + + // App must be back at its original location on the external volume with + // the original version — not at the system-temp backup path. + NSURL *plistURL = [externalTargetURL URLByAppendingPathComponent:@"Contents/Info.plist"]; + expect([NSDictionary dictionaryWithContentsOfURL:plistURL][SQRLBundleShortVersionStringKey]).withTimeout(SQRLLongTimeout).toEventually(equal(SQRLTestApplicationOriginalShortVersionString)); + + __block NSError *verifyError; + expect(@([[self.testApplicationSignature verifyBundleAtURL:externalTargetURL] waitUntilCompleted:&verifyError])).toEventually(beTruthy()); + expect(verifyError).to(beNil()); + }); +}); + it(@"should disallow writing the updated application except by the owner", ^{ NSString *command = [NSString stringWithFormat:@"chmod -R 0777 '%@'", updateURL.path]; expect(@(system(command.UTF8String))).to(equal(@0));