From 1380373ae4c305285fc033f0c2a9581dc0f15818 Mon Sep 17 00:00:00 2001 From: Thomas Faber Date: Sat, 29 Jun 2013 13:01:35 +0000 Subject: [PATCH] [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 | 167 +++++++++++++++--------- reactos/base/services/svchost/svchost.h | 7 +- 2 files changed, 105 insertions(+), 69 deletions(-) diff --git a/reactos/base/services/svchost/svchost.c b/reactos/base/services/svchost/svchost.c index a6a5b436c0a..a04346962cd 100644 --- a/reactos/base/services/svchost/svchost.c +++ b/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]; - 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) + Result = StringCbCopy(ServiceKeyBuffer, sizeof(ServiceKeyBuffer), SERVICE_KEY); + if (FAILED(Result)) { - 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) - HeapFree(GetProcessHeap(), 0, Buffer); + RegCloseKey(hServiceKey); + return FALSE; + } - Buffer = HeapAlloc(GetProcessHeap(), 0, BufferSize); - if (Buffer == NULL) + 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("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); - - } while (RetVal == ERROR_MORE_DATA); - - - RegCloseKey(hServiceKey); - - if (RetVal != ERROR_SUCCESS || BufferSize == 0) + HeapFree(GetProcessHeap(), 0, Buffer); + } + else if (ValueType == REG_SZ) { - 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)); - Service->Name = HeapAlloc(GetProcessHeap(), 0, (wcslen(ServiceName)+1) * sizeof(WCHAR)); + 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 @@ VOID FreeServices(VOID) 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) + 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); diff --git a/reactos/base/services/svchost/svchost.h b/reactos/base/services/svchost/svchost.h index 64aa115c58a..1702b3da692 100644 --- a/reactos/base/services/svchost/svchost.h +++ b/reactos/base/services/svchost/svchost.h @@ -10,21 +10,16 @@ /* INCLUDES ******************************************************************/ -#if 0 -#define _CRT_SECURE_NO_DEPRECATE 1 -#endif - #include #include #include #include #include #include +#include /* DEFINES *******************************************************************/ -#define CS_TIMEOUT 1000 - typedef struct _SERVICE { PWSTR Name; HINSTANCE hServiceDll;