diff --git a/base/system/services/database.c b/base/system/services/database.c index fa4d831b78eaa..9f0cdd95793ac 100644 --- a/base/system/services/database.c +++ b/base/system/services/database.c @@ -516,7 +516,7 @@ ScmCreateOrReferenceServiceImage(PSERVICE pService) pServiceImage->dwImageRunCount = 1; pServiceImage->hControlPipe = INVALID_HANDLE_VALUE; - pServiceImage->hProcess = INVALID_HANDLE_VALUE; + pServiceImage->hProcess = NULL; pString = (PWSTR)((INT_PTR)pServiceImage + sizeof(SERVICE_IMAGE)); @@ -617,60 +617,82 @@ ScmCreateOrReferenceServiceImage(PSERVICE pService) return dwError; } +/*static*/ VOID +ScmDereferenceOrTerminateServiceImage(PSERVICE pService) +{ + ASSERT(pService->lpImage); + ASSERT(pService->lpImage->dwImageRunCount > 0); + + /* Decrement the run counter */ + pService->lpImage->dwImageRunCount--; + + /* If we just stopped the last running service... */ + if (pService->lpImage->dwImageRunCount == 0) + { + /* Remove the service image */ + DPRINT("Removing service image %S for %S\n", + pService->lpImage->pszImagePath, pService->lpServiceName); + ScmRemoveServiceImage(pService->lpImage); + pService->lpImage = NULL; + } +} + VOID ScmRemoveServiceImage(PSERVICE_IMAGE pServiceImage) { DWORD dwError; - DPRINT1("ScmRemoveServiceImage() called\n"); + DPRINT1("ScmRemoveServiceImage(%S) called\n", pServiceImage->pszImagePath); + + ASSERT(pServiceImage->dwImageRunCount == 0); /* - * No services are running in this image anymore. - * Tell the service dispatcher to exit now, and wait - * for the process to cleanly terminate. + * No services are running in this image anymore. Notify the service + * dispatcher to exit now, and wait for the process to cleanly terminate. + * Note that the timeout used there (20 seconds) corresponds to the default + * value of the "WaitToKillServiceTimeout" registry value. */ -// FIXME: Re-enable here and remove the L"" SERVICE_CONTROL_STOP call in rpcserver.c -#if 0 + DPRINT("Stopping the dispatcher thread of service image %S\n", pServiceImage->pszImagePath); dwError = ScmControlService(pServiceImage->hControlPipe, L"", SERVICE_CONTROL_STOP, // Windows: use SERVICE_STOP NULL); if (dwError == ERROR_SUCCESS) -#endif { dwError = WaitForSingleObject(pServiceImage->hProcess, 20000); if (dwError != WAIT_OBJECT_0) DPRINT1("WaitForSingleObject failed, going to kill the process.\n"); } -#if 0 else { DPRINT1("ScmControlService failed, going to kill the process.\n"); } -#endif if (dwError != ERROR_SUCCESS || dwError != WAIT_OBJECT_0) { /* If the control or the wait failed, kill the host process - * (asynchronously), unless it is ourselves (case of services.exe - * hosting services) */ + * (asynchronously), unless it is ourselves (services.exe + * itself hosting services) */ if (pServiceImage->hProcess != GetCurrentProcess()) { TerminateProcess(pServiceImage->hProcess, 0); /* Be sure the process really terminates */ dwError = WaitForSingleObject(pServiceImage->hProcess, 20000); + if (dwError != WAIT_OBJECT_0) + DPRINT1("WaitForSingleObject failed, the process cannot be killed.\n"); } - if (dwError != WAIT_OBJECT_0) - DPRINT1("WaitForSingleObject failed, the process cannot be killed.\n"); + /* If the process cannot be killed, it may fail using the user profile + * since it gets unloaded below. In addition, the process may be left + * in a zombie state on the system. */ } /* Remove the service image from the list */ RemoveEntryList(&pServiceImage->ImageListEntry); /* Close the process handle */ - if (pServiceImage->hProcess != INVALID_HANDLE_VALUE) + if (pServiceImage->hProcess) CloseHandle(pServiceImage->hProcess); /* Close the control pipe */ @@ -678,7 +700,7 @@ ScmRemoveServiceImage(PSERVICE_IMAGE pServiceImage) CloseHandle(pServiceImage->hControlPipe); /* Unload the user profile */ - if (pServiceImage->hProfile != NULL) + if (pServiceImage->hProfile) { ScmEnableBackupRestorePrivileges(pServiceImage->hToken, TRUE); UnloadUserProfile(pServiceImage->hToken, pServiceImage->hProfile); @@ -686,7 +708,7 @@ ScmRemoveServiceImage(PSERVICE_IMAGE pServiceImage) } /* Close the logon token */ - if (pServiceImage->hToken != NULL) + if (pServiceImage->hToken) CloseHandle(pServiceImage->hToken); /* Release the service image */ @@ -865,15 +887,7 @@ ScmDeleteServiceRecord(PSERVICE lpService) /* Dereference the service image */ if (lpService->lpImage) - { - lpService->lpImage->dwImageRunCount--; - - if (lpService->lpImage->dwImageRunCount == 0) - { - ScmRemoveServiceImage(lpService->lpImage); - lpService->lpImage = NULL; - } - } + ScmDereferenceOrTerminateServiceImage(lpService); /* Decrement the group reference counter */ ScmSetServiceGroup(lpService, NULL); @@ -974,8 +988,8 @@ ScmReferenceService(PSERVICE lpService) return InterlockedIncrement(&lpService->RefCount); } -/* This function must be called with the database lock held exclusively as - it can end up deleting the service */ +/* This function must be called with the database lock held exclusively + * as it can end up deleting the service */ DWORD ScmDereferenceService(PSERVICE lpService) { @@ -1989,12 +2003,7 @@ ScmLoadService(PSERVICE Service, } else { - Service->lpImage->dwImageRunCount--; - if (Service->lpImage->dwImageRunCount == 0) - { - ScmRemoveServiceImage(Service->lpImage); - Service->lpImage = NULL; - } + ScmDereferenceOrTerminateServiceImage(Service); } } } diff --git a/base/system/services/rpcserver.c b/base/system/services/rpcserver.c index 07ddd1b520a03..a0d74ad455f2e 100644 --- a/base/system/services/rpcserver.c +++ b/base/system/services/rpcserver.c @@ -1608,6 +1608,9 @@ ScmIsValidServiceState(DWORD dwCurrentState) } } +/*static*/ VOID +ScmDereferenceOrTerminateServiceImage(PSERVICE pService); + static DWORD WINAPI @@ -1621,13 +1624,9 @@ ScmStopThread(PVOID pParam) if (lpService->lpImage->dwImageRunCount == 1) { /* Stop the dispatcher thread. - * We must not send a control message while holding the database lock, otherwise it can cause timeouts + * We must not send a control message while holding the database lock, otherwise it can cause timeouts. * We are sure that the service won't be deleted in the meantime because we still have a reference to it. */ - DPRINT("Stopping the dispatcher thread for service %S\n", lpService->lpServiceName); - ScmControlService(lpService->lpImage->hControlPipe, - L"", - SERVICE_CONTROL_STOP, - (SERVICE_STATUS_HANDLE)lpService); + /// ScmRemoveServiceImage(lpService->lpImage); /// FIXME: See below. } /* Lock the service database exclusively */ @@ -1635,17 +1634,8 @@ ScmStopThread(PVOID pParam) DPRINT("Service %S image count:%d\n", lpService->lpServiceName, lpService->lpImage->dwImageRunCount); - /* Decrement the image run counter */ - lpService->lpImage->dwImageRunCount--; - - /* If we just stopped the last running service... */ - if (lpService->lpImage->dwImageRunCount == 0) - { - /* Remove the service image */ - DPRINT("Removing service image for %S\n", lpService->lpServiceName); - ScmRemoveServiceImage(lpService->lpImage); - lpService->lpImage = NULL; - } + /* Dereference the service image */ + ScmDereferenceOrTerminateServiceImage(lpService); /* Report the results of the status change here */ if (lpService->Status.dwWin32ExitCode != ERROR_SUCCESS) @@ -1675,7 +1665,7 @@ ScmStopThread(PVOID pParam) } /* Remove the reference that was added when the service started */ - DPRINT("Service %S has %d references while stoping\n", lpService->lpServiceName, lpService->RefCount); + DPRINT("Service %S has %d references while stopping\n", lpService->lpServiceName, lpService->RefCount); ScmDereferenceService(lpService); /* Unlock the service database */ @@ -1781,13 +1771,15 @@ RSetServiceStatus( { HANDLE hStopThread; DWORD dwStopThreadId; - DPRINT("Service %S, currentstate: %d, prev: %d\n", lpService->lpServiceName, lpServiceStatus->dwCurrentState, dwPreviousState); + DPRINT("Service %S, CurrentState: %d, Prev: %d\n", lpService->lpServiceName, lpServiceStatus->dwCurrentState, dwPreviousState); /* * The service just changed its status to stopped. - * Create a thread that will complete the stop sequence. - * This thread will remove the reference that was added when the service started. - * This will ensure that the service will remain valid as long as this reference is still held. + * Create a thread that will complete the stop sequence asynchronously; + * this will give the service a chance to finish its cleanup. + * This thread will remove the reference that was added when the service + * started. This will ensure that the service will remain valid as long + * as this reference is still held. */ hStopThread = CreateThread(NULL, 0, @@ -1795,7 +1787,7 @@ RSetServiceStatus( (LPVOID)lpService, 0, &dwStopThreadId); - if (hStopThread == NULL) + if (!hStopThread) { DPRINT1("Failed to create thread to complete service stop\n"); /* We can't leave without releasing the reference.