Re: [PATCH v4 2/6] kernel32/tests: Add NtAllocateVirtualMemory tests for zero_bits behavior

[Rémi Bernon]

On Wed, 2019-05-29 at 10:42 +0100, Huw Davies wrote:
> On Tue, May 28, 2019 at 12:15:14PM +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 ==
> > -
> > - 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/kernel32/tests/virtual.c | 57
> > ++++++++++++++++++++++++++++++++---
> >  1 file changed, 52 insertions(+), 5 deletions(-)
> >
> > diff --git a/dlls/kernel32/tests/virtual.c
> > b/dlls/kernel32/tests/virtual.c
> > index dcaeb147b04..1778509d2ac 100644
> > --- a/dlls/kernel32/tests/virtual.c
> > +++ b/dlls/kernel32/tests/virtual.c
> > @@ -228,10 +228,12 @@ static void test_VirtualAllocEx(void)
> >  static void test_VirtualAlloc(void)
> >  {
> >      void *addr1, *addr2;
> > -
> >
> > ULONG zero_bits;
> > DWORD old_prot;
> > MEMORY_BASIC_INFORMATION info;
> > NTSTATUS status;
> > SIZE_T size;
> > -
> >
> > BOOL is_wow64;
> >
> > SetLastError(0xdeadbeef);
> > addr1 = VirtualAlloc(0, 0, MEM_RESERVE, PAGE_NOACCESS);
> >
> > @@ -461,13 +463,16 @@ static void test_VirtualAlloc(void)
> >          ok(status == STATUS_SUCCESS, "pNtFreeVirtualMemory return
> > %08x, addr2: %p\n", status, addr2);
> >      }
> > - /* 21 zero bits is valid */
> > - /* 1 zero bits should zero 63-31 upper bits */
> > size = 0x1000;
> > addr2 = NULL;
> > - status = pNtAllocateVirtualMemory(GetCurrentProcess(), &addr2,
> >
> > 21, &size,
> > -
> >                                  MEM_RESERVE | MEM_COMMIT,
> >
> > PAGE_EXECUTE_READWRITE);
> > - ok(status == STATUS_SUCCESS || status == STATUS_NO_MEMORY,
> > -
> >   "NtAllocateVirtualMemory returned %08x\n", status);
> > - zero_bits = 1;
> > - status = pNtAllocateVirtualMemory(GetCurrentProcess(), &addr2,
> >
> > 1, &size,
> > -
> >                                  MEM_RESERVE | MEM_COMMIT,
> >
> > PAGE_READWRITE);
> > - todo_wine
> > - ok((status == STATUS_SUCCESS || broken(status ==
> >
> > STATUS_INVALID_PARAMETER_3) /* winxp */) &&
> > -
> >   ((UINT_PTR)addr2 >> (32 - zero_bits)) == 0,
> > -
> >   "NtAllocateVirtualMemory returned %08x, addr2: %p\n",
> >
> > status, addr2);
> >      if (status == STATUS_SUCCESS)
> >      {
> >          size = 0;
> > @@ -475,6 +480,23 @@ static void test_VirtualAlloc(void)
> >          ok(status == STATUS_SUCCESS, "pNtFreeVirtualMemory return
> > %08x, addr2: %p\n", status, addr2);
> >      }
> > - for (zero_bits = 2; zero_bits <= 21; zero_bits++)
> > - {
> > -
> >    size = 0x1000;
> > -
> >    addr2 = NULL;
> > -
> >    status = pNtAllocateVirtualMemory(GetCurrentProcess(),
> >
> > &addr2, zero_bits, &size,
> > -
> >                                      MEM_RESERVE |
> >
> > MEM_COMMIT, PAGE_READWRITE);
> > -
> >    todo_wine
> > -
> >    ok((status == STATUS_SUCCESS || status ==
> >
> > STATUS_NO_MEMORY) && ((UINT_PTR)addr2 >> (32 - zero_bits)) == 0,
> > -
> >       "NtAllocateVirtualMemory with %d zero_bits returned
> >
> > %08x, addr2: %p\n", zero_bits, status, addr2);
> > -
> >    if (status == STATUS_SUCCESS)
> > -
> >    {
> > -
> >        size = 0;
> > -
> >        status = pNtFreeVirtualMemory(GetCurrentProcess(),
> >
> > &addr2, &size, MEM_RELEASE);
> > -
> >        ok(status == STATUS_SUCCESS, "pNtFreeVirtualMemory
> >
> > return %08x, addr2: %p\n", status, addr2);
> > -
> >    }
> > - }
> > -
>
> Problem is sometimes these actually work (and so fail the todo_wine):
>
> virtual.c:473: Test succeeded inside todo block:
> NtAllocateVirtualMemory returned 00000000, addr2: 00222000
> virtual.c:490: Test succeeded inside todo block:
> NtAllocateVirtualMemory with 2 zero_bits returned 00000000, addr2:
> 00222000
> virtual.c:490: Test succeeded inside todo block:
> NtAllocateVirtualMemory with 3 zero_bits returned 00000000, addr2:
> 00222000
> virtual.c:490: Test succeeded inside todo block:
> NtAllocateVirtualMemory with 4 zero_bits returned 00000000, addr2:
> 00222000
> virtual.c:490: Test succeeded inside todo block:
> NtAllocateVirtualMemory with 5 zero_bits returned 00000000, addr2:
> 00222000
> virtual.c:490: Test succeeded inside todo block:
> NtAllocateVirtualMemory with 6 zero_bits returned 00000000, addr2:
> 00222000
> virtual.c:490: Test succeeded inside todo block:
> NtAllocateVirtualMemory with 7 zero_bits returned 00000000, addr2:
> 00222000
> virtual.c:490: Test succeeded inside todo block:
> NtAllocateVirtualMemory with 8 zero_bits returned 00000000, addr2:
> 00222000
> virtual.c:490: Test succeeded inside todo block:
> NtAllocateVirtualMemory with 9 zero_bits returned 00000000, addr2:
> 00222000
> virtual.c:490: Test succeeded inside todo block:
> NtAllocateVirtualMemory with 10 zero_bits returned 00000000, addr2:
> 00222000

They work but actually shouldn't, and the later changes will "fix"
this. Should I add the todo_wine in the patch that breaks them?
> Also having these tests in kernel32 seems wrong; though short of
> adding a new virtual.c to the ntdll tests, I couldn't find a sensible
> place to put them.
>
> Huw.

It was my initial intuition, but there wasn't any ntdll tests for these
functions, whereas there were already some in kernel32. I can maybe
move all the related tests to ntdll instead.

Rémi