From: Paul Gofman pgofman@codeweavers.com
--- dlls/kernel32/tests/heap.c | 41 ++++++++++++++++++++++++++++++++++++++ dlls/ntdll/heap.c | 6 ++++-- 2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 59a82c0c579..a1d72302098 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -3270,8 +3270,49 @@ static void test_heap_checks( DWORD flags ) ret = HeapFree( GetProcessHeap(), 0, p ); ok( ret, "HeapFree failed\n" );
+ if (flags & HEAP_FREE_CHECKING_ENABLED) + { + void *p_prev; + SIZE_T count; + + p = pHeapAlloc( GetProcessHeap(), 0, 39 ); + ok( !!p, "HeapAlloc failed\n" ); + for (i = 0; i < 39 / sizeof(int); ++i) + ok( ((unsigned int *)p)[i] == 0xbaadf00d, "got %#x, i %Iu.\n", ((unsigned int *)p)[i], i ); + memset( p, 0xcc, 39 ); + + p_prev = p; + count = 5; + p = pHeapReAlloc( GetProcessHeap(), 0, p, 39 + count * 4 ); + ok( !!p, "got NULL.\n" ); + + winetest_push_context( "in place %d", p == p_prev); + if (p == p_prev) + { + if (flags & HEAP_TAIL_CHECKING_ENABLED) + ok( p[39] == 0xab, "got %#x.\n", p[39] ); + for (i = 0; i < count - 1; ++i) + ok( ((unsigned int *)p)[10 + i] == 0xbaadf00d, "got %#x, i %Iu.\n", ((unsigned int *)p)[10 + i], i ); + } + else + { + ok( p[39] == 0xba, "got %#x.\n", p[39] ); + for (i = 0; i < count - 1; ++i) + ok( ((unsigned int *)p)[10 + i] == 0xbaadf00d, "got %#x, i %Iu.\n", ((unsigned int *)p)[10 + i], i ); + } + winetest_pop_context(); + + ret = pHeapFree( GetProcessHeap(), 0, p ); + ok( ret, "failed.\n" ); + } + p = HeapAlloc( GetProcessHeap(), 0, 37 ); ok( p != NULL, "HeapAlloc failed\n" ); + if (flags & HEAP_FREE_CHECKING_ENABLED) + { + for (i = 0; i < 37 / sizeof(int); ++i) + ok( ((unsigned int *)p)[i] == 0xbaadf00d, "got %#x, i %Iu.\n", ((unsigned int *)p)[i], i ); + } memset( p, 0xcc, 37 );
ret = pHeapFree( GetProcessHeap(), 0, p ); diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index aca8e7181a8..5ce7f8fad2f 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -139,7 +139,7 @@ C_ASSERT( sizeof(ARENA_LARGE) == 4 * BLOCK_ALIGN ); #define BLOCK_TYPE_FREE 'F' #define BLOCK_TYPE_LARGE 'L'
-#define BLOCK_FILL_USED 0x55 +#define BLOCK_FILL_USED 0xbaadf00d #define BLOCK_FILL_TAIL 0xab #define BLOCK_FILL_FREE 0xfeeefeee
@@ -513,6 +513,7 @@ static inline void mark_block_tail( struct block *block, DWORD flags ) static inline void initialize_block( struct block *block, SIZE_T old_size, SIZE_T size, DWORD flags ) { char *data = (char *)(block + 1); + SIZE_T i;
if (size <= old_size) return;
@@ -524,7 +525,8 @@ static inline void initialize_block( struct block *block, SIZE_T old_size, SIZE_ else if (flags & HEAP_FREE_CHECKING_ENABLED) { valgrind_make_writable( data + old_size, size - old_size ); - memset( data + old_size, BLOCK_FILL_USED, size - old_size ); + i = ROUND_SIZE( old_size, sizeof(DWORD) - 1 ) / sizeof(DWORD); + for (; i < size / sizeof(DWORD); ++i) ((DWORD *)data)[i] = BLOCK_FILL_USED; } }
Rémi Bernon (@rbernon) commented about dlls/kernel32/tests/heap.c:
if (flags & HEAP_TAIL_CHECKING_ENABLED)ok( p[39] == 0xab, "got %#x.\n", p[39] );for (i = 0; i < count - 1; ++i)ok( ((unsigned int *)p)[10 + i] == 0xbaadf00d, "got %#x, i %Iu.\n", ((unsigned int *)p)[10 + i], i );}else{ok( p[39] == 0xba, "got %#x.\n", p[39] );for (i = 0; i < count - 1; ++i)ok( ((unsigned int *)p)[10 + i] == 0xbaadf00d, "got %#x, i %Iu.\n", ((unsigned int *)p)[10 + i], i );}winetest_pop_context();ret = pHeapFree( GetProcessHeap(), 0, p );ok( ret, "failed.\n" );- }
```suggestion:-34+0 if (flags & HEAP_FREE_CHECKING_ENABLED) { UINT *p32, tmp = 0;
size = 4 + 3; p = pHeapAlloc( GetProcessHeap(), 0, size ); ok( !!p, "HeapAlloc failed\n" ); p32 = (UINT *)p;
ok( p32[0] == 0xbaadf00d, "got %#x\n", p32[0] ); memcpy( &tmp, p + size - 3, 3 ); ok( tmp != 0xadf00d, "got %#x\n", tmp ); memset( p, 0xcc, size );
size += 2 * 4; p = pHeapReAlloc( GetProcessHeap(), 0, p, size ); ok( !!p, "HeapReAlloc failed\n" ); p32 = (UINT *)p;
ok( p32[0] == 0xcccccccc, "got %#x\n", p32[0] ); ok( p32[1] << 8 == 0xcccccc00, "got %#x\n", p32[1] ); if (flags & HEAP_TAIL_CHECKING_ENABLED) ok( p32[1] >> 24 == 0xab, "got %#x\n", p32[1] ); ok( p32[2] == 0xbaadf00d, "got %#x\n", p32[2] ); memcpy( &tmp, p + size - 3, 3 ); ok( tmp != 0xadf00d, "got %#x\n", tmp );
ret = pHeapFree( GetProcessHeap(), 0, p ); ok( ret, "failed.\n" ); } ```
I think this would be simpler, the loops seem unnecessary.
I removed the non-inplace check with 0xba, as I don't think it is reliable and hopefully doesn't matter?
Rémi Bernon (@rbernon) commented about dlls/kernel32/tests/heap.c:
for (i = 0; i < count - 1; ++i)ok( ((unsigned int *)p)[10 + i] == 0xbaadf00d, "got %#x, i %Iu.\n", ((unsigned int *)p)[10 + i], i );}winetest_pop_context();ret = pHeapFree( GetProcessHeap(), 0, p );ok( ret, "failed.\n" );- }
- p = HeapAlloc( GetProcessHeap(), 0, 37 ); ok( p != NULL, "HeapAlloc failed\n" );
- if (flags & HEAP_FREE_CHECKING_ENABLED)
- {
for (i = 0; i < 37 / sizeof(int); ++i)ok( ((unsigned int *)p)[i] == 0xbaadf00d, "got %#x, i %Iu.\n", ((unsigned int *)p)[i], i );- }
This could probably be dropped.
On Tue Jul 25 12:12:10 2023 +0000, Rémi Bernon wrote:
if (flags & HEAP_FREE_CHECKING_ENABLED) { UINT *p32, tmp = 0; size = 4 + 3; p = pHeapAlloc( GetProcessHeap(), 0, size ); ok( !!p, "HeapAlloc failed\n" ); p32 = (UINT *)p; ok( p32[0] == 0xbaadf00d, "got %#x\n", p32[0] ); memcpy( &tmp, p + size - 3, 3 ); ok( tmp != 0xadf00d, "got %#x\n", tmp ); memset( p, 0xcc, size ); size += 2 * 4; p = pHeapReAlloc( GetProcessHeap(), 0, p, size ); ok( !!p, "HeapReAlloc failed\n" ); p32 = (UINT *)p; ok( p32[0] == 0xcccccccc, "got %#x\n", p32[0] ); ok( p32[1] << 8 == 0xcccccc00, "got %#x\n", p32[1] ); if (flags & HEAP_TAIL_CHECKING_ENABLED) ok( p32[1] >> 24 == 0xab, "got %#x\n", p32[1] ); ok( p32[2] == 0xbaadf00d, "got %#x\n", p32[2] ); memcpy( &tmp, p + size - 3, 3 ); ok( tmp != 0xadf00d, "got %#x\n", tmp ); ret = pHeapFree( GetProcessHeap(), 0, p ); ok( ret, "failed.\n" ); }I think this would be simpler, the loops seem unnecessary. I removed the non-inplace check with 0xba, as I don't think it is reliable and hopefully doesn't matter?
I don't think that the test with 0xba matters too much, but it also seems to me that it is reliable. That is the high byte of BLOCK_FILL_USED (0xbaadf00d), and it gets there when the new block is allocated out of place and gets filled before 39 bytes are copied from the old location, so not a leftover from previous allocation at those addresses (testing those would be indeed flaky).
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon (＠rbernon)