Re: [PATCH v6 1/7] ntdll/tests: Move NtAllocateVirtualMemory tests that were in kernel32

[Huw Davies]

On Wed, May 29, 2019 at 03:13:07PM +0200, Rémi Bernon wrote:
> Signed-off-by: Rémi Bernon
>
> dlls/kernel32/tests/virtual.c | 47 -------------------
>  dlls/ntdll/tests/Makefile.in  |  3 +-
>  dlls/ntdll/tests/virtual.c    | 88 +++++++++++++++++++++++++++++++++++
>  3 files changed, 90 insertions(+), 48 deletions(-)
>  create mode 100644 dlls/ntdll/tests/virtual.c

About the first few patches in this series in general.  I would
re-order them.  First fix the tests in kernel32 (so put patch 2 first).
Then move them, but combine this patch with patch 3.  Then you can
add new tests.

Some more comments below.
> diff --git a/dlls/ntdll/tests/virtual.c b/dlls/ntdll/tests/virtual.c
> new file mode 100644
> index 00000000000..63300ac9460
> --- /dev/null
> +++ b/dlls/ntdll/tests/virtual.c
> @@ -0,0 +1,88 @@
> +#include <stdio.h>
> -
>
> +#include "ntstatus.h"
> +#define WIN32_NO_STATUS
> +#include "windef.h"
> +#include "winbase.h"
> +#include "winnt.h"
> +#include "winternl.h"
> +#include "winerror.h"
> +#include "winuser.h"
> +#include "excpt.h"
> +#include "wine/test.h"
> -
>
> +static NTSTATUS (WINAPI *pNtAllocateVirtualMemory)(HANDLE, PVOID *, ULONG, SIZE_T *, ULONG, ULONG);
> +static NTSTATUS (WINAPI *pNtFreeVirtualMemory)(HANDLE, PVOID *, SIZE_T *, ULONG);

I don't see any reason to dynamically load these fn ptrs, you can just
call the functions directly.
> +static void test_AllocateVirtualMemory(void)
> +{
> - void *addr1, *addr2;
> - NTSTATUS status;
> - SIZE_T size;
> -
> - /* simple allocation should success */
> - size = 0x1000;
> - addr1 = NULL;
> - status = pNtAllocateVirtualMemory(GetCurrentProcess(), &addr1, 0, &size,
> -
>                                  MEM_RESERVE | MEM_COMMIT, PAGE_EXECUTE_READWRITE);
> - ok(status == STATUS_SUCCESS, "NtAllocateVirtualMemory returned %08x\n", status);
> -
> - /* allocation conflicts because of 64k align */
> - size = 0x1000;
> - addr2 = (char *)addr1 + 0x1000;
> - status = pNtAllocateVirtualMemory(GetCurrentProcess(), &addr2, 0, &size,
> -
>                                  MEM_RESERVE | MEM_COMMIT, PAGE_EXECUTE_READWRITE);
> - ok(status == STATUS_CONFLICTING_ADDRESSES, "NtAllocateVirtualMemory returned %08x\n", status);
> -
> - /* it should conflict, even when zero_bits is explicitly set */
> - size = 0x1000;
> - addr2 = (char *)addr1 + 0x1000;
> - status = pNtAllocateVirtualMemory(GetCurrentProcess(), &addr2, 12, &size,
> -
>                                  MEM_RESERVE | MEM_COMMIT, PAGE_EXECUTE_READWRITE);
> - todo_wine
> - ok(status == STATUS_CONFLICTING_ADDRESSES, "NtAllocateVirtualMemory returned %08x\n", status);
> - if (status == STATUS_SUCCESS) ok(VirtualFree(addr2, 0, MEM_RELEASE), "VirtualFree failed\n");
> -
> - /* 21 zero bits is valid */
> - 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);

This allocation always fails on Windows see:

https://testbot.winehq.org/JobDetails.pl?Key=53086

So as part of the test fixing before you move them to ntdll, I'd
remove the STATUS_SUCCESS and you'll probably have to add a todo_wine
at the same time.
> - if (status == STATUS_SUCCESS) ok(VirtualFree(addr2, 0, MEM_RELEASE), "VirtualFree failed\n");

As mentioned above, this will now call NtVirtualFreeMemory()
> -
> - /* 22 zero bits is invalid */
> - size = 0x1000;
> - addr2 = NULL;
> - status = pNtAllocateVirtualMemory(GetCurrentProcess(), &addr2, 22, &size,
> -
>                                  MEM_RESERVE | MEM_COMMIT, PAGE_EXECUTE_READWRITE);
> - ok(status == STATUS_INVALID_PARAMETER_3, "NtAllocateVirtualMemory returned %08x\n", status);
> - if (status == STATUS_SUCCESS) ok(VirtualFree(addr2, 0, MEM_RELEASE), "VirtualFree failed\n");
> -
> - /* AT_ROUND_TO_PAGE flag is not supported for NtAllocateVirtualMemory */
> - size = 0x1000;
> - addr2 = (char *)addr1 + 0x1000;
> - status = pNtAllocateVirtualMemory(GetCurrentProcess(), &addr2, 0, &size, MEM_RESERVE |
> -
>                                  MEM_COMMIT | AT_ROUND_TO_PAGE, PAGE_EXECUTE_READWRITE);
> - todo_wine
> - ok(status == STATUS_INVALID_PARAMETER_5, "NtAllocateVirtualMemory returned %08x\n", status);
> -
> - ok(VirtualFree(addr1, 0, MEM_RELEASE), "VirtualFree failed\n");
>
> +}
> -
>
> +START_TEST(virtual)
> +{
> - SYSTEM_INFO si;
> - HMODULE hntdll;
> -
> - hntdll = GetModuleHandleA("ntdll.dll");
> -
> - pNtAllocateVirtualMemory = (void *)GetProcAddress( hntdll, "NtAllocateVirtualMemory" );
> - pNtFreeVirtualMemory = (void *)GetProcAddress( hntdll, "NtFreeVirtualMemory" );
> -
> - GetSystemInfo(&si);

I'd probably use NtQuerySystemInformation( SystemCpuInformation, ...) instead.

Huw.