From: Bernhard Übelacker Subject: [PATCH] advapi32: test and fix ChangeServiceConfig2 when given a zero length or a null description (try 2) (was: succeed for null pointer in ChangeServiceConfig2) Message-Id: <557602C5.2070200@vr-web.de> Date: Mon, 08 Jun 2015 23:01:57 +0200 Hello, sorry for the delay. The first version of this patch was sent and discussed in March. [1] https://www.winehq.org/pipermail/wine-patches/2015-March/137996.html [2] https://www.winehq.org/pipermail/wine-devel/2015-March/thread.html#106999 This iteration compared to "try 1" has following changes: - move check of descr->lpDescription to server side - add some more tests and check the results. Kind regards, Bernhard PS.: During discussion there was another issue identified when calling ChangeServiceConfig2W with an invalid server handle. This patch does not try to test or solve this issue. Am 19.03.2015 um 13:06 schrieb Bernhard Übelacker: > Hello, > attached patch avoids an issue when trying to run Flatout 2 with copy > protection. > > When starting Flatout2.exe for the first time it wants to install some > services, but shows in wine some error messages. > > The only visible sign in terminal output, that something went wrong, is > this: > err:rpc:I_RpcReceive we got fault packet with status 0x3e6 > > And of course later some dialog showing error 998 (=0x3e6) and a > unhandled exception. > > This is because the SC_RPC_CONFIG_INFOW parameter has in its > descr->lpDescription a null pointer instead of a valid LPWSTR. > > Right now this null pointer would be accessed in > svcctl_ChangeServiceConfig2W and causes an exception there , but is > hidden by the RPC layer. > > (And of course this makes the SF4 protection still not working.) > > Kind regards, > Bernhard > From e8fad9499517ef05a7d1b56cf854db5d4985cc1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernhard=20=C3=9Cbelacker?= Date: Mon, 8 Jun 2015 22:42:05 +0200 Subject: advapi32: test and fix ChangeServiceConfig2 when given a zero length or a null description --- dlls/advapi32/tests/service.c | 66 +++++++++++++++++++++++++++++++++++++++++++ programs/services/rpc.c | 3 ++ 2 files changed, 69 insertions(+) diff --git a/dlls/advapi32/tests/service.c b/dlls/advapi32/tests/service.c index 81d2f74..13a24f5 100644 --- a/dlls/advapi32/tests/service.c +++ b/dlls/advapi32/tests/service.c @@ -36,6 +36,7 @@ static const CHAR spooler[] = "Spooler"; /* Should be available on all platforms static CHAR selfname[MAX_PATH]; static BOOL (WINAPI *pChangeServiceConfig2A)(SC_HANDLE,DWORD,LPVOID); +static BOOL (WINAPI *pChangeServiceConfig2W)(SC_HANDLE,DWORD,LPVOID); static BOOL (WINAPI *pEnumServicesStatusExA)(SC_HANDLE, SC_ENUM_TYPE, DWORD, DWORD, LPBYTE, DWORD, LPDWORD, LPDWORD, LPDWORD, LPCSTR); @@ -57,6 +58,7 @@ static void init_function_pointers(void) HMODULE hadvapi32 = GetModuleHandleA("advapi32.dll"); pChangeServiceConfig2A = (void*)GetProcAddress(hadvapi32, "ChangeServiceConfig2A"); + pChangeServiceConfig2W = (void*)GetProcAddress(hadvapi32, "ChangeServiceConfig2W"); pEnumServicesStatusExA= (void*)GetProcAddress(hadvapi32, "EnumServicesStatusExA"); pEnumServicesStatusExW= (void*)GetProcAddress(hadvapi32, "EnumServicesStatusExW"); pGetSecurityInfo = (void *)GetProcAddress(hadvapi32, "GetSecurityInfo"); @@ -1954,6 +1956,7 @@ static void test_queryconfig2(void) DWORD expected, needed; BYTE buffer[MAX_PATH]; LPSERVICE_DESCRIPTIONA pConfig = (LPSERVICE_DESCRIPTIONA)buffer; + LPSERVICE_DESCRIPTIONW pConfigW = (LPSERVICE_DESCRIPTIONW)buffer; SERVICE_PRESHUTDOWN_INFO preshutdown_info; static const CHAR servicename [] = "Winetest"; static const CHAR displayname [] = "Winetest dummy service"; @@ -1961,6 +1964,9 @@ static void test_queryconfig2(void) static const CHAR dependencies[] = "Master1\0Master2\0+MasterGroup1\0"; static const CHAR password [] = ""; static const CHAR description [] = "Description"; + static const CHAR description_empty[] = ""; + static const WCHAR descriptionW [] = {'D','e','s','c','r','i','p','t','i','o','n','W',0}; + static const WCHAR descriptionW_empty[] = {0}; if(!pQueryServiceConfig2A) { @@ -2121,6 +2127,66 @@ static void test_queryconfig2(void) ret = pQueryServiceConfig2W(svc_handle, SERVICE_CONFIG_DESCRIPTION,buffer, needed,&needed); ok(ret, "expected QueryServiceConfig2W to succeed\n"); + pConfig->lpDescription = (LPSTR)description; + ret = pChangeServiceConfig2A(svc_handle, SERVICE_CONFIG_DESCRIPTION, &buffer); + ok(ret, "expected ChangeServiceConfig2A to succeed\n"); + + pConfig->lpDescription = NULL; + ret = pQueryServiceConfig2A(svc_handle, SERVICE_CONFIG_DESCRIPTION, buffer, sizeof(buffer), &needed); + ok(ret, "expected QueryServiceConfig2A to succeed\n"); + ok(pConfig->lpDescription && !strcmp(description, pConfig->lpDescription), + "expected lpDescription to be %s, got %s\n", description, pConfig->lpDescription); + + pConfig->lpDescription = NULL; + ret = pChangeServiceConfig2A(svc_handle, SERVICE_CONFIG_DESCRIPTION, &buffer); + ok(ret, "expected ChangeServiceConfig2A to succeed\n"); + + pConfig->lpDescription = NULL; + ret = pQueryServiceConfig2A(svc_handle, SERVICE_CONFIG_DESCRIPTION, buffer, sizeof(buffer), &needed); + ok(ret, "expected QueryServiceConfig2A to succeed\n"); + ok(pConfig->lpDescription && !strcmp(description, pConfig->lpDescription), + "expected lpDescription to be %s, got %s\n", description, pConfig->lpDescription); + + pConfig->lpDescription = (LPSTR)description_empty; + ret = pChangeServiceConfig2A(svc_handle, SERVICE_CONFIG_DESCRIPTION, &buffer); + ok(ret, "expected ChangeServiceConfig2A to succeed\n"); + + pConfig->lpDescription = (void*)0xdeadbeef; + ret = pQueryServiceConfig2A(svc_handle, SERVICE_CONFIG_DESCRIPTION, buffer, sizeof(buffer), &needed); + ok(ret, "expected QueryServiceConfig2A to succeed\n"); + ok(!pConfig->lpDescription, + "expected lpDescription to be null, got %s\n", pConfig->lpDescription); + + pConfigW->lpDescription = (LPWSTR)descriptionW; + ret = pChangeServiceConfig2W(svc_handle, SERVICE_CONFIG_DESCRIPTION, &buffer); + ok(ret, "expected ChangeServiceConfig2W to succeed\n"); + + pConfigW->lpDescription = NULL; + ret = pQueryServiceConfig2W(svc_handle, SERVICE_CONFIG_DESCRIPTION, buffer, sizeof(buffer), &needed); + ok(ret, "expected QueryServiceConfig2A to succeed\n"); + ok(pConfigW->lpDescription && !lstrcmpW(descriptionW, pConfigW->lpDescription), + "expected lpDescription to be %s, got %s\n", wine_dbgstr_w(descriptionW), wine_dbgstr_w(pConfigW->lpDescription)); + + pConfigW->lpDescription = NULL; + ret = pChangeServiceConfig2W(svc_handle, SERVICE_CONFIG_DESCRIPTION, &buffer); + ok(ret, "expected ChangeServiceConfig2W to succeed\n"); + + pConfigW->lpDescription = NULL; + ret = pQueryServiceConfig2W(svc_handle, SERVICE_CONFIG_DESCRIPTION, buffer, sizeof(buffer), &needed); + ok(ret, "expected QueryServiceConfig2A to succeed\n"); + ok(pConfigW->lpDescription && !lstrcmpW(descriptionW, pConfigW->lpDescription), + "expected lpDescription to be %s, got %s\n", wine_dbgstr_w(descriptionW), wine_dbgstr_w(pConfigW->lpDescription)); + + pConfigW->lpDescription = (LPWSTR)descriptionW_empty; + ret = pChangeServiceConfig2W(svc_handle, SERVICE_CONFIG_DESCRIPTION, &buffer); + ok(ret, "expected ChangeServiceConfig2W to succeed\n"); + + pConfigW->lpDescription = (void*)0xdeadbeef; + ret = pQueryServiceConfig2W(svc_handle, SERVICE_CONFIG_DESCRIPTION, buffer, sizeof(buffer), &needed); + ok(ret, "expected QueryServiceConfig2A to succeed\n"); + ok(!pConfigW->lpDescription, + "expected lpDescription to be null, got %s\n", wine_dbgstr_w(pConfigW->lpDescription)); + SetLastError(0xdeadbeef); ret = pQueryServiceConfig2W(svc_handle, SERVICE_CONFIG_PRESHUTDOWN_INFO, (LPBYTE)&preshutdown_info, sizeof(preshutdown_info), &needed); diff --git a/programs/services/rpc.c b/programs/services/rpc.c index 89a8c91..753e7ec 100644 --- a/programs/services/rpc.c +++ b/programs/services/rpc.c @@ -798,6 +798,9 @@ DWORD __cdecl svcctl_ChangeServiceConfig2W( SC_RPC_HANDLE hService, SC_RPC_CONFI { WCHAR *descr = NULL; + if (!config.u.descr->lpDescription) + break; + if (config.u.descr->lpDescription[0]) { if (!(descr = strdupW( config.u.descr->lpDescription ))) -- 2.1.4