[ekohl] 57328: [ADVAPI32/SERVICES] This patch fixes various things, from Coverity code defects to conversion warnings : - CID 715948 (logically dead code @ services/rpcserver.c) - try to fix CID 71...
Author: ekohl Date: Tue Sep 18 21:54:43 2012 New Revision: 57328
URL: http://svn.reactos.org/svn/reactos?rev=57328&view=rev Log: [ADVAPI32/SERVICES] This patch fixes various things, from Coverity code defects to conversion warnings :
- CID 715948 (logically dead code @ services/rpcserver.c) - try to fix CID 716332/3 (resource leaks) by rewriting the ScmReadString function (@ services/config.c) - zero out the freshly allocated memory (@ services) - try to fix CID 716126/7/8 (untrusted value as argument @ advapi32/services/sctrl.c)
Fix also some "size_t to DWORD" warnings on x64 build (@ advapi32/services/scm.c).
Patch by Hermes BELUSCA - MAITO. Fixes CORE-6606.
Modified: trunk/reactos/base/system/services/config.c trunk/reactos/base/system/services/database.c trunk/reactos/base/system/services/rpcserver.c trunk/reactos/base/system/services/services.h trunk/reactos/dll/win32/advapi32/service/scm.c trunk/reactos/dll/win32/advapi32/service/sctrl.c
Modified: trunk/reactos/base/system/services/config.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/base/system/services/config... ============================================================================== --- trunk/reactos/base/system/services/config.c [iso-8859-1] (original) +++ trunk/reactos/base/system/services/config.c [iso-8859-1] Tue Sep 18 21:54:43 2012 @@ -246,19 +246,17 @@
DWORD ScmReadString(HKEY hServiceKey, - LPWSTR lpValueName, + LPCWSTR lpValueName, LPWSTR *lpValue) { - DWORD dwError; - DWORD dwSize; - DWORD dwType; - DWORD dwSizeNeeded; + DWORD dwError = 0; + DWORD dwSize = 0; + DWORD dwType = 0; + LPWSTR ptr = NULL; LPWSTR expanded = NULL; - LPWSTR ptr = NULL;
*lpValue = NULL;
- dwSize = 0; dwError = RegQueryValueExW(hServiceKey, lpValueName, 0, @@ -268,7 +266,7 @@ if (dwError != ERROR_SUCCESS) return dwError;
- ptr = HeapAlloc(GetProcessHeap(), 0, dwSize); + ptr = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwSize); if (ptr == NULL) return ERROR_NOT_ENOUGH_MEMORY;
@@ -279,38 +277,46 @@ (LPBYTE)ptr, &dwSize); if (dwError != ERROR_SUCCESS) - goto done; + { + HeapFree(GetProcessHeap(), 0, ptr); + return dwError; + }
if (dwType == REG_EXPAND_SZ) { /* Expand the value... */ - dwSizeNeeded = ExpandEnvironmentStringsW((LPCWSTR)ptr, NULL, 0); - if (dwSizeNeeded == 0) + dwSize = ExpandEnvironmentStringsW(ptr, NULL, 0); + if (dwSize > 0) + { + expanded = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwSize * sizeof(WCHAR)); + if (expanded) + { + if (dwSize == ExpandEnvironmentStringsW(ptr, expanded, dwSize)) + { + *lpValue = expanded; + dwError = ERROR_SUCCESS; + } + else + { + dwError = GetLastError(); + HeapFree(GetProcessHeap(), 0, expanded); + } + } + else + { + dwError = ERROR_NOT_ENOUGH_MEMORY; + } + } + else { dwError = GetLastError(); - goto done; - } - expanded = HeapAlloc(GetProcessHeap(), 0, dwSizeNeeded * sizeof(WCHAR)); - if (dwSizeNeeded < ExpandEnvironmentStringsW((LPCWSTR)ptr, expanded, dwSizeNeeded)) - { - dwError = GetLastError(); - goto done; - } - *lpValue = expanded; + } + HeapFree(GetProcessHeap(), 0, ptr); - dwError = ERROR_SUCCESS; } else { *lpValue = ptr; - } - -done: - if (dwError != ERROR_SUCCESS) - { - HeapFree(GetProcessHeap(), 0, ptr); - if (expanded) - HeapFree(GetProcessHeap(), 0, expanded); }
return dwError;
Modified: trunk/reactos/base/system/services/database.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/base/system/services/databa... ============================================================================== --- trunk/reactos/base/system/services/database.c [iso-8859-1] (original) +++ trunk/reactos/base/system/services/database.c [iso-8859-1] Tue Sep 18 21:54:43 2012 @@ -591,7 +591,7 @@ if (dwMaxSubkeyLen > sizeof(szNameBuf) / sizeof(WCHAR)) { /* Name too big: alloc a buffer for it */ - lpszName = HeapAlloc(GetProcessHeap(), 0, dwMaxSubkeyLen * sizeof(WCHAR)); + lpszName = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwMaxSubkeyLen * sizeof(WCHAR)); }
if (!lpszName)
Modified: trunk/reactos/base/system/services/rpcserver.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/base/system/services/rpcser... ============================================================================== --- trunk/reactos/base/system/services/rpcserver.c [iso-8859-1] (original) +++ trunk/reactos/base/system/services/rpcserver.c [iso-8859-1] Tue Sep 18 21:54:43 2012 @@ -301,7 +301,7 @@ if (dwError != ERROR_SUCCESS && dwError != ERROR_MORE_DATA) goto findFreeTag;
- pdwGroupTags = HeapAlloc(GetProcessHeap(), 0, cbDataSize); + pdwGroupTags = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, cbDataSize); if (!pdwGroupTags) { dwError = ERROR_NOT_ENOUGH_MEMORY; @@ -1635,11 +1635,6 @@ }
lpService = (PSERVICE)hServiceStatus; - if (lpService == NULL) - { - DPRINT("lpService == NULL!\n"); - return ERROR_INVALID_HANDLE; - }
/* Check current state */ if (!ScmIsValidServiceState(lpServiceStatus->dwCurrentState)) @@ -1819,7 +1814,7 @@
/* Update the display name */ lpDisplayNameW = HeapAlloc(GetProcessHeap(), - 0, + HEAP_ZERO_MEMORY, (wcslen(lpDisplayName) + 1) * sizeof(WCHAR)); if (lpDisplayNameW == NULL) { @@ -2142,7 +2137,8 @@ *lpDisplayName != 0 && _wcsicmp(lpService->lpDisplayName, lpDisplayName) != 0) { - lpService->lpDisplayName = HeapAlloc(GetProcessHeap(), 0, + lpService->lpDisplayName = HeapAlloc(GetProcessHeap(), + HEAP_ZERO_MEMORY, (wcslen(lpDisplayName) + 1) * sizeof(WCHAR)); if (lpService->lpDisplayName == NULL) { @@ -2424,7 +2420,7 @@
/* Allocate memory for array of service pointers */ lpServicesArray = HeapAlloc(GetProcessHeap(), - 0, + HEAP_ZERO_MEMORY, (dwServicesReturned + 1) * sizeof(PSERVICE)); if (!lpServicesArray) { @@ -2447,7 +2443,7 @@ goto Done; }
- lpServicesPtr = (LPENUM_SERVICE_STATUSW) lpServices; + lpServicesPtr = (LPENUM_SERVICE_STATUSW)lpServices; lpStr = (LPWSTR)(lpServices + (dwServicesReturned * sizeof(ENUM_SERVICE_STATUSW)));
/* Copy EnumDepenedentService to Buffer */ @@ -2470,7 +2466,7 @@ lpServicesPtr->lpServiceName = (LPWSTR)((ULONG_PTR)lpStr - (ULONG_PTR)lpServices); lpStr += (wcslen(lpService->lpServiceName) + 1);
- lpServicesPtr ++; + lpServicesPtr++; }
*lpServicesReturned = dwServicesReturned; @@ -3190,7 +3186,7 @@ { /* Set the display name */ lpDisplayNameW = HeapAlloc(GetProcessHeap(), - 0, + HEAP_ZERO_MEMORY, (strlen(lpDisplayName) + 1) * sizeof(WCHAR)); if (lpDisplayNameW == NULL) { @@ -3268,7 +3264,7 @@ { /* Set the image path */ lpBinaryPathNameW = HeapAlloc(GetProcessHeap(), - 0, + HEAP_ZERO_MEMORY, (strlen(lpBinaryPathName) + 1) * sizeof(WCHAR)); if (lpBinaryPathNameW == NULL) { @@ -3314,7 +3310,7 @@ if (lpLoadOrderGroup != NULL && *lpLoadOrderGroup != 0) { lpLoadOrderGroupW = HeapAlloc(GetProcessHeap(), - 0, + HEAP_ZERO_MEMORY, (strlen(lpLoadOrderGroup) + 1) * sizeof(WCHAR)); if (lpLoadOrderGroupW == NULL) { @@ -3372,7 +3368,7 @@ if (lpDependencies != NULL && *lpDependencies != 0) { lpDependenciesW = HeapAlloc(GetProcessHeap(), - 0, + HEAP_ZERO_MEMORY, (strlen((LPSTR)lpDependencies) + 1) * sizeof(WCHAR)); if (lpDependenciesW == NULL) { @@ -3446,7 +3442,7 @@ if (lpServiceName) { len = MultiByteToWideChar(CP_ACP, 0, lpServiceName, -1, NULL, 0); - lpServiceNameW = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); + lpServiceNameW = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len * sizeof(WCHAR)); if (!lpServiceNameW) { SetLastError(ERROR_NOT_ENOUGH_MEMORY); @@ -3458,7 +3454,7 @@ if (lpDisplayName) { len = MultiByteToWideChar(CP_ACP, 0, lpDisplayName, -1, NULL, 0); - lpDisplayNameW = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); + lpDisplayNameW = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len * sizeof(WCHAR)); if (!lpDisplayNameW) { SetLastError(ERROR_NOT_ENOUGH_MEMORY); @@ -3470,7 +3466,7 @@ if (lpBinaryPathName) { len = MultiByteToWideChar(CP_ACP, 0, lpBinaryPathName, -1, NULL, 0); - lpBinaryPathNameW = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); + lpBinaryPathNameW = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len * sizeof(WCHAR)); if (!lpBinaryPathNameW) { SetLastError(ERROR_NOT_ENOUGH_MEMORY); @@ -3482,7 +3478,7 @@ if (lpLoadOrderGroup) { len = MultiByteToWideChar(CP_ACP, 0, lpLoadOrderGroup, -1, NULL, 0); - lpLoadOrderGroupW = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); + lpLoadOrderGroupW = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len * sizeof(WCHAR)); if (!lpLoadOrderGroupW) { SetLastError(ERROR_NOT_ENOUGH_MEMORY); @@ -3502,7 +3498,7 @@ } dwDependenciesLength++;
- lpDependenciesW = HeapAlloc(GetProcessHeap(), 0, dwDependenciesLength * sizeof(WCHAR)); + lpDependenciesW = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, dwDependenciesLength * sizeof(WCHAR)); if (!lpDependenciesW) { SetLastError(ERROR_NOT_ENOUGH_MEMORY); @@ -3514,7 +3510,7 @@ if (lpServiceStartName) { len = MultiByteToWideChar(CP_ACP, 0, lpServiceStartName, -1, NULL, 0); - lpServiceStartNameW = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); + lpServiceStartNameW = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, len * sizeof(WCHAR)); if (!lpServiceStartNameW) { SetLastError(ERROR_NOT_ENOUGH_MEMORY); @@ -3638,7 +3634,7 @@
/* Allocate memory for array of service pointers */ lpServicesArray = HeapAlloc(GetProcessHeap(), - 0, + HEAP_ZERO_MEMORY, (dwServicesReturned + 1) * sizeof(PSERVICE)); if (!lpServicesArray) { @@ -3698,7 +3694,7 @@ lpServicesPtr->lpServiceName = (LPSTR)((ULONG_PTR)lpStr - (ULONG_PTR)lpServices); lpStr += strlen(lpStr) + 1;
- lpServicesPtr ++; + lpServicesPtr++; }
*lpServicesReturned = dwServicesReturned; @@ -4755,7 +4751,7 @@ dwLength = (DWORD)((strlen(Info.lpDescription) + 1) * sizeof(WCHAR));
lpServiceDescriptonW = HeapAlloc(GetProcessHeap(), - 0, + HEAP_ZERO_MEMORY, dwLength + sizeof(SERVICE_DESCRIPTIONW)); if (!lpServiceDescriptonW) { @@ -4797,7 +4793,7 @@ dwLength = dwRebootLen + dwCommandLen + sizeof(SERVICE_FAILURE_ACTIONSW);
lpServiceFailureActionsW = HeapAlloc(GetProcessHeap(), - 0, + HEAP_ZERO_MEMORY, dwLength); if (!lpServiceFailureActionsW) {
Modified: trunk/reactos/base/system/services/services.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/base/system/services/servic... ============================================================================== --- trunk/reactos/base/system/services/services.h [iso-8859-1] (original) +++ trunk/reactos/base/system/services/services.h [iso-8859-1] Tue Sep 18 21:54:43 2012 @@ -106,7 +106,7 @@ BOOL ScmIsDeleteFlagSet(HKEY hServiceKey);
DWORD ScmReadString(HKEY hServiceKey, - LPWSTR lpValueName, + LPCWSTR lpValueName, LPWSTR *lpValue);
DWORD
Modified: trunk/reactos/dll/win32/advapi32/service/scm.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/advapi32/service/... ============================================================================== --- trunk/reactos/dll/win32/advapi32/service/scm.c [iso-8859-1] (original) +++ trunk/reactos/dll/win32/advapi32/service/scm.c [iso-8859-1] Tue Sep 18 21:54:43 2012 @@ -287,7 +287,7 @@ { DWORD dwError; DWORD dwDependenciesLength = 0; - DWORD dwLength; + SIZE_T cchLength; LPCSTR lpStr; DWORD dwPasswordLength = 0; LPBYTE lpEncryptedPassword = NULL; @@ -300,16 +300,16 @@ lpStr = lpDependencies; while (*lpStr) { - dwLength = strlen(lpStr) + 1; - dwDependenciesLength += dwLength; - lpStr = lpStr + dwLength; + cchLength = strlen(lpStr) + 1; + dwDependenciesLength += (DWORD)cchLength; + lpStr = lpStr + cchLength; } dwDependenciesLength++; }
/* FIXME: Encrypt the password */ lpEncryptedPassword = (LPBYTE)lpPassword; - dwPasswordLength = (lpPassword ? (strlen(lpPassword) + 1) * sizeof(CHAR) : 0); + dwPasswordLength = (DWORD)(lpPassword ? (strlen(lpPassword) + 1) * sizeof(CHAR) : 0);
RpcTryExcept { @@ -365,7 +365,7 @@ { DWORD dwError; DWORD dwDependenciesLength = 0; - DWORD dwLength; + SIZE_T cchLength; LPCWSTR lpStr; DWORD dwPasswordLength = 0; LPBYTE lpEncryptedPassword = NULL; @@ -378,11 +378,12 @@ lpStr = lpDependencies; while (*lpStr) { - dwLength = wcslen(lpStr) + 1; - dwDependenciesLength += dwLength; - lpStr = lpStr + dwLength; + cchLength = wcslen(lpStr) + 1; + dwDependenciesLength += (DWORD)cchLength; + lpStr = lpStr + cchLength; } dwDependenciesLength++; + dwDependenciesLength *= sizeof(WCHAR); }
/* FIXME: Encrypt the password */ @@ -547,7 +548,7 @@ SC_HANDLE hService = NULL; DWORD dwDependenciesLength = 0; DWORD dwError; - DWORD dwLength; + SIZE_T cchLength; LPCSTR lpStr; DWORD dwPasswordLength = 0; LPBYTE lpEncryptedPassword = NULL; @@ -568,16 +569,16 @@ lpStr = lpDependencies; while (*lpStr) { - dwLength = strlen(lpStr) + 1; - dwDependenciesLength += dwLength; - lpStr = lpStr + dwLength; + cchLength = strlen(lpStr) + 1; + dwDependenciesLength += (DWORD)cchLength; + lpStr = lpStr + cchLength; } dwDependenciesLength++; }
/* FIXME: Encrypt the password */ lpEncryptedPassword = (LPBYTE)lpPassword; - dwPasswordLength = (lpPassword ? (strlen(lpPassword) + 1) * sizeof(CHAR) : 0); + dwPasswordLength = (DWORD)(lpPassword ? (strlen(lpPassword) + 1) * sizeof(CHAR) : 0);
RpcTryExcept { @@ -639,7 +640,7 @@ SC_HANDLE hService = NULL; DWORD dwDependenciesLength = 0; DWORD dwError; - DWORD dwLength; + SIZE_T cchLength; LPCWSTR lpStr; DWORD dwPasswordLength = 0; LPBYTE lpEncryptedPassword = NULL; @@ -660,18 +661,17 @@ lpStr = lpDependencies; while (*lpStr) { - dwLength = wcslen(lpStr) + 1; - dwDependenciesLength += dwLength; - lpStr = lpStr + dwLength; + cchLength = wcslen(lpStr) + 1; + dwDependenciesLength += (DWORD)cchLength; + lpStr = lpStr + cchLength; } dwDependenciesLength++; - dwDependenciesLength *= sizeof(WCHAR); }
/* FIXME: Encrypt the password */ lpEncryptedPassword = (LPBYTE)lpPassword; - dwPasswordLength = (lpPassword ? (wcslen(lpPassword) + 1) * sizeof(WCHAR) : 0); + dwPasswordLength = (DWORD)(lpPassword ? (wcslen(lpPassword) + 1) * sizeof(WCHAR) : 0);
RpcTryExcept {
Modified: trunk/reactos/dll/win32/advapi32/service/sctrl.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/advapi32/service/... ============================================================================== --- trunk/reactos/dll/win32/advapi32/service/sctrl.c [iso-8859-1] (original) +++ trunk/reactos/dll/win32/advapi32/service/sctrl.c [iso-8859-1] Tue Sep 18 21:54:43 2012 @@ -290,6 +290,9 @@ LPWSTR *lpArg; DWORD i;
+ if (ControlPacket == NULL || lpArgCount == NULL || lpArgVector == NULL) + return ERROR_INVALID_PARAMETER; + *lpArgCount = 0; *lpArgVector = NULL;
@@ -334,6 +337,9 @@ DWORD dwAnsiSize; DWORD i;
+ if (ControlPacket == NULL || lpArgCount == NULL || lpArgVector == NULL) + return ERROR_INVALID_PARAMETER; + *lpArgCount = 0; *lpArgVector = NULL;
@@ -398,6 +404,9 @@ HANDLE ThreadHandle; DWORD ThreadId; DWORD dwError; + + if (lpService == NULL || ControlPacket == NULL) + return ERROR_INVALID_PARAMETER;
TRACE("ScStartService() called\n"); TRACE("Size: %lu\n", ControlPacket->dwSize); @@ -470,6 +479,9 @@ ScControlService(PACTIVE_SERVICE lpService, PSCM_CONTROL_PACKET ControlPacket) { + if (lpService == NULL || ControlPacket == NULL) + return ERROR_INVALID_PARAMETER; + TRACE("ScControlService() called\n"); TRACE("Size: %lu\n", ControlPacket->dwSize); TRACE("Service: %S\n", (PWSTR)((PBYTE)ControlPacket + ControlPacket->dwServiceNameOffset)); @@ -504,6 +516,9 @@ DWORD dwError;
TRACE("ScDispatcherLoop() called\n"); + + if (ControlPacket == NULL || dwBufferSize < sizeof(SCM_CONTROL_PACKET)) + return FALSE;
while (TRUE) {
-
ekohl＠svn.reactos.org