[ros-diffs] [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/confi…
==============================================================================
--- 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/datab…
==============================================================================
--- 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/rpcse…
==============================================================================
--- 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/servi…
==============================================================================
--- 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)
     {