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

[Zhiyi Zhang]

On 4/23/19 9:31 PM, Vijay Kiran Kamuju wrote:
> From: Qian Hong <qhong at codeweavers.com>
>
> Signed-off-by: Qian Hong <qhong at codeweavers.com>
> Signed-off-by: Vijay Kiran Kamuju <infyquest at gmail.com>
> ---
>  dlls/advapi32/tests/security.c | 8 --------
>  dlls/ntdll/sec.c               | 6 ++++++
>  2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
> index d9cae64da8b..d886ab713f3 100644
> --- a/dlls/advapi32/tests/security.c
> +++ b/dlls/advapi32/tests/security.c
> @@ -1454,10 +1454,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\n", PrivSetLen);
>      ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed,
>         "Access and/or AccessStatus were changed!\n");
> @@ -1508,12 +1506,9 @@ 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(PrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", PrivSetLen);
> -todo_wine
>      ok(Access == 0x1abe11ed && AccessStatus == 0x1abe11ed,
>         "Access and/or AccessStatus were changed!\n");
>  
> @@ -1625,12 +1620,9 @@ 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(PrivSetLen == sizeof(PRIVILEGE_SET), "PrivSetLen returns %d\n", PrivSetLen);
> -    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 02fc77dc1cc..f7f1a925770 100644
> --- a/dlls/ntdll/sec.c
> +++ b/dlls/ntdll/sec.c
> @@ -1670,6 +1670,12 @@ NtAccessCheck(
>      if (!PrivilegeSet || !ReturnLength)
>          return STATUS_ACCESS_VIOLATION;
>  
> +    if (*ReturnLength == 0)
I think testing *ReturnLength < sizeof(PRIVILEGE_SET) is more appropriate. You should add some tests to verify its
behavior, e.g., When *ReturnLength = sizeof(PRIVILEGE_SET)-1

> +    {
> +        *ReturnLength = sizeof(PRIVILEGE_SET);
> +        return STATUS_BUFFER_TOO_SMALL;
> +    }
> +
>      SERVER_START_REQ( access_check )
>      {
>          struct security_descriptor sd;