[PATCH v4 4/6] ntdll: Clarify NtAllocateVirtualMemory zero_bits parameter semantics

[Huw Davies]

On Tue, May 28, 2019 at 12:15:16PM +0200, Rémi Bernon wrote:
> This parameter was misinterpreted as an alignment parameter for the
> lower bits of the allocated memory region, although it is a constraint
> on the higher bits.
> 
> Add a new internal ntdll virtual_alloc function that has a separate
> alignment parameter which is now used instead of the zero_bits
> parameter.
> 
> Replace external calls to NtAllocateVirtualMemory with VirtualAlloc.
> 
> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
> ---
>  dlls/commdlg.dll16/filedlg.c |  3 +--
>  dlls/ntdll/directory.c       |  4 ++--
>  dlls/ntdll/heap.c            |  5 ++---
>  dlls/ntdll/ntdll_misc.h      |  2 ++
>  dlls/ntdll/signal_arm.c      | 12 +++++------
>  dlls/ntdll/signal_arm64.c    | 16 +++++++--------
>  dlls/ntdll/signal_i386.c     | 16 +++++++--------
>  dlls/ntdll/signal_powerpc.c  | 12 +++++------
>  dlls/ntdll/signal_x86_64.c   | 16 +++++++--------
>  dlls/ntdll/thread.c          |  3 +--
>  dlls/ntdll/virtual.c         | 40 +++++++++++++++++++++++++++---------
>  11 files changed, 74 insertions(+), 55 deletions(-)
> 
> diff --git a/dlls/commdlg.dll16/filedlg.c b/dlls/commdlg.dll16/filedlg.c
> index 5b72bfab100..050cddb0dd5 100644
> --- a/dlls/commdlg.dll16/filedlg.c
> +++ b/dlls/commdlg.dll16/filedlg.c
> @@ -509,8 +509,7 @@ static LPOFNHOOKPROC alloc_hook( LPOFNHOOKPROC16 hook16 )
>      SIZE_T size = 0x1000;
>      unsigned int i;
>  
> -    if (!hooks && NtAllocateVirtualMemory( GetCurrentProcess(), (void **)&hooks, 12, &size,
> -                                           MEM_COMMIT, PAGE_EXECUTE_READWRITE ))
> +    if (!hooks && !(hooks = VirtualAlloc( NULL, size, MEM_COMMIT, PAGE_EXECUTE_READWRITE )))
>          return NULL;
>  
>      for (i = 0; i < count; i++)

This hunk should be a separate patch.

> diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c
> index 78973a8cda4..90475a01f21 100644
> --- a/dlls/ntdll/virtual.c
> +++ b/dlls/ntdll/virtual.c
> @@ -2461,19 +2461,18 @@ void virtual_set_large_address_space(void)
>  NTSTATUS WINAPI NtAllocateVirtualMemory( HANDLE process, PVOID *ret, ULONG zero_bits,
>                                           SIZE_T *size_ptr, ULONG type, ULONG protect )
>  {
> -    void *base;
> -    unsigned int vprot;
> -    SIZE_T size = *size_ptr;
> -    SIZE_T mask = get_mask( zero_bits );
>      NTSTATUS status = STATUS_SUCCESS;
> -    BOOL is_dos_memory = FALSE;
> -    struct file_view *view;
> -    sigset_t sigset;
>  
> -    TRACE("%p %p %08lx %x %08x\n", process, *ret, size, type, protect );
> +    if (!size_ptr) return STATUS_ACCESS_VIOLATION;

As already mentioned, please don't add this check unless it's needed and
let's keep the original TRACE().

>  
> -    if (!size) return STATUS_INVALID_PARAMETER;
> -    if (!mask) return STATUS_INVALID_PARAMETER_3;
> +    TRACE("%p %p %08lx %x %08x\n", process, *ret, *size_ptr, type, protect );
> +
> +    if (!*size_ptr) return STATUS_INVALID_PARAMETER;
> +    if (zero_bits)
> +    {
> +        FIXME("Unimplemented zero_bits handling\n");
> +        return STATUS_INVALID_PARAMETER_3;
> +    }
>  
>      if (process != NtCurrentProcess())
>      {
> @@ -2499,6 +2498,27 @@ NTSTATUS WINAPI NtAllocateVirtualMemory( HANDLE process, PVOID *ret, ULONG zero_
>          return result.virtual_alloc.status;
>      }
>  
> +    return virtual_alloc( ret, zero_bits, size_ptr, type, protect, 0 );
> +}
> +
> +
> +/***********************************************************************
> + *             virtual_alloc   (NTDLL.@)
> + *
> + * Same as NtAllocateVirtualMemory but with an alignment parameter
> + */
> +NTSTATUS virtual_alloc( PVOID *ret, ULONG zero_bits, SIZE_T *size_ptr,
> +                        ULONG type, ULONG protect, ULONG alignment )

How about calling it virtual_alloc_aligned() ?

> +{
> +    void *base;
> +    unsigned int vprot;
> +    SIZE_T size = *size_ptr;
> +    SIZE_T mask = get_mask( alignment );
> +    NTSTATUS status = STATUS_SUCCESS;
> +    BOOL is_dos_memory = FALSE;
> +    struct file_view *view;
> +    sigset_t sigset;
> +
>      /* Round parameters to a page boundary */
>  
>      if (is_beyond_limit( 0, size, working_set_limit )) return STATUS_WORKING_SET_LIMIT_RANGE;

Huw.