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@codeweavers.com --- Based on a patch from Paul Gofman gofmanp@gmail.com v2: Fix try_map_free_area step condition.
dlls/ntdll/virtual.c | 62 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 10 deletions(-)
diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c index 68249de902e..b2c17f2c005 100644 --- a/dlls/ntdll/virtual.c +++ b/dlls/ntdll/virtual.c @@ -496,6 +496,35 @@ 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 ((step > 0 && (char *)end - (char *)start < step) || + (step < 0 && (char *)start - (char *)base < -step) || + step == 0) + break; + start = (char *)start + step; + } + + return NULL; +}
/*********************************************************************** * find_free_area @@ -503,9 +532,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 +569,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 +588,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 +1092,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 +1201,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
On Mon, Nov 04, 2019 at 04:51:56PM +0100, Rémi Bernon wrote:
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.
So it seems this patch is trying to fix two bugs. You should be able to split this so that a first patch addresses the !top_down issue and a second patch addresses the assumption about mmapped regions being known.
Huw.
On 11/20/19 11:57 AM, Huw Davies wrote:
On Mon, Nov 04, 2019 at 04:51:56PM +0100, Rémi Bernon wrote:
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.
So it seems this patch is trying to fix two bugs. You should be able to split this so that a first patch addresses the !top_down issue and a second patch addresses the assumption about mmapped regions being known.
Huw.
Yeah I had the discussion with Paul (he reported the bug and wrote the original patch), and he argued that fixing the !top_down issue first would create a regression as it would potentially then return an already used memory region, and the other way around would be modifying code that is not executed first. But I guess there's always the top_down case.
So I think it could be fixing the search first, then the !top_down case.
-
Huw Davies -
Rémi Bernon