711140 - Use maintenance service if the fallback key is present so tests can run

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

If fallback key is present always use the service when pending-service state is written, even if we already have write access to the installation directory.

This is needed before we can land bug 481815 and friends so that Ehsan's tests can run. I plan to do this today.  The patch should be very small.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

This patch fixes tests so they work after the newly added security mechanisms.

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 582171 [details] [diff] [review]
Patch v1.

>diff --git a/toolkit/mozapps/update/common/pathhash.h b/toolkit/mozapps/update/common/pathhash.h
>--- a/toolkit/mozapps/update/common/pathhash.h
>+++ b/toolkit/mozapps/update/common/pathhash.h
>@@ -44,9 +44,12 @@
>  * @param  filePath     The input file path to get a registry path from
>  * @param  registryPath A buffer to write the registry path to, must 
>  *                      be of size in WCHARs MAX_PATH + 1
>  * @return TRUE if successful
> */
> BOOL CalculateRegistryPathFromFilePath(const LPCWSTR filePath, 
>                                        LPWSTR registryPath);
> 
>+#define FallbackKey \
>+  L"SOFTWARE\\Mozilla\\MaintenanceService\\3932ecacee736d366d6436db0f55bce4"
You have both FallbackKey and fallbackKey which can be confusing. For this I think FallbackKeyPath or similar would be better

I know I didn't catch this until now so if it is a pain don't bother but it would be better to replace Fallback / fallback with Test / test throughout so it is clear that this isn't used as a fallback for normal updates.

Add a comment describing what this is used for.

>+
> #endif
>diff --git a/toolkit/mozapps/update/test/unit/head_update.js.in b/toolkit/mozapps/update/test/unit/head_update.js.in
>--- a/toolkit/mozapps/update/test/unit/head_update.js.in
>+++ b/toolkit/mozapps/update/test/unit/head_update.js.in
>@@ -542,33 +542,48 @@ function runUpdateUsingService(aInitialS
>...
>-    if (status == STATE_PENDING) {
>+    if (status == STATE_APPLYING) {
Do you think this could race where the status file isn't changed in time to STATE_APPLYING before this check? If it is a possibility it might be best to check for both STATE_PENDING and STATE_APPLYING.

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>@@ -1595,20 +1595,34 @@ int NS_main(int argc, NS_tchar **argv)
>   gSourcePath = argv[1];
> 
> #ifdef XP_WIN
>   // Disable every privilege we don't need. Processes started using
>   // CreateProcess will use the same token as this process.
>   UACHelper::DisablePrivileges(NULL);
> 
>   bool useService = false;
>+  bool fallbackKeyExists = false;
>+
>   // We never want the service to be used unless we build with
>   // the maintenance service.
> #ifdef MOZ_MAINTENANCE_SERVICE
>   IsUpdateStatusPending(useService);
>+  // Our tests run with a different apply directory for each test.
>+  // We use this registry key on our test slaves to store the 
>+  // allowed name/issuers.
>+  HKEY fallbackKey;
>+  LSTATUS retCode = RegOpenKeyExW(HKEY_LOCAL_MACHINE, 
>+                                  FallbackKey, 0,
>+                                  KEY_READ | KEY_WOW64_64KEY, &fallbackKey);
The return type for RegOpenKeyExW is LONG let's just go with LONG here and elsewhere. Since I missed this previously could you check the patches for other instances such as registrycertificates.cpp?

Alternatively, you could just do this in the if statement itself since retCode is only used in the if statement here... likely the same in the other uses as well.

>+  if (ERROR_SUCCESS == retCode) {
>+    fallbackKeyExists = true;
>+    RegCloseKey(fallbackKey);
>+  }

I'd like a quick skim over this again before r+'ing though I am tempted to just r+ with nit fixed.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Implemented review comments, thanks for the quick review.

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 582254 [details] [diff] [review]
Patch v2.

Looks good and thanks.

Comment 5

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/d4043fc86503