Retry extracting zip file without piping if extraction fails (#2616)

This is to workaround a bug in ditto prior to macOS 15.
sparkle-project · Sep 3, 2024 · 8f86cad · 8f86cad
1 parent 8bc4976
commit 8f86cad
Show file tree

Hide file tree

Showing 2 changed files with 165 additions and 52 deletions.
diff --git a/Autoupdate/SUPipedUnarchiver.m b/Autoupdate/SUPipedUnarchiver.m
@@ -20,7 +20,8 @@ @implementation SUPipedUnarchiver
     NSString *_extractionDirectory;
 }
 
-static NSArray <NSString *> * _Nullable _commandAndArgumentsConformingToTypeOfPath(NSString *path)
+// pipingData == NO is only supported for zip archives
+static NSArray<NSString *> * _Nullable _argumentsConformingToTypeOfPath(NSString *path, BOOL pipingData, NSString * __autoreleasing *outCommand)
 {
     NSArray <NSString *> *extractTGZ = @[@"/usr/bin/tar", @"-zxC"];
     NSArray <NSString *> *extractTBZ = @[@"/usr/bin/tar", @"-jxC"];
@@ -29,7 +30,7 @@ @implementation SUPipedUnarchiver
     // Note: keep this list in sync with generate_appcast's unarchiveUpdates()
     NSMutableDictionary <NSString *, NSArray<NSString *> *> *extractCommandDictionary =
     [@{
-      @".zip" : @[@"/usr/bin/ditto", @"-x",@"-k",@"-"],
+      @".zip" : (pipingData ? @[@"/usr/bin/ditto", @"-x", @"-k", @"-"] : @[@"/usr/bin/ditto", @"-x", @"-k", path]),
       @".tar" : @[@"/usr/bin/tar", @"-xC"],
       @".tar.gz" : extractTGZ,
       @".tgz" : extractTGZ,
@@ -64,15 +65,20 @@ @implementation SUPipedUnarchiver
     for (NSString *currentType in extractCommandDictionary)
     {
         if ([lastPathComponent hasSuffix:currentType]) {
-            return [extractCommandDictionary objectForKey:currentType];
+            NSArray<NSString *> *commandAndArguments = [extractCommandDictionary objectForKey:currentType];
+            if (outCommand != NULL) {
+                *outCommand = commandAndArguments.firstObject;
+            }
+
+            return [commandAndArguments subarrayWithRange:NSMakeRange(1, commandAndArguments.count - 1)];
         }
     }
     return nil;
 }
 
 + (BOOL)canUnarchivePath:(NSString *)path
 {
-    return _commandAndArgumentsConformingToTypeOfPath(path) != nil;
+    return _argumentsConformingToTypeOfPath(path, YES, NULL) != nil;
 }
 
 + (BOOL)mustValidateBeforeExtractionWithArchivePath:(NSString *)archivePath
@@ -92,60 +98,136 @@ - (instancetype)initWithArchivePath:(NSString *)archivePath extractionDirectory:
 
 - (void)unarchiveWithCompletionBlock:(void (^)(NSError * _Nullable))completionBlock progressBlock:(void (^ _Nullable)(double))progressBlock
 {
-    NSArray <NSString *> *commandAndArguments = _commandAndArgumentsConformingToTypeOfPath(_archivePath);
-    assert(commandAndArguments != nil);
-
-    NSString *command = commandAndArguments.firstObject;
+    NSString *command = nil;
+    NSArray<NSString *> *arguments = _argumentsConformingToTypeOfPath(_archivePath, YES, &command);
+    assert(arguments != nil);
     assert(command != nil);
 
-    NSArray <NSString *> *arguments = [commandAndArguments subarrayWithRange:NSMakeRange(1, commandAndArguments.count - 1)];
-
     dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
-        SUUnarchiverNotifier *notifier = [[SUUnarchiverNotifier alloc] initWithCompletionBlock:completionBlock progressBlock:progressBlock];
-        [self extractArchivePipingDataToCommand:command arguments:arguments notifier:notifier];
+        @autoreleasepool {
+            SUUnarchiverNotifier *notifier = [[SUUnarchiverNotifier alloc] initWithCompletionBlock:completionBlock progressBlock:progressBlock];
+            NSError *extractError = nil;
+            if ([self extractArchivePipingData:YES command:command arguments:arguments notifier:notifier error:&extractError]) {
+                [notifier notifySuccess];
+            } else {
+                // If we fail due to an IO PIPE write failure for zip files,
+                // we may re-attempt extracting the archive without piping archive data
+                // (and without fine grained progress reporting).
+                // This is to workaround a bug in ditto which causes extraction to fail when piping data for
+                // some types of constructed zip files.
+                // Note this bug is fixed on macOS 15+, so this workaround is not needed there.
+                // https://github.com/sparkle-project/Sparkle/issues/2544
+                BOOL useNonPipingWorkaround;
+                if (@available(macOS 15, *)) {
+                    useNonPipingWorkaround = NO;
+                } else {
+                    NSError *underlyingError = extractError.userInfo[NSUnderlyingErrorKey];
+
+                    useNonPipingWorkaround = [self->_archivePath.pathExtension isEqualToString:@"zip"] && ([extractError.domain isEqualToString:SUSparkleErrorDomain] && extractError.code == SUUnarchivingError && [underlyingError.domain isEqualToString:NSPOSIXErrorDomain] && underlyingError.code == EPIPE);
+                }
+
+                if (!useNonPipingWorkaround) {
+                    [notifier notifyFailureWithError:extractError];
+                } else {
+                    // Re-create the extraction directory, then try extracting without piping
+
+                    NSFileManager *fileManager = [NSFileManager defaultManager];
+
+                    NSURL *extractionDirectoryURL = [NSURL fileURLWithPath:self->_extractionDirectory isDirectory:YES];
+
+                    NSError *removeError = nil;
+                    if (![fileManager removeItemAtURL:extractionDirectoryURL error:&removeError]) {
+                        SULog(SULogLevelError, @"Failed to remove extraction directory path for non-piping workaround with error: %@", removeError);
+
+                        [notifier notifyFailureWithError:extractError];
+                    } else {
+                        NSError *createError = nil;
+                        if (![fileManager createDirectoryAtPath:self->_extractionDirectory withIntermediateDirectories:NO attributes:nil error:&createError]) {
+                            SULog(SULogLevelError, @"Failed to create new extraction directory path for non-piping workaround with error: %@", createError);
+
+                            [notifier notifyFailureWithError:extractError];
+                        } else {
+                            // The ditto command will be the same so no need to fetch it again
+                            NSArray<NSString *> *nonPipingArguments = _argumentsConformingToTypeOfPath(self->_archivePath, NO, NULL);
+                            assert(nonPipingArguments != nil);
+
+                            NSError *nonPipingExtractError = nil;
+                            if ([self extractArchivePipingData:NO command:command arguments:nonPipingArguments notifier:notifier error:&nonPipingExtractError]) {
+                                [notifier notifySuccess];
+                            } else {
+                                [notifier notifyFailureWithError:nonPipingExtractError];
+                            }
+                        }
+                    }
+                }
+            }
+        }
     });
 }
 
 // This method abstracts the types that use a command line tool piping data from stdin.
-- (void)extractArchivePipingDataToCommand:(NSString *)command arguments:(NSArray*)args notifier:(SUUnarchiverNotifier *)notifier SPU_OBJC_DIRECT
+- (BOOL)extractArchivePipingData:(BOOL)pipingData command:(NSString *)command arguments:(NSArray*)args notifier:(SUUnarchiverNotifier *)notifier error:(NSError * __autoreleasing *)outError SPU_OBJC_DIRECT
 {
     // *** GETS CALLED ON NON-MAIN THREAD!!!
-	@autoreleasepool {
-        NSString *destination = _extractionDirectory;
-        
-        NSFileManager *fileManager = [NSFileManager defaultManager];
-        
+    NSString *destination = _extractionDirectory;
+
+    NSFileManager *fileManager = [NSFileManager defaultManager];
+
+    if (pipingData) {
         SULog(SULogLevelDefault, @"Extracting using '%@' '%@' < '%@' '%@'", command, [args componentsJoinedByString:@"' '"], _archivePath, destination);
-
-        // Get the file size.
+    } else {
+        SULog(SULogLevelDefault, @"Extracting using '%@' '%@' '%@'", command, [args componentsJoinedByString:@"' '"], destination);
+    }
+
+    // Get expected file size for piping the archive
+    NSUInteger expectedLength;
+    if (pipingData) {
         NSDictionary *attributes = [fileManager attributesOfItemAtPath:_archivePath error:nil];
-        NSUInteger expectedLength = [(NSNumber *)[attributes objectForKey:NSFileSize] unsignedIntegerValue];
+        expectedLength = [(NSNumber *)[attributes objectForKey:NSFileSize] unsignedIntegerValue];
 
         if (expectedLength == 0) {
             NSError *error = [NSError errorWithDomain:SUSparkleErrorDomain code:SUUnarchivingError userInfo:@{ NSLocalizedDescriptionKey:[NSString stringWithFormat:@"Extraction failed, archive '%@' is empty", _archivePath]}];
 
-            [notifier notifyFailureWithError:error];
-            return;
+            if (outError != NULL) {
+                *outError = error;
+            }
+
+            return NO;
         }
-
-        NSFileHandle *archiveInput = [NSFileHandle fileHandleForReadingAtPath:_archivePath];
-
-        NSPipe *pipe = [NSPipe pipe];
-        NSTask *task = [[NSTask alloc] init];
+    } else {
+        expectedLength = 0;
+    }
+
+    NSTask *task = [[NSTask alloc] init];
+
+    NSPipe *pipe;
+    if (pipingData) {
+        pipe = [NSPipe pipe];
         [task setStandardInput:pipe];
-        [task setStandardError:[NSFileHandle fileHandleWithStandardError]];
-        [task setStandardOutput:[NSFileHandle fileHandleWithStandardOutput]];
-        [task setLaunchPath:command];
-        [task setArguments:[args arrayByAddingObject:destination]];
-
-        NSError *launchError = nil;
-        if (![task launchAndReturnError:&launchError]) {
-            [notifier notifyFailureWithError:launchError];
-            return;
+    } else {
+        pipe = nil;
+    }
+
+    [task setStandardError:[NSFileHandle fileHandleWithStandardError]];
+    [task setStandardOutput:[NSFileHandle fileHandleWithStandardOutput]];
+    [task setLaunchPath:command];
+    [task setArguments:[args arrayByAddingObject:destination]];
+
+    NSError *launchError = nil;
+    if (![task launchAndReturnError:&launchError]) {
+        if (outError != NULL) {
+            *outError = launchError;
         }
+        return NO;
+    }
+
+    NSError *underlyingOutError = nil;
+    NSUInteger bytesWritten = 0;
+
+    if (pipingData) {
+        NSFileHandle *archiveInput = [NSFileHandle fileHandleForReadingAtPath:_archivePath];
 
         NSFileHandle *archiveOutput = [pipe fileHandleForWriting];
-        NSUInteger bytesWritten = 0;
 
 #if MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_15
         BOOL hasIOErrorMethods;
@@ -177,6 +259,7 @@ - (void)extractArchivePipingDataToCommand:(NSString *)command arguments:(NSArray
 #pragma clang diagnostic pop
                 if (data == nil) {
                     SULog(SULogLevelError, @"Failed to read data from archive with error %@", readError);
+                    underlyingOutError = readError;
                 }
             }
 
@@ -192,6 +275,14 @@ - (void)extractArchivePipingDataToCommand:(NSString *)command arguments:(NSArray
                     [archiveOutput writeData:data];
                 } @catch (NSException *exception) {
                     SULog(SULogLevelError, @"Failed to write data to pipe with exception reason %@", exception.reason);
+
+                    if ([exception.name isEqualToString:NSFileHandleOperationException]) {
+                        NSError *underlyingFileHandleError = exception.userInfo[@"NSFileHandleOperationExceptionUnderlyingError"];
+                        NSError *underlyingPOSIXError = underlyingFileHandleError.userInfo[NSUnderlyingErrorKey];
+
+                        underlyingOutError = underlyingPOSIXError;
+                    }
+
                     break;
                 }
             }
@@ -203,6 +294,10 @@ - (void)extractArchivePipingDataToCommand:(NSString *)command arguments:(NSArray
                 if (![archiveOutput writeData:data error:&writeError]) {
 #pragma clang diagnostic pop
                     SULog(SULogLevelError, @"Failed to write data to pipe with error %@", writeError);
+
+                    NSError *underlyingPOSIXError = writeError.userInfo[NSUnderlyingErrorKey];
+
+                    underlyingOutError = underlyingPOSIXError;
                     break;
                 }
             }
@@ -238,25 +333,42 @@ - (void)extractArchivePipingDataToCommand:(NSString *)command arguments:(NSArray
             [archiveInput closeFile];
         }
 #endif
+    }
+
+    [task waitUntilExit];
+
+    if ([task terminationStatus] != 0) {
+        NSMutableDictionary *userInfo = [@{ NSLocalizedDescriptionKey:[NSString stringWithFormat:@"Extraction failed, command '%@' returned %d", command, [task terminationStatus]]} mutableCopy];
 
-        [task waitUntilExit];
-
-        if ([task terminationStatus] != 0) {
-            NSError *error = [NSError errorWithDomain:SUSparkleErrorDomain code:SUUnarchivingError userInfo:@{ NSLocalizedDescriptionKey:[NSString stringWithFormat:@"Extraction failed, command '%@' returned %d", command, [task terminationStatus]]}];
-
-            [notifier notifyFailureWithError:error];
-            return;
+        if (underlyingOutError != nil) {
+            userInfo[NSUnderlyingErrorKey] = underlyingOutError;
         }
 
-        if (bytesWritten != expectedLength) {
-            NSError *error = [NSError errorWithDomain:SUSparkleErrorDomain code:SUUnarchivingError userInfo:@{ NSLocalizedDescriptionKey:[NSString stringWithFormat:@"Extraction failed, command '%@' got only %ld of %ld bytes", command, (long)bytesWritten, (long)expectedLength]}];
-
-            [notifier notifyFailureWithError:error];
-            return;
+        NSError *error = [NSError errorWithDomain:SUSparkleErrorDomain code:SUUnarchivingError userInfo:[userInfo copy]];
+
+        if (outError != NULL) {
+            *outError = error;
         }
 
-        [notifier notifySuccess];
+        return NO;
     }
+
+    if (!pipingData) {
+        [notifier notifyProgress:1.0];
+    } else if (bytesWritten != expectedLength) {
+        // Don't set underlying error in this case
+        // This may fail due to a write PIPE error but we don't currently support extracting archives that have
+        // extraneous data leftover because these may be corrupt.
+        // We don't want to later workaround extraction by not piping the data.
+        NSError *error = [NSError errorWithDomain:SUSparkleErrorDomain code:SUUnarchivingError userInfo:@{ NSLocalizedDescriptionKey:[NSString stringWithFormat:@"Extraction failed, command '%@' got only %ld of %ld bytes", command, (long)bytesWritten, (long)expectedLength]}];
+
+        if (outError != NULL) {
+            *outError = error;
+        }
+        return NO;
+    }
+
+    return YES;
 }
 
 - (NSString *)description { return [NSString stringWithFormat:@"%@ <%@>", [self class], _archivePath]; }

diff --git a/Tests/SUUnarchiverTest.swift b/Tests/SUUnarchiverTest.swift
@@ -43,7 +43,8 @@ class SUUnarchiverTest: XCTestCase
     }
 
     func unarchiveNonExistentFileTestFailureAppWithExtension(_ archiveExtension: String, tempDirectoryURL: URL, password: String?, expectingInstallationType installationType: String, testExpectation: XCTestExpectation) {
-        let tempArchiveURL = tempDirectoryURL.appendingPathComponent("error-invalid").appendingPathExtension(archiveExtension)
+        let tempArchiveURL = tempDirectoryURL.deletingLastPathComponent().appendingPathComponent("error-invalid").appendingPathExtension(archiveExtension)
+
         let unarchiver = SUUnarchiver.unarchiver(forPath: tempArchiveURL.path, extractionDirectory: tempDirectoryURL.path, updatingHostBundlePath: nil, decryptionPassword: password, expectingInstallationType: installationType)!
 
         unarchiver.unarchive(completionBlock: {(error: Error?) -> Void in