[PATCH 0/4] MR9752: ntdll: Compute total size as aligned commit size in RtlCreateHeap().
From: Paul Gofman <pgofman@codeweavers.com> Those crash even on 32 bit on current Win11. --- dlls/kernel32/tests/heap.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 30c80e57985..6722bc6af28 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -1546,7 +1546,7 @@ static void test_GlobalAlloc(void) ok( !!mem, "GlobalAlloc failed, error %lu\n", GetLastError() ); tmp_mem = pGlobalFree( mem ); ok( !tmp_mem, "GlobalFree failed, error %lu\n", GetLastError() ); - if (sizeof(void *) != 8) /* crashes on 64-bit */ + if (0) /* crashes on Windows */ { SetLastError( 0xdeadbeef ); tmp_mem = pGlobalFree( mem ); @@ -1589,7 +1589,7 @@ static void test_GlobalAlloc(void) tmp_mem = GlobalReAlloc( mem, 0, GMEM_MOVEABLE ); ok( !tmp_mem, "GlobalReAlloc succeeded\n" ); ok( GetLastError() == ERROR_INVALID_HANDLE, "got error %lu\n", GetLastError() ); - if (sizeof(void *) != 8) /* crashes on 64-bit */ + if (0) /* crashes on Windows */ { SetLastError( 0xdeadbeef ); tmp_mem = GlobalHandle( mem ); @@ -1623,7 +1623,7 @@ static void test_GlobalAlloc(void) tmp_mem = GlobalReAlloc( invalid_mem, 0, GMEM_MOVEABLE ); ok( !tmp_mem, "GlobalReAlloc succeeded\n" ); ok( GetLastError() == ERROR_INVALID_HANDLE, "got error %lu\n", GetLastError() ); - if (sizeof(void *) != 8) /* crashes on 64-bit */ + if (0) /* crashes on Windows */ { SetLastError( 0xdeadbeef ); tmp_mem = GlobalHandle( invalid_mem ); @@ -2336,7 +2336,7 @@ static void test_LocalAlloc(void) tmp_mem = LocalReAlloc( mem, 0, LMEM_MOVEABLE ); ok( !tmp_mem, "LocalReAlloc succeeded\n" ); ok( GetLastError() == ERROR_INVALID_HANDLE, "got error %lu\n", GetLastError() ); - if (sizeof(void *) != 8) /* crashes on 64-bit */ + if (0) /* crashes on Windows */ { SetLastError( 0xdeadbeef ); tmp_mem = LocalHandle( mem ); @@ -2369,7 +2369,7 @@ static void test_LocalAlloc(void) tmp_mem = LocalReAlloc( invalid_mem, 0, LMEM_MOVEABLE ); ok( !tmp_mem, "LocalReAlloc succeeded\n" ); ok( GetLastError() == ERROR_INVALID_HANDLE, "got error %lu\n", GetLastError() ); - if (sizeof(void *) != 8) /* crashes on 64-bit */ + if (0) /* crashes on Windows */ { SetLastError( 0xdeadbeef ); tmp_mem = LocalHandle( invalid_mem ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9752
From: Paul Gofman <pgofman@codeweavers.com> --- dlls/kernel32/tests/heap.c | 75 +++++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 18 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 6722bc6af28..4795c60e9ea 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -38,6 +38,8 @@ #define HEAP_VALIDATE_ALL 0x20000000 #define HEAP_VALIDATE_PARAMS 0x40000000 +#define REGION_ALIGN 0x10000 +#define INITIAL_COMMIT_ALIGN (0x400 * sizeof(void *)) #define BLOCK_ALIGN (2 * sizeof(void *) - 1) #define ALIGN_BLOCK_SIZE(x) (((x) + BLOCK_ALIGN) & ~BLOCK_ALIGN) @@ -573,8 +575,7 @@ static void test_HeapCreate(void) ok( entries[0].cbData <= 0x1000 /* sizeof(*heap) */, "got cbData %#lx\n", entries[0].cbData ); ok( entries[0].cbOverhead == 0, "got cbOverhead %#x\n", entries[0].cbOverhead ); ok( entries[0].iRegionIndex == 0, "got iRegionIndex %d\n", entries[0].iRegionIndex ); - todo_wine - ok( entries[0].Region.dwCommittedSize == 0x400 * sizeof(void *), + todo_wine ok( entries[0].Region.dwCommittedSize == 0x400 * sizeof(void *), "got Region.dwCommittedSize %#lx\n", entries[0].Region.dwCommittedSize ); ok( entries[0].Region.dwUnCommittedSize == 0x10000 - entries[0].Region.dwCommittedSize || entries[0].Region.dwUnCommittedSize == 0x10000 * sizeof(void *) - entries[0].Region.dwCommittedSize /* win7 */, @@ -749,6 +750,9 @@ static void test_HeapCreate(void) while ((ret = HeapWalk( heap, &entry ))) entries[count++] = entry; ok( GetLastError() == ERROR_NO_MORE_ITEMS, "got error %lu\n", GetLastError() ); ok( count == 4, "got count %lu\n", count ); + todo_wine ok( entries->Region.dwCommittedSize == INITIAL_COMMIT_ALIGN, "got %#lx.\n", entries->Region.dwCommittedSize ); + ok( entries->Region.dwUnCommittedSize == REGION_ALIGN - entries->Region.dwCommittedSize, "got %#lx.\n", + entries->Region.dwUnCommittedSize ); ok( !memcmp( entries + 16, entries, 1 * sizeof(entry) ), "entries differ\n" ); ok( memcmp( entries + 17, entries + 2, 2 * sizeof(entry) ), "entries differ\n" ); @@ -3685,12 +3689,13 @@ static void test_GlobalMemoryStatus(void) #undef IS_WITHIN_RANGE } -static void get_valloc_info( void *mem, char **base, SIZE_T *alloc_size ) +static void get_valloc_info( void *mem, char **base, SIZE_T *alloc_size, SIZE_T *commit_size ) { MEMORY_BASIC_INFORMATION info, info2; SIZE_T size; char *p; + *commit_size = 0; size = VirtualQuery( mem, &info, sizeof(info) ); ok( size == sizeof(info), "got %Iu.\n", size ); @@ -3703,6 +3708,7 @@ static void get_valloc_info( void *mem, char **base, SIZE_T *alloc_size ) if (info2.AllocationBase != info.AllocationBase) break; ok( info2.State == MEM_RESERVE || info2.State == MEM_COMMIT, "got %#lx.\n", info2.State ); + if (info2.State == MEM_COMMIT) *commit_size += info2.RegionSize; p += info2.RegionSize; } @@ -3710,32 +3716,41 @@ static void get_valloc_info( void *mem, char **base, SIZE_T *alloc_size ) *alloc_size = p - *base; } -static void test_heap_size( SIZE_T initial_size ) +static void test_heap_size( SIZE_T initial_commit_size ) { - static const SIZE_T default_heap_size = 0x10000, init_grow_size = 0x100000, max_grow_size = 0xfd0000; + static const SIZE_T init_grow_size = 0x100000, max_grow_size = 0xfd0000, test_alloc_size = 0x60000; BOOL initial_subheap = TRUE, max_size_reached = FALSE; - SIZE_T alloc_size, current_subheap_size; + SIZE_T alloc_size, committed_size, current_subheap_size, expected, commit_size, initial_committed_size; char *base, *current_base; + BOOL subheap_changed; unsigned int i; HANDLE heap; void *p; - winetest_push_context( "init size %#Ix", initial_size ); - heap = HeapCreate( HEAP_NO_SERIALIZE, initial_size, 0 ); - get_valloc_info( heap, ¤t_base, &alloc_size ); + winetest_push_context( "init size %#Ix", initial_commit_size ); + heap = HeapCreate( HEAP_NO_SERIALIZE, initial_commit_size, 0 ); + get_valloc_info( heap, ¤t_base, &alloc_size, &committed_size ); + + commit_size = ROUND_SIZE( initial_commit_size, INITIAL_COMMIT_ALIGN - 1 ); + expected = ROUND_SIZE( commit_size + 1, REGION_ALIGN - 1 ); - ok( alloc_size == initial_size + default_heap_size || broken( (initial_size && alloc_size == initial_size) - || (!initial_size && (alloc_size == default_heap_size * sizeof(void*))) ) /* Win7 */, - "got %#Ix.\n", alloc_size ); + todo_wine_if( expected != initial_commit_size + REGION_ALIGN + && !(initial_commit_size > REGION_ALIGN - INITIAL_COMMIT_ALIGN && initial_commit_size < REGION_ALIGN)) + ok( alloc_size == expected, "got %#Ix, expected %#Ix.\n", alloc_size, expected ); + expected = max( commit_size, INITIAL_COMMIT_ALIGN ); + todo_wine_if( (!initial_commit_size || (initial_commit_size & (REGION_ALIGN - 1))) + && !(initial_commit_size > REGION_ALIGN - INITIAL_COMMIT_ALIGN && initial_commit_size < REGION_ALIGN)) + ok( committed_size == expected, "got commit size %#Ix, expected %#Ix.\n", committed_size, expected ); current_subheap_size = alloc_size; + initial_committed_size = committed_size; + commit_size = 0; for (i = 0; i < 100; ++i) { - winetest_push_context( "i %u, current_subheap_size %#Ix", i, current_subheap_size ); - p = HeapAlloc( heap, 0, 0x60000 ); - get_valloc_info( p, &base, &alloc_size ); - if (base != current_base) + p = HeapAlloc( heap, 0, test_alloc_size ); + get_valloc_info( p, &base, &alloc_size, &committed_size ); + if ((subheap_changed = (base != current_base))) { current_base = base; if (initial_subheap) @@ -3749,8 +3764,18 @@ static void test_heap_size( SIZE_T initial_size ) if (current_subheap_size == max_grow_size) max_size_reached = TRUE; } + commit_size = 0x1000; + initial_committed_size = 0; } + + winetest_push_context( "i %u, current_subheap_size %#Ix, subheap_changed %d", i, current_subheap_size, + subheap_changed ); + commit_size += test_alloc_size + 0x1000; ok( alloc_size == current_subheap_size, "got %#Ix.\n", alloc_size ); + expected = max( initial_committed_size, commit_size ); + todo_wine_if(commit_size != 0x5b0000 && expected != initial_commit_size) + ok( committed_size == expected, "got %#Ix, expected %#Ix, commit_size %#Ix, initial_committed_size %#Ix. p %p.\n", + committed_size, expected, commit_size, initial_committed_size, p ); winetest_pop_context(); } ok( max_size_reached, "Did not reach maximum subheap size.\n" ); @@ -3762,10 +3787,24 @@ static void test_heap_size( SIZE_T initial_size ) static void test_heap_sizes(void) { unsigned int i; - SIZE_T size, round_size = 0x400 * sizeof(void*); + SIZE_T size, commit_size, round_size = 0x400 * sizeof(void*); char *base; test_heap_size( 0 ); + test_heap_size( 1 ); + test_heap_size( 0x100 ); + test_heap_size( 0xf00 ); + test_heap_size( 0xfff ); + test_heap_size( 0x1000 ); + test_heap_size( 0xe000 ); + test_heap_size( 0xe001 ); + test_heap_size( 0xf000 ); + test_heap_size( 0xf001 ); + test_heap_size( 0xffff ); + test_heap_size( 0x10000 ); + test_heap_size( 0x20fff ); + test_heap_size( 0x30000 ); + test_heap_size( 0x30001 ); test_heap_size( 0x80000 ); test_heap_size( 0x150000 ); @@ -3773,7 +3812,7 @@ static void test_heap_sizes(void) { HANDLE heap = HeapCreate( 0, i * 0x100, i * 0x100 ); ok( heap != NULL, "%x: creation failed\n", i * 0x100 ); - get_valloc_info( heap, &base, &size ); + get_valloc_info( heap, &base, &size, &commit_size ); ok( size == ((i * 0x100 + round_size - 1) & ~(round_size - 1)), "%x: wrong size %Ix\n", i * 0x100, size ); HeapDestroy( heap ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9752
From: Paul Gofman <pgofman@codeweavers.com> --- dlls/ntdll/heap.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index c68c449a524..7bf68d52245 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -80,6 +80,7 @@ struct rtl_heap_entry /* header for heap blocks */ +#define INITIAL_COMMIT_ALIGN (0x400 * sizeof(void *)) #define REGION_ALIGN 0x10000 #define BLOCK_ALIGN (2 * sizeof(void *)) @@ -1511,6 +1512,7 @@ HANDLE WINAPI RtlCreateHeap( ULONG flags, void *addr, SIZE_T total_size, SIZE_T flags &= ~(HEAP_TAIL_CHECKING_ENABLED|HEAP_FREE_CHECKING_ENABLED); if (process_heap) flags |= HEAP_PRIVATE; if (!process_heap || !total_size || (flags & HEAP_SHARED)) flags |= HEAP_GROWABLE; + commit_size = ROUND_SIZE( commit_size, INITIAL_COMMIT_ALIGN - 1 ); if (!total_size) total_size = commit_size + HEAP_INITIAL_SIZE; if (!(heap = addr)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9752
From: Paul Gofman <pgofman@codeweavers.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=56286 --- dlls/kernel32/tests/heap.c | 2 -- dlls/ntdll/heap.c | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 4795c60e9ea..6fe26b1837a 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -3735,8 +3735,6 @@ static void test_heap_size( SIZE_T initial_commit_size ) commit_size = ROUND_SIZE( initial_commit_size, INITIAL_COMMIT_ALIGN - 1 ); expected = ROUND_SIZE( commit_size + 1, REGION_ALIGN - 1 ); - todo_wine_if( expected != initial_commit_size + REGION_ALIGN - && !(initial_commit_size > REGION_ALIGN - INITIAL_COMMIT_ALIGN && initial_commit_size < REGION_ALIGN)) ok( alloc_size == expected, "got %#Ix, expected %#Ix.\n", alloc_size, expected ); expected = max( commit_size, INITIAL_COMMIT_ALIGN ); todo_wine_if( (!initial_commit_size || (initial_commit_size & (REGION_ALIGN - 1))) diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 7bf68d52245..b40b2d2ace8 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -1513,7 +1513,7 @@ HANDLE WINAPI RtlCreateHeap( ULONG flags, void *addr, SIZE_T total_size, SIZE_T if (process_heap) flags |= HEAP_PRIVATE; if (!process_heap || !total_size || (flags & HEAP_SHARED)) flags |= HEAP_GROWABLE; commit_size = ROUND_SIZE( commit_size, INITIAL_COMMIT_ALIGN - 1 ); - if (!total_size) total_size = commit_size + HEAP_INITIAL_SIZE; + if (!total_size) total_size = ROUND_SIZE( commit_size + 1 /* + 1 is intentional */, REGION_ALIGN - 1 ); if (!(heap = addr)) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9752
The game from the bug is doing (rather far away) out of bound read access. However, it is lucky to hit accessible memory on Windows and used to on Wine before commit 354a8bb1f4a65bdec052606f2799db9e2907b5b1. The app creates some heaps with initial commit size of 0x1000 which after the blamed commit results in total heap sizes (and growing heap sizes) not aligned to 64k which introduces memory fragmentation with uncommitted holes, which is making the crash in the game (and possibly others doing out of bound access) more likely. More detailed testing shows that total heap size should always be 64 aligned. The added tests (as well as one existing todo) also cover committed sizes which are not matching Windows now. I prepared the patches which fix that part also (https://gitlab.winehq.org/gofman/wine/-/commits/ntdll_heap_sizes) but I think those are not safe during the code freeze. "ntdll: Round commit size to page size instead of allocation size." looks rather obvious, but it reduces the committed size and thus begs for introducing new regressions under similar pattern. The commit after that looks weird, but it matches the tests which show that Windows actually commits more than needed (so it is better to have both of those patches or none; now we usually commit more because of excessive commit size aligment to 64k). I'd send this remaining part after the code freeze. Both this MR and that fuller one fix the crash in game here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9752#note_125405
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9752
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon (＠rbernon)