Re: [PATCH] advapi32: succeed for null pointer in ChangeServiceConfig2
On 19.03.2015 15:06, Bernhard Übelacker wrote:
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
Is it actually called with infolevel 1?
Hello Nikolay, thanks for your review. Am 19.03.2015 um 13:34 schrieb Nikolay Sivov:
In this case it's better to return earlier, no need to step into try..expect block if you know you won't call anything. So would it be safe to move the assignments out of the try..except block (please see attached patch)?
Is it actually called with infolevel 1? Yes, when the error happened it was called with SERVICE_CONFIG_DESCRIPTION, but in my patch I missed that I just wanted to early return in that case.
Or should we just do the call to the RPC server and decide there? Kind regards, Bernhard
On 19.03.2015 16:07, Bernhard Übelacker wrote:
Hello Nikolay, thanks for your review.
Am 19.03.2015 um 13:34 schrieb Nikolay Sivov:
In this case it's better to return earlier, no need to step into try..expect block if you know you won't call anything. So would it be safe to move the assignments out of the try..except block (please see attached patch)?
+ if (dwInfoLevel == SERVICE_CONFIG_DESCRIPTION && + info.u.descr->lpDescription == NULL) { - SC_RPC_CONFIG_INFOW info; + err = ERROR_SUCCESS; + return TRUE; + }
Yes, something like that, expect you don't need to set 'err' at all in this case. But now that I'm thinking more about it, server side should probably be fixed.
Or should we just do the call to the RPC server and decide there?
That's a good question. There's two ways to test it - connect to a server directly from test program and see what happens on NULL description (to make it work in wine you'll need to use W-call to a server as we do A->W conversion at client), indirectly - if you call ChangeServiceConfig2() with invalid handle and NULL description for info level 1. In latter case I expect it to fail on invalid handle, and because handle validation is supposed to be done on server side only you will get your answer. Another thing to test is to try and set description to NULL and then query it back with QueryServiceConfig2W() - it's possible that it's actually allowed to set it to NULL and 0 error value indicates just that.
Kind regards, Bernhard
Hello Nikolay, Am 19.03.2015 um 14:35 schrieb Nikolay Sivov:
That's a good question. There's two ways to test it - connect to a server directly from test program and see what happens on NULL description (to make it work in wine you'll need to use W-call to a server as we do A->W conversion at client), indirectly - if you call ChangeServiceConfig2() with invalid handle and NULL description for info level 1.
In latter case I expect it to fail on invalid handle, and because handle validation is supposed to be done on server side only you will get your answer. I do not know if I completely understand you. If we call ChangeServiceConfig2W with an invalid handle and would still succeed, we can say that the check is done on client side?
But now make test fails, because the handle is needed for the RPC call? And don't we need also on windows the handle to get to the server?
Another thing to test is to try and set description to NULL and then query it back with QueryServiceConfig2W() - it's possible that it's actually allowed to set it to NULL and 0 error value indicates just that.
I tried to do some more tests: https://testbot.winehq.org/JobDetails.pl?Key=12315&log_201=1#k201 Were that the tests you proposed? And should the handle also be validated on client side in wine instead of crashing, as that differs from windows too? Kind regards, Bernhard
On 19.03.2015 20:33, Bernhard Übelacker wrote:
Hello Nikolay,
Am 19.03.2015 um 14:35 schrieb Nikolay Sivov:
That's a good question. There's two ways to test it - connect to a server directly from test program and see what happens on NULL description (to make it work in wine you'll need to use W-call to a server as we do A->W conversion at client), indirectly - if you call ChangeServiceConfig2() with invalid handle and NULL description for info level 1.
In latter case I expect it to fail on invalid handle, and because handle validation is supposed to be done on server side only you will get your answer. I do not know if I completely understand you. If we call ChangeServiceConfig2W with an invalid handle and would still succeed, we can say that the check is done on client side?
I mean that handle can only be validated at server side as server produces them. If you pass some invalid info and a valid handle it means that server also checks that info is valid after it validated a handle as your test result suggest.
But now make test fails, because the handle is needed for the RPC call? And don't we need also on windows the handle to get to the server?
Yes, to make an RPC call you need some extra steps, similar to what advapi32 is doing as a client. But you don't have to test RPC calls directly, I mentioned it like an option to verify all layers properly. Those two lines suggest that everything is checked at server side most likely: --- service.c:2095: pChangeServiceConfig2W with invalid handle and valid description: ret=0 GetLastError()=6 service.c:2107: pChangeServiceConfig2W with invalid handle and NULL description: ret=0 GetLastError()=6 ---
Another thing to test is to try and set description to NULL and then query it back with QueryServiceConfig2W() - it's possible that it's actually allowed to set it to NULL and 0 error value indicates just that.
I tried to do some more tests: https://testbot.winehq.org/JobDetails.pl?Key=12315&log_201=1#k201
Were that the tests you proposed?
Something like that, yes: --- pConfigW->lpDescription = NULL; ret = pChangeServiceConfig2W(svc_handle, SERVICE_CONFIG_DESCRIPTION, &buffer); pConfig->lpDescription = 0; needed = 0; ret = pQueryServiceConfig2W(svc_handle, SERVICE_CONFIG_DESCRIPTION,buffer,sizeof(buffer),&needed); --- It's hard to follow a bit, unless you use ok() checks. And if you expect NULL description you should set a pointer to some not-null value before a call to see a difference. So bottom line is that you should start with a simple test like that one: - change to NULL description from a not-NULL one; - query it back to see what's returned. Once we have a test for that it will be easy to fix this issue.
And should the handle also be validated on client side in wine instead of crashing, as that differs from windows too?
No, you're not supposed to check for handle validity at client, RPC API doesn't offer a way to do that. And yes, all crashes you see with wine that don't happen on windows need fixing of course.
Kind regards, Bernhard
Hello Nikolay, hello Stefan, Am 19.03.2015 um 18:57 schrieb Nikolay Sivov:> So bottom line is that you should start with a simple test like that one:
- change to NULL description from a not-NULL one; - query it back to see what's returned.
Once we have a test for that it will be easy to fix this issue.
Would this patch for the test side be acceptable? https://testbot.winehq.org/JobDetails.pl?Key=12322 And should the test be switched to the *A functions? https://testbot.winehq.org/JobDetails.pl?Key=12374 Am 20.03.2015 um 12:17 schrieb Stefan Dösinger:
Adding the test like this may work, but I don't think it is the right way. This call may overwrite settings set by the previous ChangeServiceConfig2A call if it succeeds. If it fails (and doesn't crash) the test can continue just fine since the previous call set up the configuration.
I think it would be better to move this into its own test.
Would it be sufficient to move the changes behind the other accesses to the description in the same test? So there is no need to jump to cleanup on failure. Kind regards, Bernhard
On 03/23/2015 10:50 PM, Bernhard Übelacker wrote:
Hello Nikolay, hello Stefan, ... Would this patch for the test side be acceptable? https://testbot.winehq.org/JobDetails.pl?Key=12322
+ if(!pChangeServiceConfig2W) + { + win_skip("function ChangeServiceConfig2W not present\n"); + goto cleanup; + }
Is it really possible to have ChangeServiceConfig2A() and not ChangeServiceConfig2W()? If those are both present on win2k then you don't need a skip.
+ SetLastError(0xdeadbeef); + pConfigW->lpDescription = (LPWSTR)descriptionW; + ret = pChangeServiceConfig2W(svc_handle, SERVICE_CONFIG_DESCRIPTION, &buffer); + ok(ret, "expected ChangeServiceConfig2W to succeed\n");
No need to set last error in this case.
+ SetLastError(0xdeadbeef); + pConfigW->lpDescription = 0xdeadbeef; + 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));
Same here, also it's enough to set description to NULL, as you expect it not to be NULL on return.
+ SetLastError(0xdeadbeef); + pConfigW->lpDescription = 0; + ret = pChangeServiceConfig2W(svc_handle, SERVICE_CONFIG_DESCRIPTION, &buffer); + todo_wine + { + ok(ret, "expected ChangeServiceConfig2W to succeed\n"); + }
Again, no need to set last error. So according to test setting NULL description does not fail, but doesn't change description either. Could you add another test with invalid service handle and NULL description to see if it fails with invalid handle error? If it does fail with such error then you need to fix a server part.
And should the test be switched to the *A functions? https://testbot.winehq.org/JobDetails.pl?Key=12374
I think we need both. Especially since Wine forwards A->W at client.
Hello Nikolay, thanks for your patience. Could you please take a look? - removed superfluous initializations - added tests with invalid handle and null description https://testbot.winehq.org/JobDetails.pl?Key=12385 Kind regards, Bernhard
On 24.03.2015 17:59, Bernhard Übelacker wrote: Yes, this looks much better. Now to make it shorter please combine this tests with a fix into a single patch - this way you won't need exception handling noise.
Hello Nikolay, Am 24.03.2015 um 16:22 schrieb Nikolay Sivov:
Yes, this looks much better. Now to make it shorter please combine this tests with a fix into a single patch - this way you won't need exception handling noise.
This current testbot run does at least succeed on (my local) wine and windows for both issues, the null description and the invalid handle. But I somehow I doubt that wine wants to handle all access violations as invalid handle? (At least OpenService crashes also on an invalid handle, so probably other functions will do too.) Also, as you said, validation could just be done on server side, I assume that would require far more work? So should that invalid handle part wait until a real application relies on this? https://testbot.winehq.org/JobDetails.pl?Key=12409 Kind regards, Bernhard
participants (3)
-
Bernhard Übelacker -
Nikolay Sivov -
Nikolay Sivov