Skip to content

InstallerProgressAppController: Workaround a corner case in which the bundle path of a running application contains Contents/MacOS/Executable #2726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

jeremyhu
Copy link

This handles a corner case in which [NSRunningApplication runningApplicationsWithBundleIdentifier:bundleIdentifier] returns us an instance in which the bundle path contains "Contents/MacOS/" by ignoring those trailing path components.

Fixes #2725

// Workaround cases where macOS appends Contents/MacOS/<executable> to the bundle path which can happen in corner cases
// https://github.com/sparkle-project/Sparkle/issues/2725
NSUInteger candidateCount = candidatePathComponents.count;
if ([candidatePathComponents[candidateCount - 3] isEqualToString:@"Contents"] && [candidatePathComponents[candidateCount - 2] isEqualToString:@"MacOS"]) {
Copy link
Member

@zorgiepoo zorgiepoo May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also test if candidatePathComponents[candidateCount - 1].pathExtension.length == 0 &&?
It's unlikely but I could technically create Contents/MacOS/ folder anywhere on my system and place an .app in there, or an app could be embedded in some other app's Contents/MacOS/... However it's unusual if the bundle path is not a bundle.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking candidatePathComponents[candidateCount - 1].pathExtension.length == 0 isn't right either. It's perfectly legal to have a '.' in the name of the executable. I'd suspect that is more likely than someone creating /Applications/Contents/MacOS/Foo.app, so I'm going to leave that check out.

It's unusual if the bundle path is not a bundle.

Yeah, I get that... This is a bug in macOS dealing incorrectly with our bundle layout and process bootstrapping. I can go into more details if you want, but it's more than I want to explain unless someone is actually interested in the gritty details.

Copy link
Member

@zorgiepoo zorgiepoo May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bar.app/Contents/MacOS/Foo.app is common for helper app Foo.app inside parent app Bar.app

Can we check if the path extension isn't ".app" which narrows it a bit more? We're trying to fix a narrow case while not introducing surprises for other cases which I hope people are not using (updating a helper app inside a parent app, and not having the parent app terminate), but don't want to later find out.

Yeah, I get that... This is a bug in macOS dealing incorrectly with our bundle layout and process bootstrapping. I can go into more details if you want, but it's more than I want to explain unless someone is actually interested in the gritty details.

The info that would allow me to create a repro case would be helpful, although I suspect I may have that if I try to create an app whose main binary exec's a helper binary? But if this is too much of a pain then so be it.

Copy link
Member

@zorgiepoo zorgiepoo Jun 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't reproduce the issue with Audacity (source) by the way. Its app has CFBundleExecutable set to Wrapper, which is a helper executable that then exec's main executable as far as I know. I also wasn't able to create a basic test app that simulated this / tripped up NSRunningApplication bundleURL. There's probably some more nuance here.

edit: Steam also does much weirder things by having its extensionless app bundle redirected in ~/Library/Application Support/ but the NSRunningApplication paths kind of make sense

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bar.app/Contents/MacOS/Foo.app is common for helper app Foo.app inside parent app Bar.app

Can we check if the path extension isn't ".app" which narrows it a bit more? We're trying to fix a narrow case while not introducing surprises for other cases which I hope people are not using (updating a helper app inside a parent app, and not having the parent app terminate), but don't want to later find out.

That feels a bit arbitrary... how about I check if the bundle path is a file vs a directory?

The info that would allow me to create a repro case would be helpful, although I suspect I may have that if I try to create an app whose main binary exec's a helper binary? But if this is too much of a pain then so be it.

So the case here is that we have two executables in Contents/MacOS, eg:

Foo.app/Contents/MacOS/Foo
Foo.app/Contents/MacOS/FooTrampoline

The bundle's Info.plist has CFBundleExecutable=FooTrampoline

Foo has an embedded __info_plist section in its mach-o that is identical to the bundle's Info.plist except CFBundleExecutable=Foo. Note if you're trying to make an Xcode project to do this, the app bundle target in Xcode does not honor CREATE_INFOPLIST_SECTION_IN_BINARY, so you need to do it via OTHER_LDFLAGS , eg: OTHER_LDFLAGS = $(inherited) -Xlinker -sectcreate -Xlinker __TEXT -Xlinker __info_plist -Xlinker $(DERIVED_FILE_DIR)/Foo-Info.plist ... also be aware that this really needs to be -Xlinker because of a bug in XCBuild that doesn't handle -Wl correctly when doing dependency tracking.

FooTrampoline in our case has main that kicks off some async work via GCD and then calls [NSRunLoop.mainRunLoop run]. That asynchronous work does some setup / install bits and then re-execs the main executable with posix_spawn, eg:

static void reexec_as_app(char * _Nonnull argv[], char * _Nonnull envp[]) {
    // Determine the path to our executable
    uint32_t executablePathSize = 0;
    (void)_NSGetExecutablePath(NULL, &executablePathSize);
    NSCAssert(executablePathSize > 0, @"Failed to determine path for main executable.");

    char * const executablePath = malloc(executablePathSize);
    int result = _NSGetExecutablePath(executablePath, &executablePathSize);
    NSCAssert(result == 0, @"Failed to determine path for main executable.");

    // chop off the "Trampoline" suffix to get the path to main executable
    executablePath[strlen(executablePath) - strlen("Trampoline")] = '\0';

    // Set argv[0] to be the executable path
    argv[0] = executablePath;

    // Spawn the real main executable
    posix_spawnattr_t attr;
    os_assumes_zero(posix_spawnattr_init(&attr));

    short flags = POSIX_SPAWN_SETEXEC;
    os_assumes_zero(posix_spawnattr_setflags(&attr, flags));

    pid_t pid;
    errno_t const err = posix_spawnp(&pid, executablePath, NULL /* &file_actions */, &attr, argv, envp);
    NSCAssert(err == 0, @"Failed to spawn main executable (%s): %d %s", executablePath, err, strerror(errno));
    __builtin_trap();
}

I don't reproduce the issue with Audacity (source) by the way. Its app has CFBundleExecutable set to Wrapper, which is a helper executable that then exec's main executable as far as I know. I also wasn't able to create a basic test app that simulated this / tripped up NSRunningApplication bundleURL. There's probably some more nuance here.

Does Audacity set the __info_plist section in their main binary? I needed to do that in order to get other things to function correctly in the system, but it's possible Audacity didn't hit those particular quirks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we fixed the macOS bug in Tahoe, so if you're trying to hit this, be sure you're on Sequoia.

Copy link
Member

@zorgiepoo zorgiepoo Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That feels a bit arbitrary... how about I check if the bundle path is a file vs a directory?

It's an IO check but this should be fine.

Does Audacity set the __info_plist section in their main binary? I needed to do that in order to get other things to function correctly in the system, but it's possible Audacity didn't hit those particular quirks.

I don't think so, and I think Audacity uses execve() rather than posix_spawn -- https://github.com/audacity/audacity/blob/master/au3/mac/Wrapper.c

Also, we fixed the macOS bug in Tahoe, so if you're trying to hit this, be sure you're on Sequoia.

That's great. I'd like to disable this workaround on macOS 26+ then, in similar spirit to other OS bug workarounds I've had to place in Sparkle code. Assuming rest of the PR is good, I may do this in a followup.

@jeremyhu jeremyhu force-pushed the eng/PR-2725 branch 2 times, most recently from 8c5cfa7 to 18ab0a1 Compare May 28, 2025 16:39
… bundle path of a running application contains Contents/MacOS/Executable

Fixed: sparkle-project#2725
Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
NSMutableArray<NSString *> *trimmedBundlePath = [candidatePathComponents mutableCopy];
[trimmedBundlePath removeObjectsInRange:NSMakeRange(candidatePathComponentsCount - 3, 3)];
candidatePathComponents = trimmedBundlePath;
candidatePathComponentsCount -= 3;
Copy link
Member

@zorgiepoo zorgiepoo Jun 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Product > Analyze generates this warning: Value stored to 'candidatePathComponentsCount' is never read

This variable doesn't seem to be used after fileExistsAtPath: check

Unfortunately CI will not pass if an analyzer warning exists. An easy change is moving candidatePathComponentsCount declaration inside fileExistsAtPath check and removing this line that -= 3.
(Normally the bot would make a comment here but there's permission issues if someone creates a branch outside of the repo)

Copy link
Member

@zorgiepoo zorgiepoo Jun 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we also removed placing running app bundles that hit this case towards the end of the array, once we start doing a check for the bundle being a directory, simplifying the workaround. I'm alright with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sparkle does not restart app after update completes (sandboxed app with SUEnableInstallerLauncherService=true)
2 participants