Re: [PATCH 2/3] ntdll: Improve invalid parameter handling in NtAccessCheck. (try 2)

[Alexandre Julliard]

Qian Hong  writes:
> @@ -1586,7 +1586,16 @@ NtAccessCheck(
>          SecurityDescriptor, ClientToken, DesiredAccess, GenericMapping,
>          PrivilegeSet, ReturnLength, GrantedAccess, AccessStatus);
> - if (!PrivilegeSet || !ReturnLength)
> - if (!ReturnLength)
> -
>    return STATUS_ACCESS_VIOLATION;
> -
> - if (*ReturnLength == 0)
> - {
> -
>    *ReturnLength = sizeof(PRIVILEGE_SET);

Shouldn't this depend on the number of privileges to be returned?

[PATCH 2/3] ntdll: Improve invalid parameter handling in NtAccessCheck. (try 2)

[Qian Hong]

On Wed, Feb 17, 2016 at 11:02 PM, Alexandre Julliard
 wrote:
> Shouldn't this depend on the number of privileges to be returned?

Thanks for review!

Yes. In theory this should depend on the number of privileges to be returned.
In practice, Windows requires a minimal value of
sizeof(PRIVILEGE_SET) (size equal to 20) not matter if PrivilegeCount
is 0 (size equal to 8) or PrivilegeCount is 1 (size equal to 20).
I can't find a test case where Windows returns a PrivilegeCount larger than 1.
Wine's current implementation also does not return any value larger than 1.

I agree that it is not  safe and maintainable to hardcode like this,
so I changed my implementation to MAX( 20, real_size_needed ), this
will match Windows' minimal requirement, also runs safely on Wine.

The real world app I want to fix relies on the exact last error code
when ReturnLength and PrivSet are both 0/NULL, there are also some
other programmers rely on this way to fetch required length according
to MSDN comments.

I didn't sent the NtAccessCheck version of test this time, my patch v2
show that NtAccessCheck behaviors the same to AccessCheck, I'll resent
that patch after current patch committed in order to avoid further
changing and rebasing.

Thanks!