[PATCH v2 0/1] MR3379: ntdll: Match Windows used block filling.
-- v2: ntdll: Match Windows used block filling. https://gitlab.winehq.org/wine/wine/-/merge_requests/3379
From: Paul Gofman <pgofman(a)codeweavers.com> Test rewritten by Rémi Bernon. --- dlls/kernel32/tests/heap.c | 31 +++++++++++++++++++++++++++++++ dlls/ntdll/heap.c | 6 ++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 59a82c0c579..2174aa338ea 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -3270,6 +3270,37 @@ static void test_heap_checks( DWORD flags ) ret = HeapFree( GetProcessHeap(), 0, p ); ok( ret, "HeapFree failed\n" ); + 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" ); + } + p = HeapAlloc( GetProcessHeap(), 0, 37 ); ok( p != NULL, "HeapAlloc failed\n" ); memset( p, 0xcc, 37 ); 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; } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3379
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=135234 Your paranoid android. === w864 (32 bit report) === kernel32: heap.c:3295: Test failed: got 0xbacccccc heap.c:3295: Test failed: got 0xbacccccc === w1064v1507 (32 bit report) === kernel32: heap.c:3295: Test failed: got 0xbacccccc heap.c:3295: Test failed: got 0xbacccccc === w1064v1809 (32 bit report) === kernel32: heap.c:3295: Test failed: got 0xbacccccc heap.c:3295: Test failed: got 0xbacccccc === w11pro64 (32 bit report) === kernel32: heap.c:3295: Test failed: got 0xbacccccc heap.c:3295: Test failed: got 0xbacccccc === debian11 (32 bit ar:MA report) === kernel32: heap.c:3295: Test failed: got 0xbacccccc heap.c:3295: Test failed: got 0xbacccccc === debian11 (32 bit fr report) === kernel32: heap.c:3295: Test failed: got 0xbacccccc heap.c:3295: Test failed: got 0xbacccccc === debian11 (32 bit he:IL report) === kernel32: heap.c:3295: Test failed: got 0xbacccccc heap.c:3295: Test failed: got 0xbacccccc === debian11 (32 bit hi:IN report) === kernel32: heap.c:3295: Test failed: got 0xbacccccc heap.c:3295: Test failed: got 0xbacccccc === debian11 (32 bit ja:JP report) === kernel32: heap.c:3295: Test failed: got 0xbacccccc heap.c:3295: Test failed: got 0xbacccccc === debian11 (32 bit zh:CN report) === kernel32: heap.c:3295: Test failed: got 0xbacccccc heap.c:3295: Test failed: got 0xbacccccc
v2: - applied test changes suggested by Rémi. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3379#note_40202
On Tue Jul 25 16:24:01 2023 +0000, Paul Gofman wrote:
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). Ah indeed, I couldn't figure how it ended up there but I see now.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3379#note_40203
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3379
On Tue Jul 25 16:51:36 2023 +0000, Rémi Bernon wrote:
Ah indeed, I couldn't figure how it ended up there but I see now. And it looks like my suggested change is actually causing the tests to fail on this particular out-of-place case, sorry about that. Feel free to add it back.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3379#note_40208
On Tue Jul 25 17:38:12 2023 +0000, Rémi Bernon wrote:
And it looks like my suggested change is actually causing the tests to fail on this particular out-of-place case, sorry about that. Feel free to add it back. I suggest I just drop `'ok( p32[1] >> 24 == 0xab, "got %#x\n", p32[1] );'. Looks like it is pretty clear how it works, complicating the thing keeping a lot of neat checks probably doesn't worth it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3379#note_40211
participants (4)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon