[ros-diffs] [tfaber] 59360: [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

Author: tfaber
Date: Sat Jun 29 13:01:35 2013
New Revision: 59360

URL: http://svn.reactos.org/svn/reactos?rev=59360&view=rev
Log:
[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

Modified:
    trunk/reactos/base/services/svchost/svchost.c
    trunk/reactos/base/services/svchost/svchost.h

Modified: trunk/reactos/base/services/svchost/svchost.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/services/svchost/svch…
==============================================================================
--- trunk/reactos/base/services/svchost/svchost.c	[iso-8859-1] (original)
+++ trunk/reactos/base/services/svchost/svchost.c	[iso-8859-1] Sat Jun 29 13:01:35 2013
@@ -19,7 +19,6 @@
 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 @@
 BOOL PrepareService(PCWSTR ServiceName)
 {
     HKEY hServiceKey;
-    WCHAR ServiceKeyBuffer[MAX_PATH + 1];
-    DWORD LeftOfBuffer = sizeof(ServiceKeyBuffer) / sizeof(ServiceKeyBuffer[0]);
-    DWORD KeyType;
-    PWSTR Buffer = NULL;
-    DWORD BufferSize = MAX_PATH + 1;
+    WCHAR ServiceKeyBuffer[512];
+    HRESULT Result;
+    DWORD ValueType;
+    PWSTR Buffer;
+    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);
-    LeftOfBuffer -= wcslen(SERVICE_KEY);
-    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);
+    Result = StringCbCopy(ServiceKeyBuffer, sizeof(ServiceKeyBuffer), SERVICE_KEY);
+    if (FAILED(Result))
+    {
+        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)
-    {
-        DPRINT1("Could not open service key (%s)\n", ServiceKeyBuffer);
-        return FALSE;
-    }
-
-    do
-    {
-        if (Buffer)
+    RetVal = RegOpenKeyEx(HKEY_LOCAL_MACHINE, ServiceKeyBuffer, 0, KEY_READ,
&hServiceKey);
+    if (RetVal != ERROR_SUCCESS)
+    {
+        DPRINT1("Could not open service key (%ls)\n", ServiceKeyBuffer);
+        return FALSE;
+    }
+
+    BufferSize = 0;
+    RetVal = RegQueryValueEx(hServiceKey, L"ServiceDll", NULL, &ValueType,
NULL, &BufferSize);
+    if (RetVal != ERROR_SUCCESS)
+    {
+        RegCloseKey(hServiceKey);
+        return FALSE;
+    }
+
+    Buffer = HeapAlloc(GetProcessHeap(), 0, BufferSize);
+    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("Invalid ServiceDll path: %ls, ErrorCode: %lu\n", Buffer,
GetLastError());
             HeapFree(GetProcessHeap(), 0, Buffer);
-
-        Buffer = HeapAlloc(GetProcessHeap(), 0, BufferSize);
-        if (Buffer == NULL)
-        {
-            DPRINT1("Not enough memory for service: %s\n", ServiceName);
             return FALSE;
         }
-
-        RetVal = RegQueryValueEx(hServiceKey, L"ServiceDll", NULL,
&KeyType, (LPBYTE)Buffer, &BufferSize);
-
-    } while (RetVal == ERROR_MORE_DATA);
-
-
-    RegCloseKey(hServiceKey);
-
-    if (RetVal != ERROR_SUCCESS || BufferSize == 0)
-    {
-        DPRINT1("Could not read 'ServiceDll' value from service: %s,
ErrorCode: 0x%x\n", ServiceName, RetVal);
+        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;
+        }
+
         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);
+    }
+    else if (ValueType == REG_SZ)
+    {
+        DllPath = Buffer;
+    }
+    else
+    {
+        DPRINT1("Invalid ServiceDll value type %lu for service: %ls, ErrorCode:
%lu\n", ValueType, ServiceName, GetLastError());
         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());
-        return FALSE;
-    }
+        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);
-        return FALSE;
-    }
-
-    memset(Service, 0, sizeof(SERVICE));
-    Service->Name = HeapAlloc(GetProcessHeap(), 0, (wcslen(ServiceName)+1) *
sizeof(WCHAR));
+        DPRINT1("Not enough memory for service: %ls\n", ServiceName);
+        return FALSE;
+    }
+
+    ServiceNameSize = (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 @@
 DWORD LoadServiceCategory(PCWSTR ServiceCategory)
 {
     HKEY hServicesKey;
-    DWORD KeyType;
-    DWORD BufferSize = REG_MAX_DATA_SIZE;
+    DWORD ValueType;
     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)
-    {
-        DPRINT1("Could not open service category: %s\n", ServiceCategory);
-        return 0;
-    }
-
-    if (RegQueryValueEx(hServicesKey, ServiceCategory, NULL, &KeyType,
(LPBYTE)Buffer, &BufferSize) != ERROR_SUCCESS)
-    {
-        DPRINT1("Could not open service category (2): %s\n", ServiceCategory);
+    RetVal = RegOpenKeyEx(HKEY_LOCAL_MACHINE, SVCHOST_REG_KEY, 0, KEY_READ,
&hServicesKey);
+    if (RetVal != ERROR_SUCCESS)
+    {
+        DPRINT1("Could not open service category: %ls\n", ServiceCategory);
+        return 0;
+    }
+
+    RetVal = RegQueryValueEx(hServicesKey, ServiceCategory, NULL, &ValueType,
(LPBYTE)Buffer, &BufferSize);
+    if (RetVal != ERROR_SUCCESS)
+    {
+        DPRINT1("Could not open service category (2): %ls\n",
ServiceCategory);
         RegCloseKey(hServicesKey);
         return 0;
     }
@@ -191,7 +232,7 @@
         if (Length == 0)
             break;
 
-        if (PrepareService(ServiceName) == TRUE)
+        if (PrepareService(ServiceName))
             ++NrOfServices;
 
         BufferIndex += Length + 1;
@@ -213,7 +254,7 @@
         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 @@
         return 0;
 
     ServiceTable = HeapAlloc(GetProcessHeap(), 0, sizeof(SERVICE_TABLE_ENTRY) *
(NrOfServices + 1));
-
     if (ServiceTable != NULL)
     {
         DWORD i;
@@ -235,17 +275,18 @@
         /* 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);

Modified: trunk/reactos/base/services/svchost/svchost.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/services/svchost/svch…
==============================================================================
--- trunk/reactos/base/services/svchost/svchost.h	[iso-8859-1] (original)
+++ trunk/reactos/base/services/svchost/svchost.h	[iso-8859-1] Sat Jun 29 13:01:35 2013
@@ -10,20 +10,15 @@
 
 /* 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;