[PATCH] advapi32: succeed for null pointer in ChangeServiceConfig2

[Nikolay Sivov]

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
>