[PATCH resend v3 1/2] ntdll: Try mapping free memory region outside of reserved regions.
We assumed that all mapped 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 mapped. 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> --- dlls/ntdll/virtual.c | 64 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 11 deletions(-) diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c index cb869feff02..a29517e286d 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; @@ -1148,6 +1186,7 @@ static NTSTATUS map_view( struct file_view **view_ret, void *base, size_t size, { size_t view_size = size + mask + 1; struct alloc_area alloc; + int unix_prot = VIRTUAL_GetUnixProt(vprot); alloc.size = size; alloc.mask = mask; @@ -1158,19 +1197,22 @@ static NTSTATUS map_view( struct file_view **view_ret, void *base, size_t size, { ptr = alloc.result; TRACE( "got mem in reserved area %p-%p\n", ptr, (char *)ptr + size ); - if (wine_anon_mmap( ptr, size, VIRTUAL_GetUnixProt(vprot), MAP_FIXED ) != ptr) + if (wine_anon_mmap( ptr, size, unix_prot, MAP_FIXED ) != ptr) return STATUS_INVALID_PARAMETER; 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( (void*)0, alloc.limit, size, mask, top_down, TRUE, unix_prot ))) 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, unix_prot, 0 )) == (void *)-1) { if (errno == ENOMEM) return STATUS_NO_MEMORY; return STATUS_INVALID_PARAMETER; -- 2.24.1
The search was initiated with base == 0, which returns NULL immediately if MEM_TOP_DOWN is not used. Use address_space_start instead. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47974 Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- The bug is also about some other issue with X Rebirth but these patches should fix the main crash cause. dlls/ntdll/virtual.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c index a29517e286d..985a8f5c89e 100644 --- a/dlls/ntdll/virtual.c +++ b/dlls/ntdll/virtual.c @@ -1204,7 +1204,7 @@ static NTSTATUS map_view( struct file_view **view_ret, void *base, size_t size, if (zero_bits_64) { - if (!(ptr = find_free_area( (void*)0, alloc.limit, size, mask, top_down, TRUE, unix_prot ))) + if (!(ptr = find_free_area( address_space_start, alloc.limit, size, mask, top_down, TRUE, unix_prot ))) return STATUS_NO_MEMORY; TRACE( "got mem with find_free_area %p-%p\n", ptr, (char *)ptr + size ); goto done; -- 2.24.1
Rémi Bernon <rbernon(a)codeweavers.com> writes:
We assumed that all mapped 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 mapped. Using MAP_FIXED forces mmap to succeed and corrupts the mapping.
I have a feeling that this would be cleaner with a separate function, particularly since the algorithm to find free space in the system areas could be made smarter. -- Alexandre Julliard julliard(a)winehq.org
On 12/24/19 1:27 PM, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
We assumed that all mapped 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 mapped. Using MAP_FIXED forces mmap to succeed and corrupts the mapping.
I have a feeling that this would be cleaner with a separate function, particularly since the algorithm to find free space in the system areas could be made smarter.
I'm not sure about what you mean by "smarter". The original patches this whole zero bits thing is based upon were trying semi-random addresses to try to find free system areas, with a timeout [1], but I'm not completely convinced that it's better especially with the top_down flag to implement. It could also read /proc/self/maps, making it not portable, I guess. Or maybe there's a way to enumerate mapped memory, including non-Wine, that I'm missing? Or you just mean the search within an expected free area could do a bisection instead of iterating linearly? [1] https://www.winehq.org/pipermail/wine-devel/attachments/20190302/f96c118f/at... -- Rémi Bernon <rbernon(a)codeweavers.com>
Rémi Bernon <rbernon(a)codeweavers.com> writes:
On 12/24/19 1:27 PM, Alexandre Julliard wrote:
Rémi Bernon <rbernon(a)codeweavers.com> writes:
We assumed that all mapped 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 mapped. Using MAP_FIXED forces mmap to succeed and corrupts the mapping.
I have a feeling that this would be cleaner with a separate function, particularly since the algorithm to find free space in the system areas could be made smarter.
I'm not sure about what you mean by "smarter".
The original patches this whole zero bits thing is based upon were trying semi-random addresses to try to find free system areas, with a timeout [1], but I'm not completely convinced that it's better especially with the top_down flag to implement.
It could also read /proc/self/maps, making it not portable, I guess. Or maybe there's a way to enumerate mapped memory, including non-Wine, that I'm missing?
Or you just mean the search within an expected free area could do a bisection instead of iterating linearly?
Some kind of combination of these ideas, yes. But this should wait until after code freeze of course. -- Alexandre Julliard julliard(a)winehq.org
On 12/24/19 16:17, Rémi Bernon wrote:
On 12/24/19 1:27 PM, Alexandre Julliard wrote:
The original patches this whole zero bits thing is based upon were trying semi-random addresses to try to find free system areas, with a timeout [1], but I'm not completely convinced that it's better especially with the top_down flag to implement.
It could also read /proc/self/maps, making it not portable, I guess. Or maybe there's a way to enumerate mapped memory, including non-Wine, that I'm missing?
Just to mention, there is also an mincore() system call which I suppose should be portable which can provide a preliminary guess if the range of pages is free. It should be much less expensive than an attempt to map a memory at least because the attempt always succeeds allocating the memory outside of the region which is then freed in case memory is busy. This is still not transactional of course (native code in separate threads can get the memory right after checking it availability either way).
Or you just mean the search within an expected free area could do a bisection instead of iterating linearly?
There are some applications which depend on (virtual) memory allocations not returning high addresses while the lower memory is available. Windows allocates virtual memory bottom up unless top down is explicitly requested. I made a patch [1] some time ago which does that the same way zero_bits allocation works (that depends on this fix or equivalent). Unless such sort of change is not desirable for some reason it would be great if the modified fix for zero_bits support would keep the ability to follow bottom up / top down allocation order. 1. https://github.com/wine-staging/wine-staging/blob/master/patches/ntdll-Force...
On 12/24/19 3:23 PM, Paul Gofman wrote:
On 12/24/19 16:17, Rémi Bernon wrote:
On 12/24/19 1:27 PM, Alexandre Julliard wrote:
The original patches this whole zero bits thing is based upon were trying semi-random addresses to try to find free system areas, with a timeout [1], but I'm not completely convinced that it's better especially with the top_down flag to implement.
It could also read /proc/self/maps, making it not portable, I guess. Or maybe there's a way to enumerate mapped memory, including non-Wine, that I'm missing?
Just to mention, there is also an mincore() system call which I suppose should be portable which can provide a preliminary guess if the range of pages is free. It should be much less expensive than an attempt to map a memory at least because the attempt always succeeds allocating the memory outside of the region which is then freed in case memory is busy. This is still not transactional of course (native code in separate threads can get the memory right after checking it availability either way).
Alright, I didn't know that, thanks.
Or you just mean the search within an expected free area could do a bisection instead of iterating linearly?
There are some applications which depend on (virtual) memory allocations not returning high addresses while the lower memory is available. Windows allocates virtual memory bottom up unless top down is explicitly requested. I made a patch [1] some time ago which does that the same way zero_bits allocation works (that depends on this fix or equivalent). Unless such sort of change is not desirable for some reason it would be great if the modified fix for zero_bits support would keep the ability to follow bottom up / top down allocation order.
Sure, and by bisection I rather meant that -assuming the requested size is greater than the alignment- you can probably start iterating with a bigger step to have a coarse estimate before trying to find a more precise location. -- Rémi Bernon <rbernon(a)codeweavers.com>
participants (3)
-
Alexandre Julliard -
Paul Gofman -
Rémi Bernon