[PATCH] ntdll: Fix find_free_area outside of reserved areas with zero_bits != 0.

[Rémi Bernon]

From: Paul Gofman <gofmanp(a)gmail.com>

The search was initiated with base == 0, which returns NULL immediately
if MEM_TOP_DOWN is not used. Using address_space_start instead fixes
this issue.

Then we assumed that all mmapped regions are known by Wine view tree,
which is obviously not the case with external allocations. This could
lead to memory corruption when find_free_area returns an expected free
region which is already mmapped. Using MAP_FIXED forces mmap to succeed
and corrupts the mapping.

This patch uses safe mmap calls to check if the expected free memory is
actually free, and iterates over the expected free ranges until it
succeeds.

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
Based on a patch from Paul Gofman <gofmanp(a)gmail.com>

 dlls/ntdll/virtual.c | 60 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 50 insertions(+), 10 deletions(-)

diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c
index 68249de902e..bf8dab0e7f3 100644
--- a/dlls/ntdll/virtual.c
+++ b/dlls/ntdll/virtual.c
@@ -496,6 +496,33 @@ static struct file_view *find_view_range( const void *addr, size_t size )
     return NULL;
 }

+/***********************************************************************
+ *           try_map_free_area
+ *
+ * Try mmaping some expected free memory region, eventually stepping and
+ * retrying inside it, and return where it actually succeeded, or NULL.
+ */
+static void* try_map_free_area( void *base, void *end, ptrdiff_t step,
+                                void *start, size_t size, int unix_prot )
+{
+    void *ptr;
+
+    while (start && base <= start && (char*)start + size <= (char*)end)
+    {
+        if ((ptr = wine_anon_mmap( start, size, unix_prot, 0 )) == start)
+            return start;
+        TRACE( "Found free area is already mapped, start %p.\n", start );
+
+        if (ptr != (void *)-1)
+            munmap( ptr, size );
+
+        if ((char *)end - (char *)start < step || (char *)start - (char *)base < step)
+            break;
+        start = (char *)start + step;
+    }
+
+    return NULL;
+}

 /***********************************************************************
  *           find_free_area
@@ -503,9 +530,11 @@ static struct file_view *find_view_range( const void *addr, size_t size )
  * Find a free area between views inside the specified range.
  * The csVirtual section must be held by caller.
  */
-static void *find_free_area( void *base, void *end, size_t size, size_t mask, int top_down )
+static void *find_free_area( void *base, void *end, size_t size, size_t mask, int top_down,
+                             int map_area, int unix_prot )
 {
     struct wine_rb_entry *first = NULL, *ptr = views_tree.root;
+    ptrdiff_t step = top_down ? -(mask + 1) : (mask + 1);
     void *start;

     /* find the first (resp. last) view inside the range */
@@ -538,7 +567,10 @@ static void *find_free_area( void *base, void *end, size_t size, size_t mask, in
         {
             struct file_view *view = WINE_RB_ENTRY_VALUE( first, struct file_view, entry );

-            if ((char *)view->base + view->size <= (char *)start) break;
+            if (!map_area && (char *)view->base + view->size <= (char *)start) break;
+            if (map_area && (start = try_map_free_area( (char *)view->base + view->size,
+                                                        (char *)start + size, step,
+                                                        start, size, unix_prot ))) break;
             start = ROUND_ADDR( (char *)view->base - size, mask );
             /* stop if remaining space is not large enough */
             if (!start || start >= end || start < base) return NULL;
@@ -554,13 +586,17 @@ static void *find_free_area( void *base, void *end, size_t size, size_t mask, in
         {
             struct file_view *view = WINE_RB_ENTRY_VALUE( first, struct file_view, entry );

-            if ((char *)view->base >= (char *)start + size) break;
+            if (!map_area && (char *)view->base >= (char *)start + size) break;
+            if (map_area && (start = try_map_free_area( start, view->base, step,
+                                                        start, size, unix_prot ))) break;
             start = ROUND_ADDR( (char *)view->base + view->size + mask, mask );
             /* stop if remaining space is not large enough */
             if (!start || start >= end || (char *)end - (char *)start < size) return NULL;
             first = wine_rb_next( first );
         }
     }
+
+    if (!first && map_area) return try_map_free_area( base, end, step, start, size, unix_prot );
     return start;
 }

@@ -1054,13 +1090,13 @@ static int alloc_reserved_area_callback( void *start, size_t size, void *arg )
         {
             /* range is split in two by the preloader reservation, try first part */
             if ((alloc->result = find_free_area( start, preload_reserve_start, alloc->size,
-                                                 alloc->mask, alloc->top_down )))
+                                                 alloc->mask, alloc->top_down, FALSE, 0 )))
                 return 1;
             /* then fall through to try second part */
             start = preload_reserve_end;
         }
     }
-    if ((alloc->result = find_free_area( start, end, alloc->size, alloc->mask, alloc->top_down )))
+    if ((alloc->result = find_free_area( start, end, alloc->size, alloc->mask, alloc->top_down, FALSE, 0 )))
         return 1;

     return 0;
@@ -1163,14 +1199,18 @@ static NTSTATUS map_view( struct file_view **view_ret, void *base, size_t size,
             goto done;
         }

-        for (;;)
+        if (zero_bits_64)
         {
-            if (!zero_bits_64)
-                ptr = NULL;
-            else if (!(ptr = find_free_area( (void*)0, alloc.limit, view_size, mask, top_down )))
+            if (!(ptr = find_free_area( address_space_start, alloc.limit, size,
+                                        mask, top_down, TRUE, VIRTUAL_GetUnixProt(vprot) )))
                 return STATUS_NO_MEMORY;
+            TRACE( "got mem with find_free_area %p-%p\n", ptr, (char *)ptr + size );
+            goto done;
+        }

-            if ((ptr = wine_anon_mmap( ptr, view_size, VIRTUAL_GetUnixProt(vprot), ptr ? MAP_FIXED : 0 )) == (void *)-1)
+        for (;;)
+        {
+            if ((ptr = wine_anon_mmap( NULL, view_size, VIRTUAL_GetUnixProt(vprot), 0 )) == (void *)-1)
             {
                 if (errno == ENOMEM) return STATUS_NO_MEMORY;
                 return STATUS_INVALID_PARAMETER;
--
2.24.0.rc2

[Rémi Bernon]

On 11/4/19 4:15 PM, Rémi Bernon wrote:
> +
> +        if ((char *)end - (char *)start < step || (char *)start - (char *)base < step)
> +            break;

I should probably check the sign of step and choose one of the two 
conditions here instead. Trying to do everything at once like this stops 
the iterations when base == start and step > 0 for instance. Or it 
should be base - start > step. I'll fix that.
-- 
Rémi Bernon <rbernon(a)codeweavers.com>