[PATCH v2 0/4] MR151: ntdll: Improve heap block size compat.
-- v2: ntdll: Improve block size rounding compatibility. ntdll: Remove unnecessary constants and typedefs. ntdll: Remove tail checking on large blocks. ntdll: Use a fixed block tail size. https://gitlab.winehq.org/wine/wine/-/merge_requests/151
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/heap.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 16abc158e55..2b390bfed7d 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -1176,7 +1176,7 @@ static BOOL validate_used_block( const SUBHEAP *subheap, const struct block *blo else if (!err && (flags & HEAP_TAIL_CHECKING_ENABLED)) { const unsigned char *tail = (unsigned char *)block + block_get_size( block ) - block->unused_bytes; - for (i = 0; !err && i < block->unused_bytes; i++) if (tail[i] != ARENA_TAIL_FILLER) err = "invalid block tail"; + for (i = 0; !err && i < ALIGNMENT; i++) if (tail[i] != ARENA_TAIL_FILLER) err = "invalid block tail"; } if (err) @@ -1343,7 +1343,7 @@ static void heap_set_debug_flags( HANDLE handle ) else { if (block_get_type( block ) == ARENA_PENDING_MAGIC) mark_block_free( block + 1, block_get_size( block ) - sizeof(*block), flags ); - else mark_block_tail( (char *)block + block_get_size( block ) - block->unused_bytes, block->unused_bytes, flags ); + else mark_block_tail( (char *)block + block_get_size( block ) - block->unused_bytes, ALIGNMENT, flags ); } } } @@ -1628,8 +1628,8 @@ static NTSTATUS heap_reallocate( HEAP *heap, ULONG flags, void *ptr, SIZE_T size /* Clear the extra bytes if needed */ - if (size <= old_size) mark_block_tail( (char *)(block + 1) + size, block->unused_bytes, flags ); - else initialize_block( (char *)(block + 1) + old_size, size - old_size, block->unused_bytes, flags ); + if (size <= old_size) mark_block_tail( (char *)(block + 1) + size, ALIGNMENT, flags ); + else initialize_block( (char *)(block + 1) + old_size, size - old_size, ALIGNMENT, flags ); /* Return the new arena */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/151
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/kernel32/tests/heap.c | 7 +++---- dlls/ntdll/heap.c | 9 +-------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 8ef0bfc3668..795d758a4f2 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -2242,7 +2242,6 @@ static void test_block_layout( HANDLE heap, DWORD global_flags, DWORD heap_flags ok( diff > expect_size, "got diff %#Ix\n", diff ); tail = ptr0[alloc_size] | ptr1[alloc_size] | ptr2[alloc_size]; - todo_wine_if( heap_flags & HEAP_TAIL_CHECKING_ENABLED ) ok( !tail, "got tail\n" ); ret = HeapFree( heap, 0, ptr2 ); @@ -2492,9 +2491,9 @@ static void test_heap_checks( DWORD flags ) if (flags & HEAP_TAIL_CHECKING_ENABLED) { /* Windows doesn't do tail checking on large blocks */ - ok( p[large_size] == 0xab || broken(p[large_size] == 0), "wrong data %x\n", p[large_size] ); - ok( p[large_size+1] == 0xab || broken(p[large_size+1] == 0), "wrong data %x\n", p[large_size+1] ); - ok( p[large_size+2] == 0xab || broken(p[large_size+2] == 0), "wrong data %x\n", p[large_size+2] ); + ok( p[large_size] == 0, "wrong data %x\n", p[large_size] ); + ok( p[large_size+1] == 0, "wrong data %x\n", p[large_size+1] ); + ok( p[large_size+2] == 0, "wrong data %x\n", p[large_size+2] ); if (p[large_size] == 0xab) { p[large_size] = 0xcc; diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 2b390bfed7d..f8c0eba89e0 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -785,7 +785,7 @@ static inline void shrink_used_block( SUBHEAP *subheap, struct block *block, UIN static void *allocate_large_block( HEAP *heap, DWORD flags, SIZE_T size ) { ARENA_LARGE *arena; - SIZE_T block_size = sizeof(*arena) + ROUND_SIZE(size) + HEAP_TAIL_EXTRA_SIZE(flags); + SIZE_T block_size = sizeof(*arena) + ROUND_SIZE(size); LPVOID address = NULL; if (!(flags & HEAP_GROWABLE)) return NULL; @@ -801,7 +801,6 @@ static void *allocate_large_block( HEAP *heap, DWORD flags, SIZE_T size ) arena->block_size = block_size; arena->size = ARENA_LARGE_SIZE; arena->magic = ARENA_LARGE_MAGIC; - mark_block_tail( (char *)(arena + 1) + size, block_size - sizeof(*arena) - size, flags ); list_add_tail( &heap->large_list, &arena->entry ); notify_alloc( arena + 1, size, flags & HEAP_ZERO_MEMORY ); return arena + 1; @@ -883,12 +882,6 @@ static BOOL validate_large_arena( const HEAP *heap, const ARENA_LARGE *arena ) err = "invalid block header"; else if (!contains( arena, arena->block_size, arena + 1, arena->data_size )) err = "invalid block size"; - else if (heap->flags & HEAP_TAIL_CHECKING_ENABLED) - { - SIZE_T i, unused = arena->block_size - sizeof(*arena) - arena->data_size; - const unsigned char *data = (const unsigned char *)(arena + 1) + arena->data_size; - for (i = 0; i < unused && !err; i++) if (data[i] != ARENA_TAIL_FILLER) err = "invalid block tail"; - } if (err) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/151
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/heap.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index f8c0eba89e0..e33fa0c76b3 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -76,12 +76,12 @@ struct rtl_heap_entry #define ALIGNMENT (2 * sizeof(void *)) -typedef struct block +struct block { DWORD size; /* Block size; must be the first field */ DWORD magic : 24; /* Magic number */ DWORD unused_bytes : 8; /* Number of bytes in the block not used by user data (max value is HEAP_MIN_DATA_SIZE+HEAP_MIN_SHRINK_SIZE) */ -} ARENA_INUSE; +}; C_ASSERT( sizeof(struct block) == 8 ); @@ -127,7 +127,7 @@ typedef struct /* everything is aligned on 8 byte boundaries (16 for Win64) */ #define LARGE_ALIGNMENT 16 /* large blocks have stricter alignment */ -#define ARENA_OFFSET (ALIGNMENT - sizeof(ARENA_INUSE)) +#define ARENA_OFFSET (ALIGNMENT - sizeof(struct block)) #define COMMIT_MASK 0xffff /* bitmask for commit/decommit granularity */ C_ASSERT( sizeof(ARENA_LARGE) % LARGE_ALIGNMENT == 0 ); @@ -138,7 +138,7 @@ C_ASSERT( sizeof(ARENA_LARGE) % LARGE_ALIGNMENT == 0 ); /* minimum data size (without arenas) of an allocated block */ /* make sure that it's larger than a free list entry */ #define HEAP_MIN_DATA_SIZE ROUND_SIZE(2 * sizeof(struct list)) -#define HEAP_MIN_BLOCK_SIZE (HEAP_MIN_DATA_SIZE + sizeof(ARENA_INUSE)) +#define HEAP_MIN_BLOCK_SIZE (HEAP_MIN_DATA_SIZE + sizeof(struct block)) /* minimum size that must remain to shrink an allocated block */ #define HEAP_MIN_SHRINK_SIZE (HEAP_MIN_DATA_SIZE+sizeof(struct entry)) /* minimum size to start allocating large blocks */ @@ -195,7 +195,7 @@ typedef struct tagHEAP SIZE_T min_size; /* Minimum committed size */ DWORD magic; /* Magic number */ DWORD pending_pos; /* Position in pending free requests ring */ - ARENA_INUSE **pending_free; /* Ring buffer for pending free requests */ + struct block **pending_free; /* Ring buffer for pending free requests */ RTL_CRITICAL_SECTION cs; struct entry free_lists[HEAP_NB_FREE_LISTS]; SUBHEAP subheap; @@ -1529,7 +1529,7 @@ void *WINAPI DECLSPEC_HOTPATCH RtlAllocateHeap( HANDLE heap, ULONG flags, SIZE_T static NTSTATUS heap_free( HEAP *heap, void *ptr ) { - ARENA_INUSE *block; + struct block *block; SUBHEAP *subheap; /* Inform valgrind we are trying to free memory, so it can throw up an error message */ @@ -1723,7 +1723,7 @@ BOOLEAN WINAPI RtlUnlockHeap( HANDLE heap ) static NTSTATUS heap_size( HEAP *heap, const void *ptr, SIZE_T *size ) { - const ARENA_INUSE *block; + const struct block *block; SUBHEAP *subheap; if (!(block = unsafe_block_from_ptr( heap, ptr, &subheap ))) return STATUS_INVALID_PARAMETER; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/151
From: Rémi Bernon <rbernon(a)codeweavers.com> This also increase the default heap size to 2MiB (32bit) / 4MiB (64bit), for the tests to pass. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/kernel32/tests/heap.c | 6 +---- dlls/ntdll/heap.c | 46 +++++++++++++++++++++++++------------- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 795d758a4f2..193b168def2 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -1055,7 +1055,6 @@ static void test_HeapCreate(void) { if (entries[i].wFlags != PROCESS_HEAP_ENTRY_BUSY) continue; ok( entries[i].cbData == 0x18 + 2 * sizeof(void *), "got cbData %#lx\n", entries[i].cbData ); - todo_wine_if(sizeof(void *) == 8) ok( entries[i].cbOverhead == 0x8, "got cbOverhead %#x\n", entries[i].cbOverhead ); } @@ -2175,10 +2174,7 @@ static void test_block_layout( HANDLE heap, DWORD global_flags, DWORD heap_flags expect_size = max( alloc_size, 2 * sizeof(void *) ); expect_size = ALIGN_BLOCK_SIZE( expect_size + extra_size ); diff = min( llabs( ptr2 - ptr1 ), llabs( ptr1 - ptr0 ) ); - todo_wine_if( (heap_flags & (HEAP_VALIDATE_PARAMS|HEAP_VALIDATE_ALL)) || - ((global_flags & ~FLG_HEAP_ENABLE_TAIL_CHECK) && alloc_size < 0x10000) || - (!global_flags && alloc_size < 2 * sizeof(void *)) || - (alloc_size == 0xfd000 && sizeof(void *) == 8) ) + todo_wine_if( (!(global_flags & ~FLG_HEAP_ENABLE_FREE_CHECK) && alloc_size < 2 * sizeof(void *)) ) ok( diff == expect_size, "got diff %#Ix exp %#Ix\n", diff, expect_size ); ok( !memcmp( ptr0 + alloc_size, tail_buf, tail_size ), "missing block tail\n" ); diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index e33fa0c76b3..ad2b589ee0f 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -127,20 +127,18 @@ typedef struct /* everything is aligned on 8 byte boundaries (16 for Win64) */ #define LARGE_ALIGNMENT 16 /* large blocks have stricter alignment */ -#define ARENA_OFFSET (ALIGNMENT - sizeof(struct block)) #define COMMIT_MASK 0xffff /* bitmask for commit/decommit granularity */ C_ASSERT( sizeof(ARENA_LARGE) % LARGE_ALIGNMENT == 0 ); #define ROUND_ADDR(addr, mask) ((void *)((UINT_PTR)(addr) & ~(UINT_PTR)(mask))) -#define ROUND_SIZE(size) ((((size) + ALIGNMENT - 1) & ~(ALIGNMENT-1)) + ARENA_OFFSET) - -/* minimum data size (without arenas) of an allocated block */ -/* make sure that it's larger than a free list entry */ -#define HEAP_MIN_DATA_SIZE ROUND_SIZE(2 * sizeof(struct list)) -#define HEAP_MIN_BLOCK_SIZE (HEAP_MIN_DATA_SIZE + sizeof(struct block)) -/* minimum size that must remain to shrink an allocated block */ -#define HEAP_MIN_SHRINK_SIZE (HEAP_MIN_DATA_SIZE+sizeof(struct entry)) +#define ROUND_SIZE(size, mask) ((((SIZE_T)(size) + (mask)) & ~(SIZE_T)(mask))) + +#define HEAP_MIN_BLOCK_SIZE ROUND_SIZE(sizeof(struct entry) + ALIGNMENT, ALIGNMENT - 1) + +C_ASSERT( sizeof(struct block) <= HEAP_MIN_BLOCK_SIZE ); +C_ASSERT( sizeof(struct entry) <= HEAP_MIN_BLOCK_SIZE ); + /* minimum size to start allocating large blocks */ #define HEAP_MIN_LARGE_BLOCK_SIZE (0x10000 * ALIGNMENT - 0x1000) /* extra size to add at the end of block for tail checking */ @@ -208,7 +206,7 @@ C_ASSERT( offsetof(HEAP, subheap) <= COMMIT_MASK ); #define HEAP_MAGIC ((DWORD)('H' | ('E'<<8) | ('A'<<16) | ('P'<<24))) -#define HEAP_DEF_SIZE 0x110000 /* Default heap size = 1Mb + 64Kb */ +#define HEAP_DEF_SIZE (0x40000 * ALIGNMENT) #define MAX_FREE_PENDING 1024 /* max number of free requests to delay */ /* some undocumented flags (names are made up) */ @@ -217,6 +215,7 @@ C_ASSERT( offsetof(HEAP, subheap) <= COMMIT_MASK ); #define HEAP_VALIDATE 0x10000000 #define HEAP_VALIDATE_ALL 0x20000000 #define HEAP_VALIDATE_PARAMS 0x40000000 +#define HEAP_CHECKING_ENABLED 0x80000000 static HEAP *processHeap; /* main process heap */ @@ -450,7 +449,8 @@ static RTL_CRITICAL_SECTION_DEBUG process_heap_cs_debug = static inline ULONG heap_get_flags( const HEAP *heap, ULONG flags ) { - flags &= HEAP_GENERATE_EXCEPTIONS | HEAP_NO_SERIALIZE | HEAP_ZERO_MEMORY | HEAP_REALLOC_IN_PLACE_ONLY; + if (flags & (HEAP_TAIL_CHECKING_ENABLED | HEAP_FREE_CHECKING_ENABLED)) flags |= HEAP_CHECKING_ENABLED; + flags &= HEAP_GENERATE_EXCEPTIONS | HEAP_NO_SERIALIZE | HEAP_ZERO_MEMORY | HEAP_REALLOC_IN_PLACE_ONLY | HEAP_CHECKING_ENABLED; return heap->flags | flags; } @@ -763,7 +763,7 @@ static void free_used_block( SUBHEAP *subheap, struct block *block ) static inline void shrink_used_block( SUBHEAP *subheap, struct block *block, UINT flags, SIZE_T old_block_size, SIZE_T block_size, SIZE_T size ) { - if (old_block_size >= block_size + HEAP_MIN_SHRINK_SIZE) + if (old_block_size >= block_size + HEAP_MIN_BLOCK_SIZE) { block_set_size( block, flags, block_size ); block->unused_bytes = block_size - sizeof(*block) - size; @@ -785,7 +785,7 @@ static inline void shrink_used_block( SUBHEAP *subheap, struct block *block, UIN static void *allocate_large_block( HEAP *heap, DWORD flags, SIZE_T size ) { ARENA_LARGE *arena; - SIZE_T block_size = sizeof(*arena) + ROUND_SIZE(size); + SIZE_T block_size = ROUND_SIZE( sizeof(*arena) + size, COMMIT_MASK ); LPVOID address = NULL; if (!(flags & HEAP_GROWABLE)) return NULL; @@ -1471,13 +1471,29 @@ HANDLE WINAPI RtlDestroyHeap( HANDLE heap ) return 0; } +static SIZE_T heap_get_block_size( HEAP *heap, ULONG flags, SIZE_T size ) +{ + static const ULONG padd_flags = HEAP_VALIDATE | HEAP_VALIDATE_ALL | HEAP_VALIDATE_PARAMS; + static const ULONG check_flags = HEAP_TAIL_CHECKING_ENABLED | HEAP_FREE_CHECKING_ENABLED | HEAP_CHECKING_ENABLED; + SIZE_T overhead; + + if ((flags & check_flags)) overhead = ALIGNMENT; + else overhead = sizeof(struct block); + + if ((flags & HEAP_TAIL_CHECKING_ENABLED) || RUNNING_ON_VALGRIND) overhead += ALIGNMENT; + if (flags & padd_flags) overhead += ALIGNMENT; + + if (size < ALIGNMENT) size = ALIGNMENT; + return ROUND_SIZE( size + overhead, ALIGNMENT - 1 ); +} + static NTSTATUS heap_allocate( HEAP *heap, ULONG flags, SIZE_T size, void **ret ) { SIZE_T old_block_size, block_size; struct block *block; SUBHEAP *subheap; - block_size = sizeof(*block) + ROUND_SIZE(size) + HEAP_TAIL_EXTRA_SIZE(flags); + block_size = heap_get_block_size( heap, flags, size ); if (block_size < size) return STATUS_NO_MEMORY; /* overflow */ if (block_size < HEAP_MIN_BLOCK_SIZE) block_size = HEAP_MIN_BLOCK_SIZE; @@ -1574,7 +1590,7 @@ static NTSTATUS heap_reallocate( HEAP *heap, ULONG flags, void *ptr, SIZE_T size SUBHEAP *subheap; NTSTATUS status; - block_size = sizeof(*block) + ROUND_SIZE(size) + HEAP_TAIL_EXTRA_SIZE(flags); + block_size = heap_get_block_size( heap, flags, size ); if (block_size < size) return STATUS_NO_MEMORY; /* overflow */ if (block_size < HEAP_MIN_BLOCK_SIZE) block_size = HEAP_MIN_BLOCK_SIZE; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/151
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=115770 Your paranoid android. === debian11 (64 bit WoW report) === ntdll: env.c:461: Test failed: wrong end ptr 000000000085195C/0000000000851960
Actually I think the tail changes should be done differently, splitting valgrind support from native tail checking. Closing for now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/151#note_1374
This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/151
participants (2)
-
Marvin -
Rémi Bernon