Reviewed-by: Kostiantyn Kostiuk <[email protected]> On Fri, Sep 18, 2026 at 9:57 AM Marc-André Lureau < [email protected]> wrote:
> Hi > > On Thu, Sep 17, 2026 at 7:49 PM Abed Agbaria <[email protected]> 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 <[email protected]> > > Reviewed-by: Marc-André Lureau <[email protected]> > > (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/[email protected]/ > > > > 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 > >
