1. Patchew
  2. QEMU
  3. View series

[PATCH] qga/vss-win32/install.cpp: remove orphaned VSS provider service

Abed Agbaria posted 1 patch 1 week, 4 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260915145320.215663-1-aagbaria@redhat.com
Maintainers: Kostiantyn Kostiuk <kkostiuk@redhat.com>, Michael Roth <michael.roth@amd.com>
qga/vss-win32/install.cpp | 88 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 87 insertions(+), 1 deletion(-)
[PATCH] qga/vss-win32/install.cpp: remove orphaned VSS provider service
Posted by Abed Agbaria 1 week, 4 days ago 
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().

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

diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index 5b7a8e9bc5..0e4f2ac009 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -241,6 +241,89 @@ out:
     return hr;
 }
 
+/* Check whether the QGA VSS provider service is registered with the SCM.*/
+static bool QGAProviderServiceExists(void)
+{
+    qga_debug_begin;
+
+    bool exists = false;
+    DWORD err;
+    SC_HANDLE manager = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT);
+    SC_HANDLE service = NULL;
+
+    if (!manager) {
+        errmsg(GetLastError(), "Failed to open service manager");
+        goto out;
+    }
+
+    service = OpenService(manager, QGA_PROVIDER_NAME, SERVICE_QUERY_CONFIG);
+    if (service) {
+        exists = true;
+    } else {
+        err = GetLastError();
+        if (err != ERROR_SERVICE_DOES_NOT_EXIST) {
+            errmsg(err, "Failed to open service");
+        }
+    }
+
+out:
+    if (service) {
+        CloseServiceHandle(service);
+    }
+    if (manager) {
+        CloseServiceHandle(manager);
+    }
+    qga_debug_end;
+    return exists;
+}
+
+/* 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 manager = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT);
+    SC_HANDLE service = NULL;
+
+    if (!manager) {
+        hr = HRESULT_FROM_WIN32(GetLastError());
+        errmsg(hr, "Failed to open service manager");
+        goto out;
+    }
+
+    service = OpenService(manager, QGA_PROVIDER_NAME, DELETE);
+    if (!service) {
+        err = GetLastError();
+        if (err != ERROR_SERVICE_DOES_NOT_EXIST) {
+            hr = HRESULT_FROM_WIN32(err);
+            errmsg(hr, "Failed to open service");
+        }
+        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(hr, "Failed to delete service");
+        }
+    }
+
+out:
+    if (service) {
+        CloseServiceHandle(service);
+    }
+    if (manager) {
+        CloseServiceHandle(manager);
+    }
+    qga_debug_end;
+    return hr;
+}
+
 /* Unregister this module from COM+ Applications Catalog */
 STDAPI COMUnregister(void);
 STDAPI COMUnregister(void)
@@ -251,6 +334,7 @@ STDAPI COMUnregister(void)
 
     DllUnregisterServer();
     chk(QGAProviderFind(QGAProviderRemove, NULL));
+    chk(QGAProviderRemoveService());
 out:
     qga_debug_end;
     return hr;
@@ -286,7 +370,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 +583,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 +600,7 @@ STDAPI DllRegisterServer(void)
 out:
     if (FAILED(hr)) {
         DllUnregisterServer();
+        QGAProviderRemoveService();
     }
 
     qga_debug_end;
-- 
2.55.0
Re: [PATCH] qga/vss-win32/install.cpp: remove orphaned VSS provider service
Posted by Marc-André Lureau 1 week, 3 days ago 
Hi

On Tue, Sep 15, 2026 at 7:03 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().
>
> Signed-off-by: Abed Agbaria <aagbaria@redhat.com>
> ---
>  qga/vss-win32/install.cpp | 88 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 1 deletion(-)
>
> diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
> index 5b7a8e9bc5..0e4f2ac009 100644
> --- a/qga/vss-win32/install.cpp
> +++ b/qga/vss-win32/install.cpp
> @@ -241,6 +241,89 @@ out:
>      return hr;
>  }
>
> +/* Check whether the QGA VSS provider service is registered with the SCM.*/
> +static bool QGAProviderServiceExists(void)
> +{
> +    qga_debug_begin;
> +
> +    bool exists = false;
> +    DWORD err;
> +    SC_HANDLE manager = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT);
> +    SC_HANDLE service = NULL;
> +
> +    if (!manager) {
> +        errmsg(GetLastError(), "Failed to open service manager");
> +        goto out;
> +    }
> +
> +    service = OpenService(manager, QGA_PROVIDER_NAME, SERVICE_QUERY_CONFIG);
> +    if (service) {
> +        exists = true;
> +    } else {
> +        err = GetLastError();
> +        if (err != ERROR_SERVICE_DOES_NOT_EXIST) {
> +            errmsg(err, "Failed to open service");
> +        }
> +    }
> +
> +out:
> +    if (service) {
> +        CloseServiceHandle(service);
> +    }
> +    if (manager) {
> +        CloseServiceHandle(manager);
> +    }
> +    qga_debug_end;
> +    return exists;
> +}
> +
> +/* 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 manager = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT);
> +    SC_HANDLE service = NULL;
> +
> +    if (!manager) {
> +        hr = HRESULT_FROM_WIN32(GetLastError());
> +        errmsg(hr, "Failed to open service manager");

errmsg is called with win32 error code and HRESULT. This is already
inconsistent. Imho it should be fixed first before introducing more
inconsistency.



> +        goto out;
> +    }
> +
> +    service = OpenService(manager, QGA_PROVIDER_NAME, DELETE);

you are adding 2 more places to open a service, when there is already
similar code in  StopService(). I think it could be factored out.

> +    if (!service) {
> +        err = GetLastError();
> +        if (err != ERROR_SERVICE_DOES_NOT_EXIST) {
> +            hr = HRESULT_FROM_WIN32(err);
> +            errmsg(hr, "Failed to open service");
> +        }
> +        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(hr, "Failed to delete service");
> +        }
> +    }
> +
> +out:
> +    if (service) {
> +        CloseServiceHandle(service);
> +    }
> +    if (manager) {
> +        CloseServiceHandle(manager);
> +    }
> +    qga_debug_end;
> +    return hr;
> +}
> +
>  /* Unregister this module from COM+ Applications Catalog */
>  STDAPI COMUnregister(void);
>  STDAPI COMUnregister(void)
> @@ -251,6 +334,7 @@ STDAPI COMUnregister(void)
>
>      DllUnregisterServer();
>      chk(QGAProviderFind(QGAProviderRemove, NULL));
> +    chk(QGAProviderRemoveService());
>  out:
>      qga_debug_end;
>      return hr;
> @@ -286,7 +370,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 +583,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 +600,7 @@ STDAPI DllRegisterServer(void)
>  out:
>      if (FAILED(hr)) {
>          DllUnregisterServer();
> +        QGAProviderRemoveService();
>      }
>
>      qga_debug_end;
> --
> 2.55.0
>
>


-- 
Marc-André Lureau
Re: [PATCH] qga/vss-win32/install.cpp: remove orphaned VSS provider service
Posted by Abed Agbaria 1 week, 2 days ago 
Hi Marc-André,

Thanks for the review.

v2: https://lore.kernel.org/qemu-devel/20260917154901.321702-1-aagbaria@redhat.com/

On Wed, Sep 16, 2026 at 11:29 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Tue, Sep 15, 2026 at 7:03 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().
> >
> > Signed-off-by: Abed Agbaria <aagbaria@redhat.com>
> > ---
> >  qga/vss-win32/install.cpp | 88 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 87 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
> > index 5b7a8e9bc5..0e4f2ac009 100644
> > --- a/qga/vss-win32/install.cpp
> > +++ b/qga/vss-win32/install.cpp
> > @@ -241,6 +241,89 @@ out:
> >      return hr;
> >  }
> >
> > +/* Check whether the QGA VSS provider service is registered with the SCM.*/
> > +static bool QGAProviderServiceExists(void)
> > +{
> > +    qga_debug_begin;
> > +
> > +    bool exists = false;
> > +    DWORD err;
> > +    SC_HANDLE manager = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT);
> > +    SC_HANDLE service = NULL;
> > +
> > +    if (!manager) {
> > +        errmsg(GetLastError(), "Failed to open service manager");
> > +        goto out;
> > +    }
> > +
> > +    service = OpenService(manager, QGA_PROVIDER_NAME, SERVICE_QUERY_CONFIG);
> > +    if (service) {
> > +        exists = true;
> > +    } else {
> > +        err = GetLastError();
> > +        if (err != ERROR_SERVICE_DOES_NOT_EXIST) {
> > +            errmsg(err, "Failed to open service");
> > +        }
> > +    }
> > +
> > +out:
> > +    if (service) {
> > +        CloseServiceHandle(service);
> > +    }
> > +    if (manager) {
> > +        CloseServiceHandle(manager);
> > +    }
> > +    qga_debug_end;
> > +    return exists;
> > +}
> > +
> > +/* 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 manager = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT);
> > +    SC_HANDLE service = NULL;
> > +
> > +    if (!manager) {
> > +        hr = HRESULT_FROM_WIN32(GetLastError());
> > +        errmsg(hr, "Failed to open service manager");
>
> errmsg is called with win32 error code and HRESULT. This is already
> inconsistent. Imho it should be fixed first before introducing more
> inconsistency.

Fixed in v2: every win32 path now passes the GetLastError() code to
errmsg(). StopService() was the remaining offender - it passed E_FAIL
three times and discarded the real error; it now logs the win32 code
and returns HRESULT_FROM_WIN32(). errmsg() still receives HRESULTs from
the COM paths via chk(), which I left alone.
>
>
>
> > +        goto out;
> > +    }
> > +
> > +    service = OpenService(manager, QGA_PROVIDER_NAME, DELETE);
>
> you are adding 2 more places to open a service, when there is already
> similar code in  StopService(). I think it could be factored out.

Factored into QGAProviderOpenService(access, &err), and StopService()
is converted to it - v2 removes more of this boilerplate than it adds.
I also replaced SC_MANAGER_ALL_ACCESS there with the minimal rights:
SC_MANAGER_CONNECT plus SERVICE_QUERY_CONFIG / DELETE / SERVICE_STOP.

>
>
> > +    if (!service) {
> > +        err = GetLastError();
> > +        if (err != ERROR_SERVICE_DOES_NOT_EXIST) {
> > +            hr = HRESULT_FROM_WIN32(err);
> > +            errmsg(hr, "Failed to open service");
> > +        }
> > +        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(hr, "Failed to delete service");
> > +        }
> > +    }
> > +
> > +out:
> > +    if (service) {
> > +        CloseServiceHandle(service);
> > +    }
> > +    if (manager) {
> > +        CloseServiceHandle(manager);
> > +    }
> > +    qga_debug_end;
> > +    return hr;
> > +}
> > +
> >  /* Unregister this module from COM+ Applications Catalog */
> >  STDAPI COMUnregister(void);
> >  STDAPI COMUnregister(void)
> > @@ -251,6 +334,7 @@ STDAPI COMUnregister(void)
> >
> >      DllUnregisterServer();
> >      chk(QGAProviderFind(QGAProviderRemove, NULL));
> > +    chk(QGAProviderRemoveService());
> >  out:
> >      qga_debug_end;
> >      return hr;
> > @@ -286,7 +370,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 +583,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 +600,7 @@ STDAPI DllRegisterServer(void)
> >  out:
> >      if (FAILED(hr)) {
> >          DllUnregisterServer();
> > +        QGAProviderRemoveService();
> >      }
> >
> >      qga_debug_end;
> > --
> > 2.55.0
> >
> >
>
>
> --
> Marc-André Lureau
>


Abed Agbaria

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.