Re: [PATCH v7 3/6] ntdll/tests: Add NtAllocateVirtualMemory tests for zero_bits behavior

[Huw Davies]

On Fri, May 31, 2019 at 12:02:00PM +0200, Rémi Bernon wrote:
> The zero_bits parameter doesn't behave as expected, and some 64bit code
> use it to allocate memory in the lower 32bit address space.
>
> The expected full behaviour is:
> - zero_bits == 0: no constraint on address range
> - 0 < zero_bits <= 15: returned address should have as many upper bits
>                      set to 0, starting at bit 31. In 64bit mode,
>                      upper 32bits should all be 0 as well.
> - 15 < zero_bits <= 31: unsure, but probably same as zero_bits == 15.
> - zero_bits > 31: (64bit/WoW64 only) zero_bits behaves as a bitmask, as
>                 if it was set to the number of leading 0 in the
>                 bitmask, works in the whole 64bit range.
> Signed-off-by: Rémi Bernon
>
> dlls/ntdll/tests/virtual.c | 67 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>
> diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
> index 0434c29af31..25cd36bd028 100644
> --- a/dlls/ntdll/tests/virtual.c
> +++ b/dlls/ntdll/tests/virtual.c
> @@ -11,11 +11,15 @@
>  #include "excpt.h"
>  #include "wine/test.h"
>
> +static BOOL (WINAPI *pIsWow64Process)(HANDLE, PBOOL);
> -
>
> static void test_AllocateVirtualMemory(void)
>  {
>      void *addr1, *addr2;
>      NTSTATUS status;
>      SIZE_T size;
> -
>
> ULONG zero_bits;
> -
>
> BOOL is_wow64;
>
> /* simple allocation should success */
> size = 0x1000;
>
> @@ -45,6 +49,40 @@ static void test_AllocateVirtualMemory(void)
>          ok(status == STATUS_SUCCESS, "NtFreeVirtualMemory return %08x, addr2: %p\n", status, addr2);
>      }
> - /* 1 zero bits should zero 63-31 upper bits */
> - size = 0x1000;
> - addr2 = NULL;
> - zero_bits = 1;
> - status = NtAllocateVirtualMemory(NtCurrentProcess(), &addr2, 1, &size,
> -
>                                 MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE);
> - ok(((status == STATUS_SUCCESS || status == STATUS_NO_MEMORY) && ((UINT_PTR)addr2 >> (32 - zero_bits)) == 0) ||
> -
>   broken(status == STATUS_INVALID_PARAMETER_3) /* winxp */,
> -
>   "NtAllocateVirtualMemory returned %08x, addr2: %p\n", status, addr2);

It would be clearer to move the addr2 >> (32 - zero_bits) == 0 test to
a separate ok(), probably inside the if block below.  This also
applies to several other places further down.  The point being that
the main purpose of these tests is to test the form of address
returned and that test is currently hidden inside a complicated
expression which looks to have more to do with the precise value of
the returned status.
> - if (status == STATUS_SUCCESS)
> - {
> -
>    size = 0;
> -
>    status = NtFreeVirtualMemory(NtCurrentProcess(), &addr2, &size, MEM_RELEASE);
> -
>    ok(status == STATUS_SUCCESS, "NtFreeVirtualMemory return %08x, addr2: %p\n", status, addr2);
> - }