[SVCHOST]

- Fix various incorrect buffer size calculations
- Don't expand environment variables in REG_SZ values
- Repair all DPRINTs
- Other misc fixes
CORE-7296 #resolve

svn path=/trunk/; revision=59360

reactos/base/services/svchost/svchost.c

@@ -19,7 +19,6 @@ static PCWSTR SVCHOST_REG_KEY   = L"SOFTWARE\\Microsoft\\Windows NT\\CurrentVers
 static PCWSTR SERVICE_KEY       = L"SYSTEM\\CurrentControlSet\\Services\\";
 static PCWSTR PARAMETERS_KEY    = L"\\Parameters";
 
-#define SERVICE_KEY_LENGTH  wcslen(SERVICE_KEY);
 #define REG_MAX_DATA_SIZE   2048
 
 static PSERVICE FirstService = NULL;
@@ -29,104 +28,143 @@ static PSERVICE FirstService = NULL;
 BOOL PrepareService(PCWSTR ServiceName)
 {
     HKEY hServiceKey;
-    WCHAR ServiceKeyBuffer[MAX_PATH + 1];
+    WCHAR ServiceKeyBuffer[512];
-    DWORD LeftOfBuffer = sizeof(ServiceKeyBuffer) / sizeof(ServiceKeyBuffer[0]);
+    HRESULT Result;
-    DWORD KeyType;
+    DWORD ValueType;
-    PWSTR Buffer = NULL;
+    PWSTR Buffer;
-    DWORD BufferSize = MAX_PATH + 1;
+    DWORD BufferSize;
     LONG RetVal;
     HINSTANCE hServiceDll;
-    WCHAR DllPath[MAX_PATH + 2]; /* See MSDN on ExpandEnvironmentStrings() for ANSI strings for more details on + 2 */
+    PWSTR DllPath;
+    DWORD DllPathSize;
     LPSERVICE_MAIN_FUNCTION ServiceMainFunc;
     PSERVICE Service;
+    DWORD ServiceNameSize;
 
     /* Compose the registry path to the service's "Parameter" key */
-    wcsncpy(ServiceKeyBuffer, SERVICE_KEY, LeftOfBuffer);
+    Result = StringCbCopy(ServiceKeyBuffer, sizeof(ServiceKeyBuffer), SERVICE_KEY);
-    LeftOfBuffer -= wcslen(SERVICE_KEY);
+    if (FAILED(Result))
-    wcsncat(ServiceKeyBuffer, ServiceName, LeftOfBuffer);
-    LeftOfBuffer -= wcslen(ServiceName);
-    wcsncat(ServiceKeyBuffer, PARAMETERS_KEY, LeftOfBuffer);
-    LeftOfBuffer -= wcslen(PARAMETERS_KEY);
-
-    if (LeftOfBuffer < 0)
     {
-        DPRINT1("Buffer overflow for service name: '%s'\n", ServiceName);
+        DPRINT1("Buffer overflow for service name: '%ls'\n", ServiceName);
+        return FALSE;
+    }
+    Result = StringCbCat(ServiceKeyBuffer, sizeof(ServiceKeyBuffer), ServiceName);
+    if (FAILED(Result))
+    {
+        DPRINT1("Buffer overflow for service name: '%ls'\n", ServiceName);
+        return FALSE;
+    }
+    Result = StringCbCat(ServiceKeyBuffer, sizeof(ServiceKeyBuffer), PARAMETERS_KEY);
+    if (FAILED(Result))
+    {
+        DPRINT1("Buffer overflow for service name: '%ls'\n", ServiceName);
         return FALSE;
     }
 
     /* Open the service registry key to find the dll name */
-    if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, ServiceKeyBuffer, 0, KEY_READ, &hServiceKey) != ERROR_SUCCESS)
+    RetVal = RegOpenKeyEx(HKEY_LOCAL_MACHINE, ServiceKeyBuffer, 0, KEY_READ, &hServiceKey);
+    if (RetVal != ERROR_SUCCESS)
     {
-        DPRINT1("Could not open service key (%s)\n", ServiceKeyBuffer);
+        DPRINT1("Could not open service key (%ls)\n", ServiceKeyBuffer);
         return FALSE;
     }
 
-    do
+    BufferSize = 0;
+    RetVal = RegQueryValueEx(hServiceKey, L"ServiceDll", NULL, &ValueType, NULL, &BufferSize);
+    if (RetVal != ERROR_SUCCESS)
     {
-        if (Buffer)
+        RegCloseKey(hServiceKey);
-            HeapFree(GetProcessHeap(), 0, Buffer);
+        return FALSE;
+    }
 
-        Buffer = HeapAlloc(GetProcessHeap(), 0, BufferSize);
+    Buffer = HeapAlloc(GetProcessHeap(), 0, BufferSize);
-        if (Buffer == NULL)
+    if (Buffer == NULL)
+    {
+        DPRINT1("Not enough memory for service: %ls\n", ServiceName);
+        RegCloseKey(hServiceKey);
+        return FALSE;
+    }
+
+    RetVal = RegQueryValueEx(hServiceKey, L"ServiceDll", NULL, &ValueType, (LPBYTE)Buffer, &BufferSize);
+    if (RetVal != ERROR_SUCCESS || BufferSize == 0)
+    {
+        DPRINT1("Could not read 'ServiceDll' value from service: %ls, ErrorCode: 0x%lx\n", ServiceName, RetVal);
+        RegCloseKey(hServiceKey);
+        HeapFree(GetProcessHeap(), 0, Buffer);
+        return FALSE;
+    }
+    RegCloseKey(hServiceKey);
+
+    if (ValueType == REG_EXPAND_SZ)
+    {
+        /* Convert possible %SystemRoot% to a real path */
+        DllPathSize = ExpandEnvironmentStrings(Buffer, NULL, 0);
+        if (DllPathSize == 0)
         {
-            DPRINT1("Not enough memory for service: %s\n", ServiceName);
+            DPRINT1("Invalid ServiceDll path: %ls, ErrorCode: %lu\n", Buffer, GetLastError());
+            HeapFree(GetProcessHeap(), 0, Buffer);
+            return FALSE;
+        }
+        DllPath = HeapAlloc(GetProcessHeap(), 0, DllPathSize * sizeof(WCHAR));
+        if (DllPath == NULL)
+        {
+            DPRINT1("Not enough memory for service: %ls\n", ServiceName);
+            HeapFree(GetProcessHeap(), 0, Buffer);
+            return FALSE;
+        }
+        if (ExpandEnvironmentStrings(Buffer, DllPath, DllPathSize) != DllPathSize)
+        {
+            DPRINT1("Invalid ServiceDll path: %ls, ErrorCode: %lu\n", Buffer, GetLastError());
+            HeapFree(GetProcessHeap(), 0, DllPath);
+            HeapFree(GetProcessHeap(), 0, Buffer);
             return FALSE;
         }
 
-        RetVal = RegQueryValueEx(hServiceKey, L"ServiceDll", NULL, &KeyType, (LPBYTE)Buffer, &BufferSize);
+        HeapFree(GetProcessHeap(), 0, Buffer);
-
+    }
-    } while (RetVal == ERROR_MORE_DATA);
+    else if (ValueType == REG_SZ)
-
-
-    RegCloseKey(hServiceKey);
-
-    if (RetVal != ERROR_SUCCESS || BufferSize == 0)
     {
-        DPRINT1("Could not read 'ServiceDll' value from service: %s, ErrorCode: 0x%x\n", ServiceName, RetVal);
+        DllPath = Buffer;
+    }
+    else
+    {
+        DPRINT1("Invalid ServiceDll value type %lu for service: %ls, ErrorCode: %lu\n", ValueType, ServiceName, GetLastError());
         HeapFree(GetProcessHeap(), 0, Buffer);
         return FALSE;
     }
 
-    /* Convert possible %SystemRoot% to a real path */
-    BufferSize = ExpandEnvironmentStrings(Buffer, DllPath, _countof(DllPath));
-    if (BufferSize == 0)
-    {
-        DPRINT1("Invalid ServiceDll path: %s\n", Buffer);
-        HeapFree(GetProcessHeap(), 0, Buffer);
-        return FALSE;
-    }
-
-    HeapFree(GetProcessHeap(), 0, Buffer);
-
     /* Load the service dll */
     DPRINT("Trying to load dll\n");
     hServiceDll = LoadLibrary(DllPath);
-
     if (hServiceDll == NULL)
     {
-        DPRINT1("Unable to load ServiceDll: %s, ErrorCode: %u\n", DllPath, GetLastError());
+        DPRINT1("Unable to load ServiceDll: %ls, ErrorCode: %lu\n", DllPath, GetLastError());
+        HeapFree(GetProcessHeap(), 0, DllPath);
         return FALSE;
     }
+    HeapFree(GetProcessHeap(), 0, DllPath);
 
     ServiceMainFunc = (LPSERVICE_MAIN_FUNCTION)GetProcAddress(hServiceDll, "ServiceMain");
 
     /* Allocate a service node in the linked list */
-    Service = HeapAlloc(GetProcessHeap(), 0, sizeof(SERVICE));
+    Service = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(SERVICE));
     if (Service == NULL)
     {
-        DPRINT1("Not enough memory for service: %s\n", ServiceName);
+        DPRINT1("Not enough memory for service: %ls\n", ServiceName);
         return FALSE;
     }
 
-    memset(Service, 0, sizeof(SERVICE));
+    ServiceNameSize = (wcslen(ServiceName) + 1) * sizeof(WCHAR);
-    Service->Name = HeapAlloc(GetProcessHeap(), 0, (wcslen(ServiceName)+1) * sizeof(WCHAR));
+    Service->Name = HeapAlloc(GetProcessHeap(), 0, ServiceNameSize);
     if (Service->Name == NULL)
     {
-        DPRINT1("Not enough memory for service: %s\n", ServiceName);
+        DPRINT1("Not enough memory for service: %ls\n", ServiceName);
         HeapFree(GetProcessHeap(), 0, Service);
         return FALSE;
     }
-    wcscpy(Service->Name, ServiceName);
+    Result = StringCbCopy(Service->Name, ServiceNameSize, ServiceName);
+    ASSERT(SUCCEEDED(Result));
+    (VOID)Result;
 
     Service->hServiceDll = hServiceDll;
     Service->ServiceMainFunc = ServiceMainFunc;
@@ -157,23 +195,26 @@ VOID FreeServices(VOID)
 DWORD LoadServiceCategory(PCWSTR ServiceCategory)
 {
     HKEY hServicesKey;
-    DWORD KeyType;
+    DWORD ValueType;
-    DWORD BufferSize = REG_MAX_DATA_SIZE;
     WCHAR Buffer[REG_MAX_DATA_SIZE];
+    DWORD BufferSize = sizeof(Buffer);
     PCWSTR ServiceName;
     DWORD BufferIndex = 0;
     DWORD NrOfServices = 0;
+    LONG RetVal;
 
     /* Get all the services in this category */
-    if (RegOpenKeyEx(HKEY_LOCAL_MACHINE, SVCHOST_REG_KEY, 0, KEY_READ, &hServicesKey) != ERROR_SUCCESS)
+    RetVal = RegOpenKeyEx(HKEY_LOCAL_MACHINE, SVCHOST_REG_KEY, 0, KEY_READ, &hServicesKey);
+    if (RetVal != ERROR_SUCCESS)
     {
-        DPRINT1("Could not open service category: %s\n", ServiceCategory);
+        DPRINT1("Could not open service category: %ls\n", ServiceCategory);
         return 0;
     }
 
-    if (RegQueryValueEx(hServicesKey, ServiceCategory, NULL, &KeyType, (LPBYTE)Buffer, &BufferSize) != ERROR_SUCCESS)
+    RetVal = RegQueryValueEx(hServicesKey, ServiceCategory, NULL, &ValueType, (LPBYTE)Buffer, &BufferSize);
+    if (RetVal != ERROR_SUCCESS)
     {
-        DPRINT1("Could not open service category (2): %s\n", ServiceCategory);
+        DPRINT1("Could not open service category (2): %ls\n", ServiceCategory);
         RegCloseKey(hServicesKey);
         return 0;
     }
@@ -191,7 +232,7 @@ DWORD LoadServiceCategory(PCWSTR ServiceCategory)
         if (Length == 0)
             break;
 
-        if (PrepareService(ServiceName) == TRUE)
+        if (PrepareService(ServiceName))
             ++NrOfServices;
 
         BufferIndex += Length + 1;
@@ -213,7 +254,7 @@ int wmain(int argc, wchar_t **argv)
         return 0;
     }
 
-    if (wcscmp(argv[1], L"-k") != 0)
+    if (wcscmp(argv[1], L"-k"))
     {
         /* For now, we only handle "-k" */
         return 0;
@@ -226,7 +267,6 @@ int wmain(int argc, wchar_t **argv)
         return 0;
 
     ServiceTable = HeapAlloc(GetProcessHeap(), 0, sizeof(SERVICE_TABLE_ENTRY) * (NrOfServices + 1));
-
     if (ServiceTable != NULL)
     {
         DWORD i;
@@ -235,17 +275,18 @@ int wmain(int argc, wchar_t **argv)
         /* Fill the service table */
         for (i = 0; i < NrOfServices; i++)
         {
-            DPRINT("Loading service: %s\n", Service->Name);
+            DPRINT("Loading service: %ls\n", Service->Name);
             ServiceTable[i].lpServiceName = Service->Name;
             ServiceTable[i].lpServiceProc = Service->ServiceMainFunc;
             Service = Service->Next;
         }
+        ASSERT(Service == NULL);
 
         /* Set a NULL entry to end the service table */
         ServiceTable[i].lpServiceName = NULL;
         ServiceTable[i].lpServiceProc = NULL;
 
-        if (StartServiceCtrlDispatcher(ServiceTable) == FALSE)
+        if (!StartServiceCtrlDispatcher(ServiceTable))
             DPRINT1("Failed to start service control dispatcher, ErrorCode: %lu\n", GetLastError());
 
         HeapFree(GetProcessHeap(), 0, ServiceTable);

reactos/base/services/svchost/svchost.h

@@ -10,21 +10,16 @@
 
 /* INCLUDES ******************************************************************/
 
-#if 0
-#define _CRT_SECURE_NO_DEPRECATE 1
-#endif
-
 #include <stdlib.h>
 #include <stdarg.h>
 #include <windef.h>
 #include <winbase.h>
 #include <winreg.h>
 #include <winsvc.h>
+#include <strsafe.h>
 
 /* DEFINES *******************************************************************/
 
-#define CS_TIMEOUT  1000
-
 typedef struct _SERVICE {
     PWSTR Name;
     HINSTANCE hServiceDll;