[PATCH 2/2] ntdll: Improve invalid paramater handling in NtAccessCheck.

[Sebastian Lackner]

On 15.02.2016 21:53, Qian Hong wrote:
> 
> Signed-off-by: Qian Hong <qhong at codeweavers.com>
> ---
>  dlls/advapi32/tests/security.c |  4 ----
>  dlls/ntdll/sec.c               | 11 ++++++++++-
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> 
> 
> 0002-ntdll-Improve-invalid-paramater-handling-in-NtAccessCh.txt
> 
> 
> diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
> index 69c0f77..08666dd 100644
> --- a/dlls/advapi32/tests/security.c
> +++ b/dlls/advapi32/tests/security.c
> @@ -1429,10 +1429,8 @@ static void test_AccessCheck(void)
>      ret = AccessCheck(SecurityDescriptor, Token, KEY_READ, &Mapping,
>                        0, &PrivSetLen, &Access, &AccessStatus);
>      err = GetLastError();
> -todo_wine
>      ok(!ret && err == ERROR_INSUFFICIENT_BUFFER, "AccessCheck should have "
>         "failed with ERROR_INSUFFICIENT_BUFFER, instead of %d\n", err);
> -todo_wine
>      ok(PrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d, expects %d\n", PrivSetLen, sizeof(PRIVILEGE_SET));
>      ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed,
>         "Access and/or AccessStatus were changed!\n");
> @@ -1444,10 +1442,8 @@ todo_wine
>      ret = AccessCheck(SecurityDescriptor, Token, KEY_READ, &Mapping,
>                        PrivSet, &PrivSetLen, &Access, &AccessStatus);
>      err = GetLastError();
> -todo_wine
>      ok(!ret && err == ERROR_INSUFFICIENT_BUFFER, "AccessCheck should have "
>         "failed with ERROR_INSUFFICIENT_BUFFER, instead of %d\n", err);
> -todo_wine
>      ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed,
>         "Access and/or AccessStatus were changed!\n");
>  
> diff --git a/dlls/ntdll/sec.c b/dlls/ntdll/sec.c
> index 125c86e..c32ae0c 100644
> --- a/dlls/ntdll/sec.c
> +++ b/dlls/ntdll/sec.c
> @@ -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);
> +        return STATUS_BUFFER_TOO_SMALL;
> +    }

This looks a bit hacky. The code below assumes that *ReturnLength > FIELD_OFFSET( PRIVILEGE_SET, Privilege ),
so it would be interesting to know what happens for sizes 0 ... 8.

> +
> +    if (!PrivilegeSet)
>          return STATUS_ACCESS_VIOLATION;
>  
>      SERVER_START_REQ( access_check )
> 
> 
>