COMRegister() probes the COM+ catalog to decide whether to unregister
first, but it is the SCM that blocks installation when a service of the
same name is left over.  If the COM+ application is gone and the service
survives, install fails with 0x80070431 and never recovers.

 - Check the SCM as well as the COM+ catalog in COMRegister().
 - Delete the provider service in COMUnregister() and at the
   DllUnregisterServer() call sites in DllRegisterServer().

Opening the SCM, opening the provider service and closing both handles
is boilerplate that StopService() already contains, so move it into
QGAProviderOpenService(), which returns a single service handle and
reports the win32 error through an out parameter, and convert
StopService() to it.  The service handle stays valid after the SCM
handle is closed, so the helper closes it and each caller is left with
one handle to release.

While at it, fix two pre-existing problems in StopService():

 - It logged errmsg(E_FAIL, ...) and returned E_FAIL for every failure,
   discarding the GetLastError() value.  It now passes the win32 error
   code to errmsg() and returns HRESULT_FROM_WIN32().
 - It opened the SCM with SC_MANAGER_ALL_ACCESS, which implies
   SC_MANAGER_CREATE_SERVICE and is therefore refused to
   non-administrators, and passed that same SCM mask to OpenService()
   as a service access mask.  Each operation now requests only what it
   needs: SC_MANAGER_CONNECT on the SCM, and SERVICE_QUERY_CONFIG,
   DELETE or SERVICE_STOP on the service.

StopService()'s return value changes from E_FAIL to a specific
HRESULT_FROM_WIN32(); its only caller, in requester.cpp, ignores it.

Signed-off-by: Abed Agbaria <aagbaria@redhat.com>
---
 qga/vss-win32/install.cpp | 122 ++++++++++++++++++++++++++++++++------
 1 file changed, 105 insertions(+), 17 deletions(-)

v1: https://lore.kernel.org/qemu-devel/20260915145320.215663-1-aagbaria@redhat.com/

v2:
 - Pass win32 error codes to errmsg() consistently instead of mixing
   error codes and HRESULTs (Marc-André)
 - Factor the SCM/OpenService boilerplate into QGAProviderOpenService()
   and convert the existing StopService() to it (Marc-André)
 - Restrict StopService() to the access rights it actually needs

Tested on a Windows guest by replacing the installed build with this
one and running a full install/uninstall cycle, including installation
over a leftover provider service.

diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index 5b7a8e9bc5..cfafc2d95e 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -241,6 +241,96 @@ out:
     return hr;
 }
 
+/*
+ * Open the QGA VSS provider service with the requested access rights.
+ *
+ * On success, return a service handle which the caller must release with
+ * CloseServiceHandle().  On failure, return NULL and store the win32 error
+ * code in *err.  ERROR_SERVICE_DOES_NOT_EXIST just means the service is not
+ * installed, which is a normal condition for all callers, so it is not
+ * reported here; the caller decides what it means.
+ */
+static SC_HANDLE QGAProviderOpenService(DWORD access, DWORD *err)
+{
+    qga_debug_begin;
+
+    SC_HANDLE service = NULL;
+    SC_HANDLE manager = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT);
+
+    *err = ERROR_SUCCESS;
+
+    if (!manager) {
+        *err = GetLastError();
+        errmsg(*err, "Failed to open service manager");
+        goto out;
+    }
+
+    service = OpenService(manager, QGA_PROVIDER_NAME, access);
+    if (!service) {
+        *err = GetLastError();
+        if (*err != ERROR_SERVICE_DOES_NOT_EXIST) {
+            errmsg(*err, "Failed to open service");
+        }
+    }
+
+out:
+    if (manager) {
+        CloseServiceHandle(manager);
+    }
+    qga_debug_end;
+    return service;
+}
+
+/* Check whether the QGA VSS provider service is registered with the SCM */
+static bool QGAProviderServiceExists(void)
+{
+    qga_debug_begin;
+
+    DWORD err;
+    SC_HANDLE service = QGAProviderOpenService(SERVICE_QUERY_CONFIG, &err);
+
+    if (service) {
+        CloseServiceHandle(service);
+    }
+
+    qga_debug_end;
+    return service != NULL;
+}
+
+/* Delete a QGA VSS provider service that COM+ no longer owns */
+static HRESULT QGAProviderRemoveService(void)
+{
+    qga_debug_begin;
+
+    HRESULT hr = S_OK;
+    DWORD err;
+    SC_HANDLE service = QGAProviderOpenService(DELETE, &err);
+
+    if (!service) {
+        if (err != ERROR_SERVICE_DOES_NOT_EXIST) {
+            hr = HRESULT_FROM_WIN32(err);
+        }
+        goto out;
+    }
+
+    qga_debug("Removing service: %s", QGA_PROVIDER_NAME);
+
+    if (!DeleteService(service)) {
+        err = GetLastError();
+        if (err != ERROR_SERVICE_MARKED_FOR_DELETE) {
+            hr = HRESULT_FROM_WIN32(err);
+            errmsg(err, "Failed to delete service");
+        }
+    }
+
+out:
+    if (service) {
+        CloseServiceHandle(service);
+    }
+    qga_debug_end;
+    return hr;
+}
+
 /* Unregister this module from COM+ Applications Catalog */
 STDAPI COMUnregister(void);
 STDAPI COMUnregister(void)
@@ -251,6 +341,7 @@ STDAPI COMUnregister(void)
 
     DllUnregisterServer();
     chk(QGAProviderFind(QGAProviderRemove, NULL));
+    chk(QGAProviderRemoveService());
 out:
     qga_debug_end;
     return hr;
@@ -286,7 +377,7 @@ STDAPI COMRegister(void)
     }
 
     chk(QGAProviderFind(QGAProviderCount, (void *)&count));
-    if (count) {
+    if (count || QGAProviderServiceExists()) {
         qga_debug("QGA VSS Provider is already installed. Attempting to unregister first.");
         hr = COMUnregister();
         if (FAILED(hr)) {
@@ -499,6 +590,7 @@ STDAPI DllRegisterServer(void)
                                      g_gProviderVersion);
     if (hr == (long int) VSS_E_PROVIDER_ALREADY_REGISTERED) {
         DllUnregisterServer();
+        QGAProviderRemoveService();
         hr = pVssAdmin->RegisterProvider(g_gProviderId, CLSID_QGAVSSProvider,
                                          const_cast<WCHAR * >
                                          (QGA_PROVIDER_LNAME),
@@ -515,6 +607,7 @@ STDAPI DllRegisterServer(void)
 out:
     if (FAILED(hr)) {
         DllUnregisterServer();
+        QGAProviderRemoveService();
     }
 
     qga_debug_end;
@@ -575,29 +668,24 @@ STDAPI StopService(void)
     qga_debug_begin;
 
     HRESULT hr = S_OK;
-    SC_HANDLE manager = OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS);
-    SC_HANDLE service = NULL;
-
-    if (!manager) {
-        errmsg(E_FAIL, "Failed to open service manager");
-        hr = E_FAIL;
-        goto out;
-    }
-    service = OpenService(manager, QGA_PROVIDER_NAME, SC_MANAGER_ALL_ACCESS);
+    DWORD err;
+    SC_HANDLE service = QGAProviderOpenService(SERVICE_STOP, &err);
 
     if (!service) {
-        errmsg(E_FAIL, "Failed to open service");
-        hr =  E_FAIL;
+        hr = HRESULT_FROM_WIN32(err);
         goto out;
     }
-    if (!(ControlService(service, SERVICE_CONTROL_STOP, NULL))) {
-        errmsg(E_FAIL, "Failed to stop service");
-        hr = E_FAIL;
+
+    if (!ControlService(service, SERVICE_CONTROL_STOP, NULL)) {
+        err = GetLastError();
+        hr = HRESULT_FROM_WIN32(err);
+        errmsg(err, "Failed to stop service");
     }
 
 out:
-    CloseServiceHandle(service);
-    CloseServiceHandle(manager);
+    if (service) {
+        CloseServiceHandle(service);
+    }
     qga_debug_end;
     return hr;
 }
-- 
2.55.0

Hi

On Thu, Sep 17, 2026 at 7:49 PM Abed Agbaria <aagbaria@redhat.com> wrote:
>
> COMRegister() probes the COM+ catalog to decide whether to unregister
> first, but it is the SCM that blocks installation when a service of the
> same name is left over.  If the COM+ application is gone and the service
> survives, install fails with 0x80070431 and never recovers.
>
>  - Check the SCM as well as the COM+ catalog in COMRegister().
>  - Delete the provider service in COMUnregister() and at the
>    DllUnregisterServer() call sites in DllRegisterServer().
>
> Opening the SCM, opening the provider service and closing both handles
> is boilerplate that StopService() already contains, so move it into
> QGAProviderOpenService(), which returns a single service handle and
> reports the win32 error through an out parameter, and convert
> StopService() to it.  The service handle stays valid after the SCM
> handle is closed, so the helper closes it and each caller is left with
> one handle to release.
>
> While at it, fix two pre-existing problems in StopService():
>
>  - It logged errmsg(E_FAIL, ...) and returned E_FAIL for every failure,
>    discarding the GetLastError() value.  It now passes the win32 error
>    code to errmsg() and returns HRESULT_FROM_WIN32().
>  - It opened the SCM with SC_MANAGER_ALL_ACCESS, which implies
>    SC_MANAGER_CREATE_SERVICE and is therefore refused to
>    non-administrators, and passed that same SCM mask to OpenService()
>    as a service access mask.  Each operation now requests only what it
>    needs: SC_MANAGER_CONNECT on the SCM, and SERVICE_QUERY_CONFIG,
>    DELETE or SERVICE_STOP on the service.
>
> StopService()'s return value changes from E_FAIL to a specific
> HRESULT_FROM_WIN32(); its only caller, in requester.cpp, ignores it.
>
> Signed-off-by: Abed Agbaria <aagbaria@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

(you could have easily made a small series, I'll let other decide if
it is necessary)

thanks

> ---
>  qga/vss-win32/install.cpp | 122 ++++++++++++++++++++++++++++++++------
>  1 file changed, 105 insertions(+), 17 deletions(-)
>
> v1: https://lore.kernel.org/qemu-devel/20260915145320.215663-1-aagbaria@redhat.com/
>
> v2:
>  - Pass win32 error codes to errmsg() consistently instead of mixing
>    error codes and HRESULTs (Marc-André)
>  - Factor the SCM/OpenService boilerplate into QGAProviderOpenService()
>    and convert the existing StopService() to it (Marc-André)
>  - Restrict StopService() to the access rights it actually needs
>
> Tested on a Windows guest by replacing the installed build with this
> one and running a full install/uninstall cycle, including installation
> over a leftover provider service.
>
> diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
> index 5b7a8e9bc5..cfafc2d95e 100644
> --- a/qga/vss-win32/install.cpp
> +++ b/qga/vss-win32/install.cpp
> @@ -241,6 +241,96 @@ out:
>      return hr;
>  }
>
> +/*
> + * Open the QGA VSS provider service with the requested access rights.
> + *
> + * On success, return a service handle which the caller must release with
> + * CloseServiceHandle().  On failure, return NULL and store the win32 error
> + * code in *err.  ERROR_SERVICE_DOES_NOT_EXIST just means the service is not
> + * installed, which is a normal condition for all callers, so it is not
> + * reported here; the caller decides what it means.
> + */
> +static SC_HANDLE QGAProviderOpenService(DWORD access, DWORD *err)
> +{
> +    qga_debug_begin;
> +
> +    SC_HANDLE service = NULL;
> +    SC_HANDLE manager = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT);
> +
> +    *err = ERROR_SUCCESS;
> +
> +    if (!manager) {
> +        *err = GetLastError();
> +        errmsg(*err, "Failed to open service manager");
> +        goto out;
> +    }
> +
> +    service = OpenService(manager, QGA_PROVIDER_NAME, access);
> +    if (!service) {
> +        *err = GetLastError();
> +        if (*err != ERROR_SERVICE_DOES_NOT_EXIST) {
> +            errmsg(*err, "Failed to open service");
> +        }
> +    }
> +
> +out:
> +    if (manager) {
> +        CloseServiceHandle(manager);
> +    }
> +    qga_debug_end;
> +    return service;
> +}
> +
> +/* Check whether the QGA VSS provider service is registered with the SCM */
> +static bool QGAProviderServiceExists(void)
> +{
> +    qga_debug_begin;
> +
> +    DWORD err;
> +    SC_HANDLE service = QGAProviderOpenService(SERVICE_QUERY_CONFIG, &err);
> +
> +    if (service) {
> +        CloseServiceHandle(service);
> +    }
> +
> +    qga_debug_end;
> +    return service != NULL;
> +}
> +
> +/* Delete a QGA VSS provider service that COM+ no longer owns */
> +static HRESULT QGAProviderRemoveService(void)
> +{
> +    qga_debug_begin;
> +
> +    HRESULT hr = S_OK;
> +    DWORD err;
> +    SC_HANDLE service = QGAProviderOpenService(DELETE, &err);
> +
> +    if (!service) {
> +        if (err != ERROR_SERVICE_DOES_NOT_EXIST) {
> +            hr = HRESULT_FROM_WIN32(err);
> +        }
> +        goto out;
> +    }
> +
> +    qga_debug("Removing service: %s", QGA_PROVIDER_NAME);
> +
> +    if (!DeleteService(service)) {
> +        err = GetLastError();
> +        if (err != ERROR_SERVICE_MARKED_FOR_DELETE) {
> +            hr = HRESULT_FROM_WIN32(err);
> +            errmsg(err, "Failed to delete service");
> +        }
> +    }
> +
> +out:
> +    if (service) {
> +        CloseServiceHandle(service);
> +    }
> +    qga_debug_end;
> +    return hr;
> +}
> +
>  /* Unregister this module from COM+ Applications Catalog */
>  STDAPI COMUnregister(void);
>  STDAPI COMUnregister(void)
> @@ -251,6 +341,7 @@ STDAPI COMUnregister(void)
>
>      DllUnregisterServer();
>      chk(QGAProviderFind(QGAProviderRemove, NULL));
> +    chk(QGAProviderRemoveService());
>  out:
>      qga_debug_end;
>      return hr;
> @@ -286,7 +377,7 @@ STDAPI COMRegister(void)
>      }
>
>      chk(QGAProviderFind(QGAProviderCount, (void *)&count));
> -    if (count) {
> +    if (count || QGAProviderServiceExists()) {
>          qga_debug("QGA VSS Provider is already installed. Attempting to unregister first.");
>          hr = COMUnregister();
>          if (FAILED(hr)) {
> @@ -499,6 +590,7 @@ STDAPI DllRegisterServer(void)
>                                       g_gProviderVersion);
>      if (hr == (long int) VSS_E_PROVIDER_ALREADY_REGISTERED) {
>          DllUnregisterServer();
> +        QGAProviderRemoveService();
>          hr = pVssAdmin->RegisterProvider(g_gProviderId, CLSID_QGAVSSProvider,
>                                           const_cast<WCHAR * >
>                                           (QGA_PROVIDER_LNAME),
> @@ -515,6 +607,7 @@ STDAPI DllRegisterServer(void)
>  out:
>      if (FAILED(hr)) {
>          DllUnregisterServer();
> +        QGAProviderRemoveService();
>      }
>
>      qga_debug_end;
> @@ -575,29 +668,24 @@ STDAPI StopService(void)
>      qga_debug_begin;
>
>      HRESULT hr = S_OK;
> -    SC_HANDLE manager = OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS);
> -    SC_HANDLE service = NULL;
> -
> -    if (!manager) {
> -        errmsg(E_FAIL, "Failed to open service manager");
> -        hr = E_FAIL;
> -        goto out;
> -    }
> -    service = OpenService(manager, QGA_PROVIDER_NAME, SC_MANAGER_ALL_ACCESS);
> +    DWORD err;
> +    SC_HANDLE service = QGAProviderOpenService(SERVICE_STOP, &err);
>
>      if (!service) {
> -        errmsg(E_FAIL, "Failed to open service");
> -        hr =  E_FAIL;
> +        hr = HRESULT_FROM_WIN32(err);
>          goto out;
>      }
> -    if (!(ControlService(service, SERVICE_CONTROL_STOP, NULL))) {
> -        errmsg(E_FAIL, "Failed to stop service");
> -        hr = E_FAIL;
> +
> +    if (!ControlService(service, SERVICE_CONTROL_STOP, NULL)) {
> +        err = GetLastError();
> +        hr = HRESULT_FROM_WIN32(err);
> +        errmsg(err, "Failed to stop service");
>      }
>
>  out:
> -    CloseServiceHandle(service);
> -    CloseServiceHandle(manager);
> +    if (service) {
> +        CloseServiceHandle(service);
> +    }
>      qga_debug_end;
>      return hr;
>  }
> --
> 2.55.0
>


-- 
Marc-André Lureau