-
Notifications
You must be signed in to change notification settings - Fork 140
fix: remove ShipIt's launchd job once installation completes #331
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,10 @@ | |
| #include <sys/wait.h> | ||
| #include <mach/mach.h> | ||
| #include <servers/bootstrap.h> | ||
| #include <signal.h> | ||
| #include <unistd.h> | ||
|
|
||
| #import <ServiceManagement/ServiceManagement.h> | ||
|
|
||
| #import "NSError+SQRLVerbosityExtensions.h" | ||
| #import "RACSignal+SQRLTransactionExtensions.h" | ||
|
|
@@ -87,6 +91,50 @@ static void drainMachServicePort(const char *serviceName) { | |
| } | ||
| } | ||
|
|
||
| // Remove ShipIt's own launchd job on the way out. A job submitted with | ||
| // SMJobSubmit is a registration, not a one-shot: after ShipIt exits, the | ||
| // job would otherwise stay in the domain as "not running" until the login | ||
| // session ends (or indefinitely, for the system domain), and macOS 27 shows | ||
| // a Dock tile for any app with a registered background job — making an | ||
| // updated-and-quit app look like it is still running. Removal is limited to | ||
| // the terminal success paths; failure exits must keep the registration so | ||
| // the KeepAlive/SuccessfulExit policy can respawn ShipIt to retry. | ||
| // | ||
| // SMJobRemove is deprecated but is the counterpart of the SMJobSubmit that | ||
| // created the job (SQRLShipItLauncher); there is no other API that can | ||
| // remove a submitted job. | ||
| static void removeOwnLaunchdJob(NSString *jobLabel) { | ||
| // launchd terminates a running job as part of removing it. Ignore | ||
| // SIGTERM so we still exit through our own exit() call with the | ||
| // intended status rather than dying by signal mid-cleanup. | ||
| signal(SIGTERM, SIG_IGN); | ||
|
|
||
| // Privileged installs run ShipIt as root in the system domain; | ||
| // unprivileged ones run as the user in their domain. | ||
| // | ||
| // The authorization is deliberately NULL. By this point the installer | ||
| // has replaced (and typically deleted) the bundle containing this | ||
| // running ShipIt binary, so authd can no longer validate our code | ||
| // signature on disk and AuthorizationCreate fails with | ||
| // errAuthorizationDenied, even for root (verified empirically). | ||
| // SMJobRemove with a NULL authorization instead authorizes on the | ||
| // caller itself — root may modify the system domain and any caller its | ||
| // own user domain — which neither consults authd nor can present a | ||
| // prompt, and works with the binary already gone. | ||
| CFStringRef domain = (geteuid() == 0 ? kSMDomainSystemLaunchd : kSMDomainUserLaunchd); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. q: this only clears the domain this run lives in. Someone who previously did an admin-prompted (system-domain) update keeps that stale job indefinitely and later unprivileged runs can't remove it, so the Dock tile could persist for them. Intentional / worth a note? |
||
|
|
||
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wdeprecated-declarations" | ||
| // `wait` must be false: waiting blocks until the job (i.e. this | ||
| // process) exits, which would deadlock against our own exit(). | ||
| CFErrorRef cfError = NULL; | ||
| if (!SMJobRemove(domain, (__bridge CFStringRef)jobLabel, NULL, false, &cfError)) { | ||
| NSError *error = CFBridgingRelease(cfError); | ||
| NSLog(@"Could not remove ShipIt launchd job %@: %@", jobLabel, error); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: worth swallowing |
||
| } | ||
| #pragma clang diagnostic pop | ||
| } | ||
|
|
||
| // Waits for all instances of the target application (as described in the | ||
| // `request`) to exit, then sends completed. | ||
| static RACSignal *waitForTerminationIfNecessary(SQRLShipItRequest *request) { | ||
|
|
@@ -231,13 +279,15 @@ static void installRequest(RACSignal *readRequestSignal, NSString *applicationId | |
| NSLog(@"Installation cancelled: %@", error); | ||
| clearInstallationAttempts(applicationIdentifier); | ||
| drainMachServicePort(applicationIdentifier.UTF8String); | ||
| removeOwnLaunchdJob(applicationIdentifier); | ||
| exit(EXIT_SUCCESS); | ||
| } else { | ||
| NSLog(@"Installation error: %@", error); | ||
| exit(EXIT_FAILURE); | ||
| } | ||
| } completed:^{ | ||
| drainMachServicePort(applicationIdentifier.UTF8String); | ||
| removeOwnLaunchdJob(applicationIdentifier); | ||
| exit(EXIT_SUCCESS); | ||
| }]; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -236,13 +236,17 @@ - (NSRunningApplication *)launchTestApplicationWithEnvironment:(NSDictionary *)e | |
|
|
||
| // Remove ShipIt's launchd job so it doesn't relaunch itself. | ||
| // SMJobRemove is deprecated but has no test-suitable replacement | ||
| // (SMAppService requires registration via the same API). | ||
| // (SMAppService requires registration via the same API). The job | ||
| // is usually already gone — ShipIt removes it on success — so a | ||
| // job-not-found error is expected and not worth logging. | ||
| CFErrorRef error = NULL; | ||
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wdeprecated-declarations" | ||
| if (!SMJobRemove(kSMDomainUserLaunchd, CFSTR("com.github.Squirrel.TestApplication.ShipIt"), NULL, true, &error)) { | ||
| NSLog(@"Could not remove ShipIt job after tests: %@", error); | ||
| if (error != NULL) CFRelease(error); | ||
| NSError *removeError = CFBridgingRelease(error); | ||
| if (![removeError.domain isEqual:(__bridge id)kSMErrorDomainLaunchd] || removeError.code != kSMErrorJobNotFound) { | ||
| NSLog(@"Could not remove ShipIt job after tests: %@", removeError); | ||
| } | ||
| } | ||
| #pragma clang diagnostic pop | ||
| }]; | ||
|
|
@@ -338,7 +342,10 @@ - (void)submitShipItRequest:(SQRLShipItRequest *)request { | |
| #pragma clang diagnostic ignored "-Wdeprecated-declarations" | ||
| static NSNumber *SQRLShipItLastExitStatus(NSString *jobLabel) { | ||
| NSDictionary *job = CFBridgingRelease(SMJobCopyDictionary(kSMDomainUserLaunchd, (__bridge CFStringRef)jobLabel)); | ||
| if (job == nil || job[@"PID"] != nil) return nil; | ||
| // ShipIt removes its own launchd job when it finishes successfully, so | ||
| // once the job has been submitted, its disappearance means a clean exit. | ||
| if (job == nil) return @0; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this makes "never submitted" and "succeeded and removed" indistinguishable, so a launch failure now sails through |
||
| if (job[@"PID"] != nil) return nil; | ||
| return job[@"LastExitStatus"]; | ||
| } | ||
| #pragma clang diagnostic pop | ||
|
|
@@ -348,8 +355,9 @@ - (void)waitForShipItJobToExitWithLabel:(NSString *)jobLabel { | |
| // spawn — it does not wait for ShipIt to actually run. Block until launchd | ||
| // reports the job has exited so callers can assert on the install result | ||
| // synchronously instead of racing Nimble's default 1s poll timeout. | ||
| // LastExitStatus only appears once the process has run and exited, which | ||
| // avoids the brief no-PID window before launchd spawns it. | ||
| // LastExitStatus only appears once the process has run and exited (and the | ||
| // job disappears entirely on a successful run), which avoids the brief | ||
| // no-PID window before launchd spawns it. | ||
| expect(SQRLShipItLastExitStatus(jobLabel)).withTimeout(SQRLLongTimeout).toEventuallyNot(beNil()); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
|
|
||
| #import "QuickSpec+SQRLFixtures.h" | ||
|
|
||
| #import <ServiceManagement/ServiceManagement.h> | ||
| #import <sys/xattr.h> | ||
|
|
||
| @interface SQRLInstaller (SQRLTestingHooks) | ||
|
|
@@ -53,6 +54,23 @@ - (RACSignal *)deleteOwnedBundleAtURL:(NSURL *)bundleURL; | |
| expect(self.testApplicationBundleVersion).to(equal(SQRLTestApplicationUpdatedShortVersionString)); | ||
| }); | ||
|
|
||
| it(@"should remove its launchd job once the install completes", ^{ | ||
| SQRLShipItRequest *request = [[SQRLShipItRequest alloc] initWithUpdateBundleURL:updateURL targetBundleURL:self.testApplicationURL bundleIdentifier:nil launchAfterInstallation:NO useUpdateBundleName:NO]; | ||
|
|
||
| [self installWithRequest:request remote:YES]; | ||
|
|
||
| expect(self.testApplicationBundleVersion).to(equal(SQRLTestApplicationUpdatedShortVersionString)); | ||
|
|
||
| // ShipIt removes its own job on success (SMJobRemove with wait=false), | ||
| // so the registration may lag its exit by a moment — poll for it to | ||
| // disappear rather than asserting immediately. A lingering registration | ||
| // causes macOS 27 to show a Dock tile for the updated app after quit. | ||
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wdeprecated-declarations" | ||
| expect(CFBridgingRelease(SMJobCopyDictionary(kSMDomainUserLaunchd, (__bridge CFStringRef)self.shipItDirectoryManager.applicationIdentifier))).withTimeout(SQRLLongTimeout).toEventually(beNil()); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: in the suite ShipIt runs from the test host's Squirrel.framework, so the running binary isn't inside the bundle being replaced. The production case (binary swapped for a differently-signed ShipIt at the same path, user domain, NULL auth) is only covered by the manual probes in the description — fine, just noting it isn't under CI. |
||
| #pragma clang diagnostic pop | ||
| }); | ||
|
|
||
| it(@"should round-trip the owned bundle through CFPreferences", ^{ | ||
| SQRLInstaller *installer = [[SQRLInstaller alloc] initWithApplicationIdentifier:self.shipItDirectoryManager.applicationIdentifier]; | ||
| SQRLInstallerOwnedBundle *original = [[SQRLInstallerOwnedBundle alloc] initWithOriginalURL:self.testApplicationURL temporaryURL:updateURL codeSignature:self.testApplicationSignature]; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q: if the install lands during logout/restart (the on-demand-only case) and the
SMJobRemoveIPC stalls, we now ignore launchd's TERM and hold the session forExitTimeOutuntil KILL. Probably fine, but analarm()here would bound it.