Analyzer (false positive): `[SPUInstallerDriver extractDownloadedUpdate]` does not check if update is `SPUDownloadedUpdate `

[jozefizso]

Summary

There is a potential code path from [SPUCoreBasedUpdateDriver deferInformationalUpdate] to [SPUInstallerDriver extractDownloadedUpdate] where the downloadedUpdate variable of type SPUDownloadedUpdate might be a SPUInformationalUpdate instance and this will crash the app when calling members of the SPUDownloadedUpdate class.

SPUCoreBasedUpdateDriver.m class:

id<SPUResumableUpdate> _resumableUpdate;

- (void)deferInformationalUpdate:(SUAppcastItem *)updateItem secondaryUpdate:(SUAppcastItem * _Nullable)secondaryUpdateItem
{
    // 1. the _resumableUpdate is initialised as SPUInformationalUpdate
    _resumableUpdate = [[SPUInformationalUpdate alloc] initWithAppcastItem:updateItem secondaryAppcastItem:secondaryUpdateItem];
}

- (void)extractDownloadedUpdate
{
    assert(_resumableUpdate != nil);
    // 2. the _resumableUpdate might be SPUInformationalUpdate (or 
    [self extractUpdate:_resumableUpdate];
}

- (void)extractUpdate:(SPUDownloadedUpdate *)downloadedUpdate SPU_OBJC_DIRECT
{
    ...
    // 3. the downloadedUpdate could be instance of SPUInformationalUpdate (because of code paths 1. and 2.)
    [_installerDriver extractDownloadedUpdate:downloadedUpdate silently:_silentInstall completion:^(NSError * _Nullable error)
}

SPUInstallerDriver.m class:

- (void)extractDownloadedUpdate:(SPUDownloadedUpdate *)downloadedUpdate silently:(BOOL)silently completion:(void (^)(NSError * _Nullable))completionHandler
{
    _updateItem = downloadedUpdate.updateItem;
    // 4. this will crash if downloadedUpdate is instance of SPUInformationalUpdate which does not have downloadBookmarkData property
    _updateURLBookmarkData = downloadedUpdate.downloadBookmarkData;
}

Version

2.5.0

[zorgiepoo]

I cannot reproduce this issue with the Sparkle Test App if I opt into automatic downloading/installing of updates and encounter an informational-only update.

In basicDriverDidFindUpdateWithAppcastItem:secondaryAppcastItem: in the reply handler there is:

// Rule out invalid choices
SPUUserUpdateChoice validatedChoice;
if (updateItem.isInformationOnlyUpdate && userChoice == SPUUserUpdateChoiceInstall) {
    validatedChoice = SPUUserUpdateChoiceDismiss;
} else {
    validatedChoice = userChoice;
}

For extractDownloadedUpdate to be called below, validatedChoice needs to be SPUUserUpdateChoiceInstall. Even if you had a custom SPUUserDriver that incorrectly passed SPUUserUpdateChoiceInstall for an informational update, validatedChoice would still be SPUUserUpdateChoiceDismiss.

There's not enough info here to describe the repro case past this above logic.

[zorgiepoo]

Furthermore, the stage needs to also be SPUUserUpdateStageDownloaded for extractDownloadedUpdate to be called.

SPUUserUpdateStage stage;
// Major upgrades and information only updates are not downloaded automatically
if (_resumingDownloadedInfoOrUpdate && !updateItem.majorUpgrade && !updateItem.informationOnlyUpdate) {
    stage = SPUUserUpdateStageDownloaded;
} else if (_resumingInstallingUpdate) {
    stage = SPUUserUpdateStageInstalling;
} else {
    stage = SPUUserUpdateStageNotDownloaded;
}

According to this logic it's not possible for the stage to be SPUUserUpdateStageDownloaded if updateItem.informationOnlyUpdate is YES or if it's a major upgrade (which are the cases deferInformationalUpdate:secondaryUpdate: are called)

It's not clear if you encountered a problematic code path or found this from static analysis (sure, we could add a type assert in extractDownloadedUpdate)

[jozefizso]

We need to use Sparkle in a .dylib so we cannot use the full Sparkle.framework.

I was exploring options for using just parts of the Sparkle library which check for updates and either creating a partial ObjC library or even going for partial rewrite to Swift to have SPM library.

And yeah, this was caugh by the Swift analyzer that there are places were objects are cast in incopatible way.

[zorgiepoo]

So this isn't an actual issue then.

How is the swift analyzer run? Xcode's analyzer (Product > Analyze) doesn't warn about this for me.

We need to use Sparkle in a .dylib so we cannot use the full Sparkle.framework.

Extracting partial bits from the framework does not sound like a good idea to me, beyond configuring ConfigCommon.xcconfig.. A dylib included in an app bundle should not have issues using Sparkle so this statement on its own does not make sense. If on the other hand your constraint is you don't want to include a bundle of things then you may be out of luck -- part of installs being reliable and supporting installs requiring authentication is using separate helper tools that Sparkle provides.

[jozefizso]

In our use case we must build a standalone .dylib file.

And we must use parts of Sparkle without any UI.

[zorgiepoo]

Without any UI is resolvable by using your own SPUUserDriver. ConfigCommon.xcconfig has SPARKLE_BUILD_UI_BITS which can be turned off which will make a build of Sparkle.framework that doesn't require AppKit and not provide SPUStandardUserDriver.

Standalone dylib file on its own is not really resolvable. There are only two things that come to mind here. One is shipping a framework instead which includes Sparkle. The other is the dylib may be able to weakly link against Sparkle and use it if it is available, which would require the app using the dylib to embed Sparkle on its own.

[zorgiepoo]

One clarification: the app will need to show some UI if you want to support installs requiring authorization.

Silent installs are usually supported via SUAutomaticallyUpdate but the user driver may still be shown in some cases (one of these cases is if updates cannot be installed automatically (i.e, authorization)).