[ros-diffs] 01/01: [SERVICES][ADVAPI32] Fix the broken service stop sequence

[Eric Kohl]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=1dfbed9c3dbd135c7bf51…

commit 1dfbed9c3dbd135c7bf51090b9cadde6d2642340
Author:     Eric Kohl <eric.kohl(a)reactos.org>
AuthorDate: Sat Feb 24 11:14:05 2018 +0100
Commit:     Eric Kohl <eric.kohl(a)reactos.org>
CommitDate: Sat Feb 24 11:14:05 2018 +0100

    [SERVICES][ADVAPI32] Fix the broken service stop sequence
    
    services\database.c:
    - Refactor ScmControlService() so that it can be used to send the dispatcher loop stop
command.
    - Separate the code to decrement the image run counter from the service image cleanup
code.
    
    services\rpcserver.c:
    - RSetServiceStatus(): Stop the dispatcher loop when the image run counter is zero and
remove the service image after that.
    
    advapi32\service\sctrl.c:
    - Do not terminate the service dispatcher loop when the last service is being stopped.
Wait for an explicit dispatcher stop command (empty service name).
    
    CORE-12413
---
 base/system/services/database.c    | 105 +++++++++++++++++++------------------
 base/system/services/rpcserver.c   |  28 +++++++++-
 base/system/services/services.h    |   5 +-
 dll/win32/advapi32/service/sctrl.c |  70 +++++++++++++------------
 4 files changed, 121 insertions(+), 87 deletions(-)

diff --git a/base/system/services/database.c b/base/system/services/database.c
index d31d353a87..d28a98c082 100644
--- a/base/system/services/database.c
+++ b/base/system/services/database.c
@@ -432,41 +432,34 @@ done:
 }
 
 
-static VOID
-ScmDereferenceServiceImage(PSERVICE_IMAGE pServiceImage)
+VOID
+ScmRemoveServiceImage(PSERVICE_IMAGE pServiceImage)
 {
-    DPRINT1("ScmDereferenceServiceImage() called\n");
-
-    pServiceImage->dwImageRunCount--;
-
-    if (pServiceImage->dwImageRunCount == 0)
-    {
-        DPRINT1("dwImageRunCount == 0\n");
+    DPRINT1("ScmRemoveServiceImage() called\n");
 
-        /* FIXME: Terminate the process */
+    /* FIXME: Terminate the process */
 
-        /* Remove the service image from the list */
-        RemoveEntryList(&pServiceImage->ImageListEntry);
+    /* Remove the service image from the list */
+    RemoveEntryList(&pServiceImage->ImageListEntry);
 
-        /* Close the process handle */
-        if (pServiceImage->hProcess != INVALID_HANDLE_VALUE)
-            CloseHandle(pServiceImage->hProcess);
+    /* Close the process handle */
+    if (pServiceImage->hProcess != INVALID_HANDLE_VALUE)
+        CloseHandle(pServiceImage->hProcess);
 
-        /* Close the control pipe */
-        if (pServiceImage->hControlPipe != INVALID_HANDLE_VALUE)
-            CloseHandle(pServiceImage->hControlPipe);
+    /* Close the control pipe */
+    if (pServiceImage->hControlPipe != INVALID_HANDLE_VALUE)
+        CloseHandle(pServiceImage->hControlPipe);
 
-        /* Unload the user profile */
-        if (pServiceImage->hProfile != NULL)
-            UnloadUserProfile(pServiceImage->hToken, pServiceImage->hProfile);
+    /* Unload the user profile */
+    if (pServiceImage->hProfile != NULL)
+        UnloadUserProfile(pServiceImage->hToken, pServiceImage->hProfile);
 
-        /* Close the logon token */
-        if (pServiceImage->hToken != NULL)
-            CloseHandle(pServiceImage->hToken);
+    /* Close the logon token */
+    if (pServiceImage->hToken != NULL)
+        CloseHandle(pServiceImage->hToken);
 
-        /* Release the service image */
-        HeapFree(GetProcessHeap(), 0, pServiceImage);
-    }
+    /* Release the service image */
+    HeapFree(GetProcessHeap(), 0, pServiceImage);
 }
 
 
@@ -618,7 +611,15 @@ ScmDeleteServiceRecord(PSERVICE lpService)
 
     /* Dereference the service image */
     if (lpService->lpImage)
-        ScmDereferenceServiceImage(lpService->lpImage);
+    {
+        lpService->lpImage->dwImageRunCount--;
+
+        if (lpService->lpImage->dwImageRunCount == 0)
+        {
+            ScmRemoveServiceImage(lpService->lpImage);
+            lpService->lpImage = NULL;
+        }
+    }
 
     /* Decrement the group reference counter */
     ScmSetServiceGroup(lpService, NULL);
@@ -1095,7 +1096,9 @@ ScmGetBootAndSystemDriverState(VOID)
 
 
 DWORD
-ScmControlService(PSERVICE Service,
+ScmControlService(HANDLE hControlPipe,
+                  PWSTR pServiceName,
+                  SERVICE_STATUS_HANDLE hServiceStatus,
                   DWORD dwControl)
 {
     PSCM_CONTROL_PACKET ControlPacket;
@@ -1116,7 +1119,7 @@ ScmControlService(PSERVICE Service,
 
     /* Calculate the total length of the start command line */
     PacketSize = sizeof(SCM_CONTROL_PACKET);
-    PacketSize += (DWORD)((wcslen(Service->lpServiceName) + 1) * sizeof(WCHAR));
+    PacketSize += (DWORD)((wcslen(pServiceName) + 1) * sizeof(WCHAR));
 
     ControlPacket = HeapAlloc(GetProcessHeap(),
                               HEAP_ZERO_MEMORY,
@@ -1129,17 +1132,17 @@ ScmControlService(PSERVICE Service,
 
     ControlPacket->dwSize = PacketSize;
     ControlPacket->dwControl = dwControl;
-    ControlPacket->hServiceStatus = (SERVICE_STATUS_HANDLE)Service;
+    ControlPacket->hServiceStatus = hServiceStatus;
 
     ControlPacket->dwServiceNameOffset = sizeof(SCM_CONTROL_PACKET);
 
     Ptr = (PWSTR)((PBYTE)ControlPacket + ControlPacket->dwServiceNameOffset);
-    wcscpy(Ptr, Service->lpServiceName);
+    wcscpy(Ptr, pServiceName);
 
     ControlPacket->dwArgumentsCount = 0;
     ControlPacket->dwArgumentsOffset = 0;
 
-    bResult = WriteFile(Service->lpImage->hControlPipe,
+    bResult = WriteFile(hControlPipe,
                         ControlPacket,
                         PacketSize,
                         &dwWriteCount,
@@ -1153,13 +1156,13 @@ ScmControlService(PSERVICE Service,
         {
             DPRINT("dwError: ERROR_IO_PENDING\n");
 
-            dwError = WaitForSingleObject(Service->lpImage->hControlPipe,
+            dwError = WaitForSingleObject(hControlPipe,
                                           PipeTimeout);
             DPRINT("WaitForSingleObject() returned %lu\n", dwError);
 
             if (dwError == WAIT_TIMEOUT)
             {
-                bResult = CancelIo(Service->lpImage->hControlPipe);
+                bResult = CancelIo(hControlPipe);
                 if (bResult == FALSE)
                 {
                     DPRINT1("CancelIo() failed (Error: %lu)\n",
GetLastError());
@@ -1170,7 +1173,7 @@ ScmControlService(PSERVICE Service,
             }
             else if (dwError == WAIT_OBJECT_0)
             {
-                bResult = GetOverlappedResult(Service->lpImage->hControlPipe,
+                bResult = GetOverlappedResult(hControlPipe,
                                               &Overlapped,
                                               &dwWriteCount,
                                               TRUE);
@@ -1193,7 +1196,7 @@ ScmControlService(PSERVICE Service,
     /* Read the reply */
     Overlapped.hEvent = (HANDLE) NULL;
 
-    bResult = ReadFile(Service->lpImage->hControlPipe,
+    bResult = ReadFile(hControlPipe,
                        &ReplyPacket,
                        sizeof(SCM_REPLY_PACKET),
                        &dwReadCount,
@@ -1207,13 +1210,13 @@ ScmControlService(PSERVICE Service,
         {
             DPRINT("dwError: ERROR_IO_PENDING\n");
 
-            dwError = WaitForSingleObject(Service->lpImage->hControlPipe,
+            dwError = WaitForSingleObject(hControlPipe,
                                           PipeTimeout);
             DPRINT("WaitForSingleObject() returned %lu\n", dwError);
 
             if (dwError == WAIT_TIMEOUT)
             {
-                bResult = CancelIo(Service->lpImage->hControlPipe);
+                bResult = CancelIo(hControlPipe);
                 if (bResult == FALSE)
                 {
                     DPRINT1("CancelIo() failed (Error: %lu)\n",
GetLastError());
@@ -1224,7 +1227,7 @@ ScmControlService(PSERVICE Service,
             }
             else if (dwError == WAIT_OBJECT_0)
             {
-                bResult = GetOverlappedResult(Service->lpImage->hControlPipe,
+                bResult = GetOverlappedResult(hControlPipe,
                                               &Overlapped,
                                               &dwReadCount,
                                               TRUE);
@@ -1255,13 +1258,6 @@ Done:
         dwError = ReplyPacket.dwError;
     }
 
-    if (dwError == ERROR_SUCCESS &&
-        dwControl == SERVICE_CONTROL_STOP)
-    {
-        ScmDereferenceServiceImage(Service->lpImage);
-        Service->lpImage = NULL;
-    }
-
     LeaveCriticalSection(&ControlServiceCriticalSection);
 
     DPRINT("ScmControlService() done\n");
@@ -1831,8 +1827,12 @@ ScmLoadService(PSERVICE Service,
             }
             else
             {
-                ScmDereferenceServiceImage(Service->lpImage);
-                Service->lpImage = NULL;
+                Service->lpImage->dwImageRunCount--;
+                if (Service->lpImage->dwImageRunCount == 0)
+                {
+                    ScmRemoveServiceImage(Service->lpImage);
+                    Service->lpImage = NULL;
+                }
             }
         }
     }
@@ -2168,8 +2168,11 @@ ScmAutoShutdownServices(VOID)
             CurrentService->Status.dwCurrentState == SERVICE_START_PENDING)
         {
             /* shutdown service */
-            DPRINT("Shutdown service: %S\n",
CurrentService->szServiceName);
-            ScmControlService(CurrentService, SERVICE_CONTROL_SHUTDOWN);
+            DPRINT("Shutdown service: %S\n",
CurrentService->lpServiceName);
+            ScmControlService(CurrentService->lpImage->hControlPipe,
+                              CurrentService->lpServiceName,
+                              (SERVICE_STATUS_HANDLE)CurrentService,
+                              SERVICE_CONTROL_SHUTDOWN);
         }
 
         ServiceEntry = ServiceEntry->Flink;
diff --git a/base/system/services/rpcserver.c b/base/system/services/rpcserver.c
index a4718aa15f..0a518d06ad 100644
--- a/base/system/services/rpcserver.c
+++ b/base/system/services/rpcserver.c
@@ -1228,7 +1228,9 @@ RControlService(
         }
 
         /* Send control code to the service */
-        dwError = ScmControlService(lpService,
+        dwError = ScmControlService(lpService->lpImage->hControlPipe,
+                                    lpService->lpServiceName,
+                                    (SERVICE_STATUS_HANDLE)lpService,
                                     dwControl);
 
         /* Return service status information */
@@ -1725,6 +1727,28 @@ RSetServiceStatus(
     /* Restore the previous service type */
     lpService->Status.dwServiceType = dwPreviousType;
 
+    /* Handle a stopped service */
+    if ((lpServiceStatus->dwServiceType & SERVICE_WIN32) &&
+        (lpServiceStatus->dwCurrentState == SERVICE_STOPPED))
+    {
+        /* Decrement the image run counter */
+        lpService->lpImage->dwImageRunCount--;
+
+        /* If we just stopped the last running service... */
+        if (lpService->lpImage->dwImageRunCount == 0)
+        {
+            /* Stop the dispatcher thread */
+            ScmControlService(lpService->lpImage->hControlPipe,
+                              L"",
+                              (SERVICE_STATUS_HANDLE)lpService,
+                              SERVICE_CONTROL_STOP);
+
+            /* Remove the service image */
+            ScmRemoveServiceImage(lpService->lpImage);
+            lpService->lpImage = NULL;
+        }
+    }
+
     /* Unlock the service database */
     ScmUnlockDatabase();
 
@@ -1773,6 +1797,8 @@ RSetServiceStatus(
                     lpLogStrings);
     }
 
+
+
     DPRINT("Set %S to %lu\n", lpService->lpDisplayName,
lpService->Status.dwCurrentState);
     DPRINT("RSetServiceStatus() done\n");
 
diff --git a/base/system/services/services.h b/base/system/services/services.h
index 0f03849967..3592ba2560 100644
--- a/base/system/services/services.h
+++ b/base/system/services/services.h
@@ -164,6 +164,7 @@ DWORD ScmStartService(PSERVICE Service,
                       DWORD argc,
                       LPWSTR *argv);
 
+VOID ScmRemoveServiceImage(PSERVICE_IMAGE pServiceImage);
 PSERVICE ScmGetServiceEntryByName(LPCWSTR lpServiceName);
 PSERVICE ScmGetServiceEntryByDisplayName(LPCWSTR lpDisplayName);
 PSERVICE ScmGetServiceEntryByResumeCount(DWORD dwResumeCount);
@@ -174,7 +175,9 @@ DWORD ScmCreateNewServiceRecord(LPCWSTR lpServiceName,
 VOID ScmDeleteServiceRecord(PSERVICE lpService);
 DWORD ScmMarkServiceForDelete(PSERVICE pService);
 
-DWORD ScmControlService(PSERVICE Service,
+DWORD ScmControlService(HANDLE hControlPipe,
+                        PWSTR pServiceName,
+                        SERVICE_STATUS_HANDLE hServiceStatus,
                         DWORD dwControl);
 
 BOOL ScmLockDatabaseExclusive(VOID);
diff --git a/dll/win32/advapi32/service/sctrl.c b/dll/win32/advapi32/service/sctrl.c
index 4a0ae5665d..fe2469732f 100644
--- a/dll/win32/advapi32/service/sctrl.c
+++ b/dll/win32/advapi32/service/sctrl.c
@@ -544,7 +544,7 @@ ScServiceDispatcher(HANDLE hPipe,
 {
     DWORD Count;
     BOOL bResult;
-    DWORD dwRunningServices = 0;
+    BOOL bRunning = TRUE;
     LPWSTR lpServiceName;
     PACTIVE_SERVICE lpService;
     SCM_REPLY_PACKET ReplyPacket;
@@ -555,7 +555,7 @@ ScServiceDispatcher(HANDLE hPipe,
     if (ControlPacket == NULL || dwBufferSize < sizeof(SCM_CONTROL_PACKET))
         return FALSE;
 
-    while (TRUE)
+    while (bRunning)
     {
         /* Read command from the control pipe */
         bResult = ReadFile(hPipe,
@@ -572,39 +572,44 @@ ScServiceDispatcher(HANDLE hPipe,
         lpServiceName = (LPWSTR)((PBYTE)ControlPacket +
ControlPacket->dwServiceNameOffset);
         TRACE("Service: %S\n", lpServiceName);
 
-        if (ControlPacket->dwControl == SERVICE_CONTROL_START_OWN)
-            lpActiveServices[0].bOwnProcess = TRUE;
-
-        lpService = ScLookupServiceByServiceName(lpServiceName);
-        if (lpService != NULL)
+        if (lpServiceName[0] == UNICODE_NULL)
         {
-            /* Execute command */
-            switch (ControlPacket->dwControl)
-            {
-                case SERVICE_CONTROL_START_SHARE:
-                case SERVICE_CONTROL_START_OWN:
-                    TRACE("Start command - received SERVICE_CONTROL_START\n");
-                    dwError = ScStartService(lpService, ControlPacket);
-                    if (dwError == ERROR_SUCCESS)
-                        dwRunningServices++;
-                    break;
-
-                case SERVICE_CONTROL_STOP:
-                    TRACE("Stop command - received SERVICE_CONTROL_STOP\n");
-                    dwError = ScControlService(lpService, ControlPacket);
-                    if (dwError == ERROR_SUCCESS)
-                        dwRunningServices--;
-                    break;
-
-                default:
-                    TRACE("Command %lu received",
ControlPacket->dwControl);
-                    dwError = ScControlService(lpService, ControlPacket);
-                    break;
-            }
+            ERR("Stop dispatcher thread\n");
+            bRunning = FALSE;
+            dwError = ERROR_SUCCESS;
         }
         else
         {
-            dwError = ERROR_SERVICE_DOES_NOT_EXIST;
+            if (ControlPacket->dwControl == SERVICE_CONTROL_START_OWN)
+                lpActiveServices[0].bOwnProcess = TRUE;
+
+            lpService = ScLookupServiceByServiceName(lpServiceName);
+            if (lpService != NULL)
+            {
+                /* Execute command */
+                switch (ControlPacket->dwControl)
+                {
+                    case SERVICE_CONTROL_START_SHARE:
+                    case SERVICE_CONTROL_START_OWN:
+                        TRACE("Start command - received
SERVICE_CONTROL_START\n");
+                        dwError = ScStartService(lpService, ControlPacket);
+                        break;
+
+                    case SERVICE_CONTROL_STOP:
+                        TRACE("Stop command - received
SERVICE_CONTROL_STOP\n");
+                        dwError = ScControlService(lpService, ControlPacket);
+                        break;
+
+                    default:
+                        TRACE("Command %lu received",
ControlPacket->dwControl);
+                        dwError = ScControlService(lpService, ControlPacket);
+                        break;
+                }
+            }
+            else
+            {
+                dwError = ERROR_SERVICE_DOES_NOT_EXIST;
+            }
         }
 
         ReplyPacket.dwError = dwError;
@@ -620,9 +625,6 @@ ScServiceDispatcher(HANDLE hPipe,
             ERR("Pipe write failed (Error: %lu)\n", GetLastError());
             return FALSE;
         }
-
-        if (dwRunningServices == 0)
-            break;
     }
 
     return TRUE;