Make uninstallation of extension robust

Bug: 1142584
Change-Id: Icdae6303b0b04df7c1f0ecc4abe6705cd85f3946
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2518450Reviewed-by: Rakesh Soma <rakeshsoma@google.com>
Commit-Queue: Yusuf Sengul <yusufsn@google.com>
Cr-Commit-Position: refs/heads/master@{#824541}

chrome/credential_provider/extension/extension_utils.cc

@@ -55,13 +55,19 @@ DWORD InstallGCPWExtension(const base::FilePath& extension_exe_path) {
       credential_provider::extension::OSServiceManager::Get();
   if (error_code == ERROR_SUCCESS) {
     if (service_status.dwCurrentState != SERVICE_STOPPED) {
-      error_code = service_manager->ControlService(SERVICE_CONTROL_STOP,
-                                                   &service_status);
+      error_code = service_manager->ControlService(SERVICE_CONTROL_STOP);
       if (error_code != ERROR_SUCCESS) {
         LOGFN(ERROR) << "service_manager->ControlService failed win32="
                      << error_code;
         return error_code;
       }
+
+      error_code = service_manager->WaitForServiceStopped();
+      if (error_code != ERROR_SUCCESS) {
+        LOGFN(ERROR) << "service_manager->WaitForServiceStopped failed win32="
+                     << error_code;
+        return error_code;
+      }
     }
 
     error_code = service_manager->DeleteService();
@@ -95,25 +101,45 @@ DWORD UninstallGCPWExtension() {
     return error_code;
   }
 
-  if (error_code == ERROR_SUCCESS) {
-    if (service_status.dwCurrentState != SERVICE_STOPPED) {
-      error_code = service_manager->ControlService(SERVICE_CONTROL_STOP,
-                                                   &service_status);
-      if (error_code != ERROR_SUCCESS) {
-        LOGFN(ERROR) << "service_manager->ControlService failed win32="
-                     << error_code;
-        return error_code;
-      }
+  if (error_code == ERROR_SERVICE_DOES_NOT_EXIST) {
+    LOGFN(WARNING) << "Service wasn't found when attempting to delete";
+    return ERROR_SUCCESS;
+  }
+
+  // Update the service start type to manual just in case anything goes wrong,
+  // but we don't want service keep running at Windows boot.
+  error_code = service_manager->ChangeServiceConfig(
+      SERVICE_NO_CHANGE, SERVICE_DEMAND_START, SERVICE_NO_CHANGE);
+
+  if (error_code != ERROR_SUCCESS) {
+    LOGFN(ERROR) << "service_manager->ChangeServiceConfig failed win32="
+                 << error_code;
+    return error_code;
+  }
+
+  if (service_status.dwCurrentState != SERVICE_STOPPED) {
+    error_code = service_manager->ControlService(SERVICE_CONTROL_STOP);
+    if (error_code != ERROR_SUCCESS) {
+      LOGFN(ERROR) << "service_manager->ControlService failed win32="
+        << error_code;
+      return error_code;
     }
 
-    error_code = service_manager->DeleteService();
+    error_code = service_manager->WaitForServiceStopped();
     if (error_code != ERROR_SUCCESS) {
-      LOGFN(ERROR) << "service_manager->DeleteService failed win32="
-                   << error_code;
+      LOGFN(ERROR) << "service_manager->WaitForServiceStopped failed win32="
+        << error_code;
       return error_code;
     }
   }
 
+  error_code = service_manager->DeleteService();
+  if (error_code != ERROR_SUCCESS) {
+    LOGFN(ERROR) << "service_manager->DeleteService failed win32="
+      << error_code;
+    return error_code;
+  }
+
   return ERROR_SUCCESS;
 }
 

chrome/credential_provider/extension/os_service_manager.cc

@@ -10,6 +10,12 @@
 namespace credential_provider {
 namespace extension {
 
+namespace {
+const unsigned int kServiceQueryWaitTimeMs = 100;
+// The number of iterations to poll if a service is stopped correctly.
+const unsigned int kMaxServiceQueryIterations = 100;
+}  // namespace
+
 OSServiceManager** OSServiceManager::GetInstanceStorage() {
   static OSServiceManager* instance = new OSServiceManager();
 
@@ -112,9 +118,48 @@ DWORD OSServiceManager::StartGCPWService() {
   return ERROR_SUCCESS;
 }
 
-DWORD OSServiceManager::ControlService(DWORD control,
-                                       SERVICE_STATUS* service_status) {
-  DCHECK(service_status);
+DWORD OSServiceManager::WaitForServiceStopped() {
+  LOGFN(VERBOSE);
+
+  ScopedScHandle scm_handle(
+      ::OpenSCManager(nullptr, nullptr, SC_MANAGER_ALL_ACCESS));
+  if (scm_handle.Get() == nullptr)
+    return ::GetLastError();
+
+  ScopedScHandle s_handle(::OpenService(
+      scm_handle.Get(), kGCPWExtensionServiceName, SERVICE_QUERY_STATUS));
+  if (s_handle.Get() == nullptr)
+    return ::GetLastError();
+
+  // Wait until the service is completely stopped.
+  for (unsigned int iteration = 0; iteration < kMaxServiceQueryIterations;
+       ++iteration) {
+    SERVICE_STATUS service_status;
+    if (!QueryServiceStatus(s_handle.Get(), &service_status)) {
+      DWORD error = ::GetLastError();
+      LOGFN(ERROR) << "QueryServiceStatus failed error=" << error;
+      return error;
+    }
+    if (service_status.dwCurrentState == SERVICE_STOPPED)
+      return ERROR_SUCCESS;
+
+    if (service_status.dwCurrentState != SERVICE_STOP_PENDING &&
+        service_status.dwCurrentState != SERVICE_RUNNING) {
+      LOGFN(ERROR) << "Cannot stop service state="
+                   << service_status.dwCurrentState;
+      return E_FAIL;
+    }
+    ::Sleep(kServiceQueryWaitTimeMs);
+  }
+
+  // The service didn't terminate.
+  LOGFN(ERROR) << "Stopping service timed out";
+
+  return E_FAIL;
+}
+
+DWORD OSServiceManager::ControlService(DWORD control) {
+  LOGFN(VERBOSE);
 
   ScopedScHandle scm_handle(
       ::OpenSCManager(nullptr, nullptr, SC_MANAGER_ALL_ACCESS));
@@ -128,9 +173,36 @@ DWORD OSServiceManager::ControlService(DWORD control,
   if (!s_handle.IsValid())
     return ::GetLastError();
 
-  if (!::ControlService(s_handle.Get(), control,
-                        (LPSERVICE_STATUS)&service_status))
+  SERVICE_STATUS service_status;
+  if (!::ControlService(s_handle.Get(), control, &service_status)) {
+    DWORD error = ::GetLastError();
+    LOGFN(ERROR) << "ControlService failed with error=" << error;
+    return error;
+  }
+
+  return ERROR_SUCCESS;
+}
+
+DWORD OSServiceManager::ChangeServiceConfig(DWORD dwServiceType,
+                                            DWORD dwStartType,
+                                            DWORD dwErrorControl) {
+  LOGFN(VERBOSE);
+
+  ScopedScHandle scm_handle(
+      ::OpenSCManager(nullptr, nullptr, SC_MANAGER_ALL_ACCESS));
+  if (!scm_handle.IsValid())
+    return ::GetLastError();
+
+  ScopedScHandle s_handle(::OpenService(
+      scm_handle.Get(), kGCPWExtensionServiceName, SERVICE_CHANGE_CONFIG));
+  if (!s_handle.IsValid())
+    return ::GetLastError();
+
+  if (!::ChangeServiceConfig(s_handle.Get(), dwServiceType, dwStartType,
+                             dwErrorControl, nullptr, nullptr, nullptr,
+                             nullptr, nullptr, nullptr, nullptr)) {
     return ::GetLastError();
+  }
 
   return ERROR_SUCCESS;
 }

chrome/credential_provider/extension/os_service_manager.h

@@ -41,12 +41,22 @@ class OSServiceManager {
   // Starts the GCPW extension using StartService Windows API.
   virtual DWORD StartGCPWService();
 
+  // Waits until service transitions to SERVICE_STOPPED state or the wait times
+  // out. Returns ERROR_SUCCESS if service is stopped successfully. Returns an
+  // error code in any other case.
+  virtual DWORD WaitForServiceStopped();
+
   // Calls the ControlService API to change the state of the service. |control|
   // needs to be one of the service controls as specified in documentation [1].
   // As a result |service_status| is returned that has the latest state of the
   // service. [1]
   // https://docs.microsoft.com/en-us/windows/win32/api/winsvc/nf-winsvc-controlservice
-  virtual DWORD ControlService(DWORD control, SERVICE_STATUS* service_status);
+  virtual DWORD ControlService(DWORD control);
+
+  // Updates the configuration of the service.
+  virtual DWORD ChangeServiceConfig(DWORD dwServiceType,
+                                    DWORD dwStartType,
+                                    DWORD dwErrorControl);
 
   // When the service control manager starts a service process, it waits for the
   // process to call the StartServiceCtrlDispatcher function. The main thread of

chrome/credential_provider/setup/setup_lib.cc

@@ -235,15 +235,15 @@ HRESULT DoUninstall(const base::FilePath& installer_path,
   has_failures |= FAILED(UnregisterDlls(dest_path, register_dlls.data(),
                                         register_dlls.size(), fakes));
 
-  has_failures |=
-      FAILED(HRESULT_FROM_WIN32(extension::UninstallGCPWExtension()));
-
   // If the DLLs are unregistered, Credential Provider will not be loaded by
   // Winlogon. Therefore, it is safe to delete the startup sentinel file at this
   // time.
   if (!has_failures)
     DeleteStartupSentinel();
 
+  has_failures |=
+      FAILED(HRESULT_FROM_WIN32(extension::UninstallGCPWExtension()));
+
   // Delete all files in the destination directory.  This directory does not
   // contain any configuration files or anything else user generated.
   if (!base::DeletePathRecursively(dest_path)) {

chrome/credential_provider/test/gcp_fakes.cc

@@ -1376,6 +1376,12 @@ DWORD FakeOSServiceManager::DeleteService() {
   return ERROR_SUCCESS;
 }
 
+DWORD FakeOSServiceManager::ChangeServiceConfig(DWORD dwServiceType,
+                                                DWORD dwStartType,
+                                                DWORD dwErrorControl) {
+  return ERROR_SUCCESS;
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 
 FakeTaskManager::FakeTaskManager()

chrome/credential_provider/test/gcp_fakes.h

@@ -691,6 +691,10 @@ class FakeOSServiceManager : public extension::OSServiceManager {
 
   DWORD DeleteService() override;
 
+  DWORD ChangeServiceConfig(DWORD dwServiceType,
+                            DWORD dwStartType,
+                            DWORD dwErrorControl) override;
+
  private:
   DWORD GetControlRequestForTesting() {
     std::unique_lock<std::mutex> lock(m);