[PATCH 0/1] MR3379: ntdll: Match Windows used block filling.

[Paul Gofman (＠gofman)]

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3379

[PATCH 1/1] ntdll: Match Windows used block filling.

[Paul Gofman]

From: Paul Gofman <pgofman(a)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;
     }
 }
 
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/3379

[Rémi Bernon]

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" );
> +    }
    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] &lt;&lt; 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?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3379#note_40129

[Rémi Bernon]

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.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3379#note_40130

[Paul Gofman (＠gofman)]

On Tue Jul 25 12:12:10 2023 +0000, Rémi Bernon wrote:
> ```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?

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).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3379#note_40195