[PATCH v3 0/1] MR8852: ntdll: Zero aligned size in initialize_block().
-- v3: ntdll: Zero aligned size in initialize_block(). https://gitlab.winehq.org/wine/wine/-/merge_requests/8852
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/kernel32/tests/heap.c | 60 ++++++++++++++++++++++++++++++++++++++ dlls/ntdll/heap.c | 8 +++-- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index f88a140a115..93fee8b274f 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -82,6 +82,21 @@ static void load_functions(void) #undef LOAD_FUNC } +static BOOL check_win_version(int min_major, int min_minor) +{ + HMODULE hntdll = GetModuleHandleA("ntdll.dll"); + NTSTATUS (WINAPI *pRtlGetVersion)(RTL_OSVERSIONINFOEXW *); + RTL_OSVERSIONINFOEXW rtlver; + + rtlver.dwOSVersionInfoSize = sizeof(RTL_OSVERSIONINFOEXW); + pRtlGetVersion = (void *)GetProcAddress(hntdll, "RtlGetVersion"); + pRtlGetVersion(&rtlver); + return rtlver.dwMajorVersion > min_major || + (rtlver.dwMajorVersion == min_major && + rtlver.dwMinorVersion >= min_minor); +} +#define is_win8_plus() check_win_version(6, 2) + struct heap { UINT_PTR unknown1[2]; @@ -3456,6 +3471,49 @@ static void test_heap_layout( HANDLE handle, DWORD global_flag, DWORD heap_flags } } +static void test_heap_tail_zeroing( DWORD heap_flags ) +{ + static const ULONG_PTR large_block_min_size = 65536 * (2 * sizeof(void *)); + BOOL before_win8 = !is_win8_plus(); + HANDLE heap = GetProcessHeap(); + size_t size, size_aligned; + ULONG_PTR v, expected; + char *p1, *p2; + + if (heap_flags & HEAP_PAGE_ALLOCS) + { + /* This behaves differently, no support yet. */ + skip( "Skipping test with HEAP_PAGE_ALLOCS.\n" ); + return; + } + + for (size = 1; size <= 1048576 * 2; size *= 2) + { + winetest_push_context( "heap_flags %#lx, size %Iu", heap_flags, size ); + p1 = pHeapAlloc( heap, 0, size + 1 ); + ok( !!p1, "got NULL.\n" ); + size_aligned = (size + 1 + sizeof(ULONG_PTR) - 1) & ~(sizeof(ULONG_PTR) - 1); + /* This and read access below is going to make valgrind or ASAN unhappy but the purpose of this test is + * to specifically check what happens with the tail bytes following the allocation. */ + if (!(heap_flags & (HEAP_VALIDATE_PARAMS | HEAP_VALIDATE_ALL))) + memset( p1, 0xcc, size_aligned ); + pHeapFree( heap, 0, p1 ); + + /* We are not guarenteed to get the same pointer here but that often happens, especially when the test + * is run first, and spoling the data before that adds certainity to the results. */ + p2 = pHeapAlloc( heap, HEAP_ZERO_MEMORY, size + 1 ); + ok( !!p2, "got NULL.\n" ); + v = 0; + memcpy( &v, p2 + size, size_aligned - size ); + expected = 0; + if (size_aligned - size > 1 && size + 1 < large_block_min_size && heap_flags & HEAP_TAIL_CHECKING_ENABLED) + memset( (char *)&expected + 1, 0xab, size_aligned - size - 1 ); + ok( v == expected || broken( before_win8 && !expected ), "got %#Ix, expected %#Ix.\n", v, expected ); + pHeapFree( heap, 0, p2 ); + winetest_pop_context(); + } +} + static void test_child_heap( const char *arg ) { char buffer[32]; @@ -3535,6 +3593,7 @@ static void test_child_heap( const char *arg ) ok( ret, "HeapDestroy failed, error %lu\n", GetLastError() ); test_heap_checks( heap_flags ); + test_heap_tail_zeroing( heap_flags ); } static void test_GetPhysicallyInstalledSystemMemory(void) @@ -3808,6 +3867,7 @@ START_TEST(heap) test_GetPhysicallyInstalledSystemMemory(); test_GlobalMemoryStatus(); test_HeapSummary(); + test_heap_tail_zeroing( 0 ); if (pRtlGetNtGlobalFlags) { diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index e761268095e..c68c449a524 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -503,14 +503,16 @@ 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; + SIZE_T i, aligned_size; if (size <= old_size) return; if (flags & HEAP_ZERO_MEMORY) { - valgrind_make_writable( data + old_size, size - old_size ); - memset( data + old_size, 0, size - old_size ); + aligned_size = ROUND_SIZE( size, sizeof(void *) - 1 ); + valgrind_make_writable( data + old_size, aligned_size - old_size ); + memset( data + old_size, 0, aligned_size - old_size ); + valgrind_make_noaccess( data + size, aligned_size - size ); } else if (flags & HEAP_FREE_CHECKING_ENABLED) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8852
v3: - use pHeapAlloc / pHeapFree in tests. (v2 had some totally unrelated ad-hoc tests accidentally committed). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8852#note_114416
On Mon Sep 1 15:42:24 2025 +0000, Rémi Bernon wrote:
The difference between the computation here and the implementation is confusing. Shouldn't it be `size_aligned = ROUND_SIZE( size, sizeof(void *) - 1 )` too? Do you mean `ULONG_PTR` vs 'void *' or something else? Note that allocated size is HeapSize is `size + 1`, that is to make test hit interesting cases of unalgined requested size. So the aligned size matches the implementation, the only difference is that I used `ULONG_PTR` is test, should I change it to 'void *'?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8852#note_114421
On Mon Sep 1 16:00:57 2025 +0000, Paul Gofman wrote:
Do you mean `ULONG_PTR` vs 'void *' or something else? Note that allocated size is HeapSize is `size + 1`, that is to make test hit interesting cases of unalgined requested size. So the aligned size matches the implementation, the only difference is that I used `ULONG_PTR` is test, should I change it to 'void *'? Ah right, I missed that it was allocating size + 1, sorry. Defining the ROUND_SIZE macro in this file could be nice.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8852#note_114423
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon