Bug 715910 - Set permissions for the service even on upgrades. r=rstrong

toolkit/components/maintenanceservice/serviceinstall.cpp

@@ -120,8 +120,7 @@ SvcInstall(SvcInstallAction action)
     return FALSE;
   }
 
-  // Check if we already have an open service
-  BOOL serviceAlreadyExists = FALSE;
+  // Check if we already have the service installed.
   nsAutoServiceHandle schService(OpenServiceW(schSCManager, 
                                               SVC_NAME, 
                                               SERVICE_ALL_ACCESS));
@@ -133,7 +132,15 @@ SvcInstall(SvcInstallAction action)
   }
   
   if (schService) {
-    serviceAlreadyExists = TRUE;
+    // The service exists but it may not have the correct permissions.
+    // This could happen if the permissions were not set correctly originally
+    // or have been changed after the installation.  This will reset the 
+    // permissions back to allow limited user accounts.
+    if (!SetUserAccessServiceDACL(schService)) {
+      LOG(("Could not reset security ACE on service handle. It might not be "
+           "possible to start the service. This error should never "
+           "happen.  (%d)\n", GetLastError()));
+    }
 
     // The service exists and we opened it
     DWORD bytesNeeded;
@@ -202,8 +209,10 @@ SvcInstall(SvcInstallAction action)
       // copy it in.  First try the safest / easiest way to overwrite the file.
       if (!CopyFileW(newServiceBinaryPath, 
                      serviceConfig.lpBinaryPathName, FALSE)) {
-        LOG(("WARNING: Could not overwrite old service binary file."
-             " (%d)\n", GetLastError()));
+        LOG(("Could not overwrite old service binary file. "
+             "This should never happen, but if it does the next upgrade will "
+             "fix it, the service is not a critical component that needs to be "
+             "installed for upgrades to work. (%d)\n", GetLastError()));
 
         // We rename the last 3 filename chars in an unsafe way.  Manually
         // verify there are more than 3 chars for safe failure in MoveFileExW.
@@ -272,44 +281,42 @@ SvcInstall(SvcInstallAction action)
              newServiceBinaryPath));
       }
       
-      // Setup the new module path
-      wcsncpy(newServiceBinaryPath, serviceConfig.lpBinaryPathName, MAX_PATH);
       return result;
-    } else {
-      // We don't need to copy ourselves to the existing location.
-      // The tmp file (the process of which we are executing right now) will be
-      // left over.  Attempt to delete the file on the next reboot.
-      MoveFileExW(newServiceBinaryPath, NULL, MOVEFILE_DELAY_UNTIL_REBOOT);
-      
-      return TRUE; // nothing to do, we already have a newer service installed
     }
-  } else if (UpgradeSvc == action) {
+
+    // We don't need to copy ourselves to the existing location.
+    // The tmp file (the process of which we are executing right now) will be
+    // left over.  Attempt to delete the file on the next reboot.
+    MoveFileExW(newServiceBinaryPath, NULL, MOVEFILE_DELAY_UNTIL_REBOOT);
+    
+    // nothing to do, we already have a newer service installed
+    return TRUE; 
+  }
+  
+  // If the service does not exist and we are upgrading, don't install it.
+  if (UpgradeSvc == action) {
     // The service does not exist and we are upgrading, so don't install it
     return TRUE;
   }
 
-  if (!serviceAlreadyExists) {
-    // Create the service as on demand
-    schService.own(CreateServiceW(schSCManager, SVC_NAME, SVC_DISPLAY_NAME,
-                                  SERVICE_ALL_ACCESS, SERVICE_WIN32_OWN_PROCESS,
-                                  SERVICE_DEMAND_START, SERVICE_ERROR_NORMAL,
-                                  newServiceBinaryPath, NULL, NULL, NULL, 
-                                  NULL, NULL));
-    if (!schService) {
-      LOG(("Could not create Windows service. "
-           "This error should never happen since a service install "
-           "should only be called when elevated. (%d)\n", GetLastError()));
-      return FALSE;
-    } 
-  }
+  // The service does not already exist so create the service as on demand
+  schService.own(CreateServiceW(schSCManager, SVC_NAME, SVC_DISPLAY_NAME,
+                                SERVICE_ALL_ACCESS, SERVICE_WIN32_OWN_PROCESS,
+                                SERVICE_DEMAND_START, SERVICE_ERROR_NORMAL,
+                                newServiceBinaryPath, NULL, NULL, NULL, 
+                                NULL, NULL));
+  if (!schService) {
+    LOG(("Could not create Windows service. "
+         "This error should never happen since a service install "
+         "should only be called when elevated. (%d)\n", GetLastError()));
+    return FALSE;
+  } 
 
-  if (schService) {
-    if (!SetUserAccessServiceDACL(schService)) {
-      LOG(("Could not set security ACE on service handle, the service will not"
-           "be able to be started from unelevated processes. "
-           "This error should never happen.  (%d)\n", 
-           GetLastError()));
-    }
+  if (!SetUserAccessServiceDACL(schService)) {
+    LOG(("Could not set security ACE on service handle, the service will not "
+         "be able to be started from unelevated processes. "
+         "This error should never happen.  (%d)\n", 
+         GetLastError()));
   }
 
   return TRUE;