[PATCH 0/4] MR1361: kernel32: Validate globalmem pointers before calling heap functions.

[Rémi Bernon (＠rbernon)]

Tests show that Global / Local functions are more robust and catch invalid pointers, to the contrary to heap functions.

I'd like to relax a bit the heap quick pointer validation, in order to avoid looping over the heap regions on every call, and to reduce the locking requirements, though this would then break the globalmem invalid pointer tests, unless they validate the pointers beforehand.

They should maybe use exception handlers rather than pointer validation, but the result is the same. I can change that if it matters.

[PATCH 1/4] kernel32: Validate pointer before calling HeapSize.

[Rémi Bernon]

From: Rémi Bernon

---
 dlls/kernel32/heap.c       | 3 ++-
 dlls/kernel32/tests/heap.c | 2 --
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/dlls/kernel32/heap.c b/dlls/kernel32/heap.c
index 5f7f950d23d..d433a703d16 100644
--- a/dlls/kernel32/heap.c
+++ b/dlls/kernel32/heap.c
@@ -385,7 +385,8 @@ SIZE_T WINAPI LocalSize( HLOCAL handle )
     TRACE_(globalmem)( "handle %p\n", handle );

RtlLockHeap( heap );
-    if ((ptr = unsafe_ptr_from_HLOCAL( handle )))
+    if ((ptr = unsafe_ptr_from_HLOCAL( handle )) &&
+        HeapValidate( heap, HEAP_NO_SERIALIZE, ptr ))
         ret = HeapSize( heap, HEAP_NO_SERIALIZE, ptr );
     else if ((mem = unsafe_mem_from_HLOCAL( handle )))
     {
diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c
index 46256493913..3e861f52248 100644
--- a/dlls/kernel32/tests/heap.c
+++ b/dlls/kernel32/tests/heap.c
@@ -1623,7 +1623,6 @@ static void test_GlobalAlloc(void)
     SetLastError( 0xdeadbeef );
     size = GlobalSize( invalid_ptr );
     ok( size == 0, "GlobalSize succeeded\n" );
-    todo_wine
     ok( GetLastError() == ERROR_INVALID_HANDLE, "got error %lu\n", GetLastError() );
     SetLastError( 0xdeadbeef );
     ptr = GlobalLock( invalid_ptr );
@@ -2014,7 +2013,6 @@ static void test_LocalAlloc(void)
     SetLastError( 0xdeadbeef );
     size = LocalSize( invalid_ptr );
     ok( size == 0, "LocalSize succeeded\n" );
-    todo_wine
     ok( GetLastError() == ERROR_INVALID_HANDLE, "got error %lu\n", GetLastError() );
     SetLastError( 0xdeadbeef );
     ptr = LocalLock( invalid_ptr );

[PATCH 2/4] kernel32: Lock heap and validate pointer in LocalHandle.

[Rémi Bernon]

From: Rémi Bernon

---
 dlls/kernel32/heap.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/dlls/kernel32/heap.c b/dlls/kernel32/heap.c
index d433a703d16..9e1da01778d 100644
--- a/dlls/kernel32/heap.c
+++ b/dlls/kernel32/heap.c
@@ -343,6 +343,7 @@ UINT WINAPI LocalFlags( HLOCAL handle )
  */
 HLOCAL WINAPI LocalHandle( const void *ptr )
 {
+    HANDLE heap = GetProcessHeap();
     HLOCAL handle = (HANDLE)ptr;
     ULONG flags;

@@ -354,11 +355,14 @@ HLOCAL WINAPI LocalHandle( const void *ptr )
         return 0;
     }

-    if (!RtlGetUserInfoHeap( GetProcessHeap(), 0, (void *)ptr, &handle, &flags ))
+    RtlLockHeap( heap );
+    if (!HeapValidate( heap, HEAP_NO_SERIALIZE, ptr ) ||
+        !RtlGetUserInfoHeap( heap, HEAP_NO_SERIALIZE, (void *)ptr, &handle, &flags ))
     {
         SetLastError( ERROR_INVALID_HANDLE );
-        return 0;
+        handle = 0;
     }
+    RtlUnlockHeap( heap );

return handle;
 }

[PATCH 3/4] kernelbase: Validate pointers before calling heap functions.

[Rémi Bernon]

From: Rémi Bernon

---
 dlls/kernelbase/memory.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/dlls/kernelbase/memory.c b/dlls/kernelbase/memory.c
index 2a503587e93..4c462c1633b 100644
--- a/dlls/kernelbase/memory.c
+++ b/dlls/kernelbase/memory.c
@@ -903,7 +903,8 @@ HLOCAL WINAPI DECLSPEC_HOTPATCH LocalFree( HLOCAL handle )
     TRACE_(globalmem)( "handle %p\n", handle );

RtlLockHeap( heap );
-    if ((ptr = unsafe_ptr_from_HLOCAL( handle )))
+    if ((ptr = unsafe_ptr_from_HLOCAL( handle )) &&
+        HeapValidate( heap, HEAP_NO_SERIALIZE, ptr ))
     {
         if (HeapFree( heap, HEAP_NO_SERIALIZE, ptr )) ret = 0;
     }
@@ -985,7 +986,8 @@ HLOCAL WINAPI DECLSPEC_HOTPATCH LocalReAlloc( HLOCAL handle, SIZE_T size, UINT f
     if (flags & LMEM_ZEROINIT) heap_flags |= HEAP_ZERO_MEMORY;

RtlLockHeap( heap );
-    if ((ptr = unsafe_ptr_from_HLOCAL( handle )))
+    if ((ptr = unsafe_ptr_from_HLOCAL( handle )) &&
+        HeapValidate( heap, HEAP_NO_SERIALIZE, ptr ))
     {
         if (flags & LMEM_MODIFY) ret = handle;
         else

[PATCH 4/4] ntdll: Cleanup heap block macro definitions.

[Rémi Bernon]

From: Rémi Bernon

---
 dlls/ntdll/heap.c | 84 +++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 46 deletions(-)

diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
index 3fe601feb2e..dba189d9435 100644
--- a/dlls/ntdll/heap.c
+++ b/dlls/ntdll/heap.c
@@ -81,19 +81,19 @@ struct block
     WORD block_size;   /* block size in multiple of ALIGNMENT */
     BYTE block_flags;
     BYTE tail_size;    /* unused size (used block) / high size bits (free block) */
-    DWORD magic;
+    DWORD block_type;
 };

C_ASSERT( sizeof(struct block) == 8 );

/* block specific flags */

-#define BLOCK_FLAG_FREE        0x00000001
-#define BLOCK_FLAG_PREV_FREE   0x00000002
-#define BLOCK_FLAG_FREE_LINK   0x00000003
-#define BLOCK_FLAG_LARGE       0x00000004
-#define BLOCK_FLAG_USER_INFO   0x00000010 /* user flags up to 0xf0 */
-#define BLOCK_FLAG_USER_MASK   0x000000f0
+#define BLOCK_FLAG_FREE        0x01
+#define BLOCK_FLAG_PREV_FREE   0x02
+#define BLOCK_FLAG_FREE_LINK   0x03
+#define BLOCK_FLAG_LARGE       0x04
+#define BLOCK_FLAG_USER_INFO   0x10 /* user flags up to 0xf0 */
+#define BLOCK_FLAG_USER_MASK   0xf0

#define BLOCK_USER_FLAGS( heap_flags ) (((heap_flags) >> 4) & BLOCK_FLAG_USER_MASK)
 #define HEAP_USER_FLAGS( block_flags ) (((block_flags) & BLOCK_FLAG_USER_MASK) << 4)
@@ -122,20 +122,15 @@ typedef struct
 C_ASSERT( sizeof(ARENA_LARGE) == offsetof(ARENA_LARGE, block) + sizeof(struct block) );
 C_ASSERT( sizeof(ARENA_LARGE) == 4 * ALIGNMENT );

+#define BLOCK_TYPE_USED        0x455355
+#define BLOCK_TYPE_DEAD        0xbedead
+#define BLOCK_TYPE_FREE        0x45455246
+#define BLOCK_TYPE_LARGE       0x6752614c

-#define ARENA_SIZE_MASK        (~3)
+#define BLOCK_FILL_USED        0x55
+#define BLOCK_FILL_TAIL        0xab
+#define BLOCK_FILL_FREE        0xfeeefeee

-/* Value for arena 'magic' field */
-#define ARENA_INUSE_MAGIC      0x455355
-#define ARENA_PENDING_MAGIC    0xbedead
-#define ARENA_FREE_MAGIC       0x45455246
-#define ARENA_LARGE_MAGIC      0x6752614c
-
-#define ARENA_INUSE_FILLER     0x55
-#define ARENA_TAIL_FILLER      0xab
-#define ARENA_FREE_FILLER      0xfeeefeee
-
-/* everything is aligned on 8 byte boundaries (16 for Win64) */
 #define COMMIT_MASK            0xffff  /* bitmask for commit/decommit granularity */

#define ROUND_ADDR(addr, mask) ((void *)((UINT_PTR)(addr) & ~(UINT_PTR)(mask)))
@@ -148,9 +143,6 @@ 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 */
-#define HEAP_TAIL_EXTRA_SIZE(flags) \
-    ((flags & HEAP_TAIL_CHECKING_ENABLED) || RUNNING_ON_VALGRIND ? ALIGNMENT : 0)

/* There will be a free list bucket for every arena size up to and including this value */
 #define HEAP_MAX_SMALL_FREE_LIST 0x100
@@ -239,12 +231,12 @@ static inline UINT block_get_flags( const struct block *block )

static inline UINT block_get_type( const struct block *block )
 {
-    return block->magic;
+    return block->block_type;
 }

static inline void block_set_type( struct block *block, UINT type )
 {
-    block->magic = type;
+    block->block_type = type;
 }

static inline UINT block_get_overhead( const struct block *block )
@@ -363,7 +355,7 @@ static inline void mark_block_free( void *ptr, SIZE_T size, DWORD flags )
     if (flags & HEAP_FREE_CHECKING_ENABLED)
     {
         SIZE_T i;
-        for (i = 0; i < size / sizeof(DWORD); i++) ((DWORD *)ptr)[i] = ARENA_FREE_FILLER;
+        for (i = 0; i < size / sizeof(DWORD); i++) ((DWORD *)ptr)[i] = BLOCK_FILL_FREE;
     }
     valgrind_make_noaccess( ptr, size );
 }
@@ -375,7 +367,7 @@ static inline void mark_block_tail( struct block *block, DWORD flags )
     if (flags & HEAP_TAIL_CHECKING_ENABLED)
     {
         valgrind_make_writable( tail, ALIGNMENT );
-        memset( tail, ARENA_TAIL_FILLER, ALIGNMENT );
+        memset( tail, BLOCK_FILL_TAIL, ALIGNMENT );
     }
     valgrind_make_noaccess( tail, ALIGNMENT );
     if (flags & HEAP_ADD_USER_INFO)
@@ -397,7 +389,7 @@ static inline void initialize_block( void *ptr, SIZE_T size, DWORD flags )
     else if (flags & HEAP_FREE_CHECKING_ENABLED)
     {
         valgrind_make_writable( ptr, size );
-        memset( ptr, ARENA_INUSE_FILLER, size );
+        memset( ptr, BLOCK_FILL_USED, size );
     }
 }

@@ -436,7 +428,7 @@ static void valgrind_notify_free_all( SUBHEAP *subheap )
     for (block = first_block( subheap ); block; block = next_block( subheap, block ))
     {
         if (block_get_flags( block ) & BLOCK_FLAG_FREE) continue;
-        if (block_get_type( block ) == ARENA_INUSE_MAGIC) valgrind_notify_free( block + 1 );
+        if (block_get_type( block ) == BLOCK_TYPE_USED) valgrind_notify_free( block + 1 );
     }
 #endif
 }
@@ -693,7 +685,7 @@ static void create_free_block( struct heap *heap, SUBHEAP *subheap, struct block
     struct block *next;

valgrind_make_writable( block, sizeof(*entry) );
-    block_set_type( block, ARENA_FREE_MAGIC );
+    block_set_type( block, BLOCK_TYPE_FREE );
     block_set_flags( block, ~0, BLOCK_FLAG_FREE );
     block_set_size( block, block_size );

@@ -736,7 +728,7 @@ static void free_used_block( struct heap *heap, SUBHEAP *subheap, struct block *
         struct block *tmp = heap->pending_free[heap->pending_pos];
         heap->pending_free[heap->pending_pos] = block;
         heap->pending_pos = (heap->pending_pos + 1) % MAX_FREE_PENDING;
-        block_set_type( block, ARENA_PENDING_MAGIC );
+        block_set_type( block, BLOCK_TYPE_DEAD );
         mark_block_free( block + 1, block_get_size( block ) - sizeof(*block), heap->flags );
         if (!(block = tmp) || !(subheap = find_subheap( heap, block, FALSE ))) return;
     }
@@ -816,7 +808,7 @@ static struct block *allocate_large_block( struct heap *heap, DWORD flags, SIZE_
     arena->data_size = size;
     arena->block_size = (char *)address + total_size - (char *)block;

-    block_set_type( block, ARENA_LARGE_MAGIC );
+    block_set_type( block, BLOCK_TYPE_LARGE );
     block_set_flags( block, ~0, BLOCK_FLAG_LARGE | BLOCK_USER_FLAGS( flags ) );
     block_set_size( block, 0 );
     list_add_tail( &heap->large_list, &arena->entry );
@@ -903,7 +895,7 @@ static BOOL validate_large_block( const struct heap *heap, const struct block *b
         err = "invalid block size";
     else if (!(block_get_flags( block ) & BLOCK_FLAG_LARGE))
         err = "invalid block flags";
-    else if (block_get_type( block ) != ARENA_LARGE_MAGIC)
+    else if (block_get_type( block ) != BLOCK_TYPE_LARGE)
         err = "invalid block type";
     else if (!contains( block, arena->block_size, block + 1, arena->data_size ))
         err = "invalid block size";
@@ -984,7 +976,7 @@ static SUBHEAP *HEAP_CreateSubHeap( struct heap **heap_ptr, LPVOID address, DWOR
         {
             block_set_flags( &pEntry->block, ~0, BLOCK_FLAG_FREE_LINK );
             block_set_size( &pEntry->block, 0 );
-            block_set_type( &pEntry->block, ARENA_FREE_MAGIC );
+            block_set_type( &pEntry->block, BLOCK_TYPE_FREE );
             if (i) list_add_after( &pEntry[-1].entry, &pEntry->entry );
         }

@@ -1091,7 +1083,7 @@ static BOOL validate_free_block( const struct heap *heap, const SUBHEAP *subheap

if ((ULONG_PTR)(block + 1) % ALIGNMENT)
         err = "invalid block alignment";
-    else if (block_get_type( block ) != ARENA_FREE_MAGIC)
+    else if (block_get_type( block ) != BLOCK_TYPE_FREE)
         err = "invalid block header";
     else if (!(block_get_flags( block ) & BLOCK_FLAG_FREE) || (block_get_flags( block ) & BLOCK_FLAG_PREV_FREE))
         err = "invalid block flags";
@@ -1099,11 +1091,11 @@ static BOOL validate_free_block( const struct heap *heap, const SUBHEAP *subheap
         err = "invalid block size";
     else if (!is_valid_free_block( heap, (next = (struct block *)LIST_ENTRY( entry->entry.next, struct entry, entry )) ))
         err = "invalid next free block pointer";
-    else if (!(block_get_flags( next ) & BLOCK_FLAG_FREE) || block_get_type( next ) != ARENA_FREE_MAGIC)
+    else if (!(block_get_flags( next ) & BLOCK_FLAG_FREE) || block_get_type( next ) != BLOCK_TYPE_FREE)
         err = "invalid next free block header";
     else if (!is_valid_free_block( heap, (prev = (struct block *)LIST_ENTRY( entry->entry.prev, struct entry, entry )) ))
         err = "invalid previous free block pointer";
-    else if (!(block_get_flags( prev ) & BLOCK_FLAG_FREE) || block_get_type( prev ) != ARENA_FREE_MAGIC)
+    else if (!(block_get_flags( prev ) & BLOCK_FLAG_FREE) || block_get_type( prev ) != BLOCK_TYPE_FREE)
         err = "invalid previous free block header";
     else if ((next = next_block( subheap, (struct block *)block )))
     {
@@ -1120,7 +1112,7 @@ static BOOL validate_free_block( const struct heap *heap, const SUBHEAP *subheap
         if (end > commit_end) end = commit_end;
         while (!err && ptr < end)
         {
-            if (*(DWORD *)ptr != ARENA_FREE_FILLER) err = "free block overwritten";
+            if (*(DWORD *)ptr != BLOCK_FILL_FREE) err = "free block overwritten";
             ptr += sizeof(DWORD);
         }
     }
@@ -1144,7 +1136,7 @@ static BOOL validate_used_block( const struct heap *heap, const SUBHEAP *subheap

if ((ULONG_PTR)(block + 1) % ALIGNMENT)
         err = "invalid block alignment";
-    else if (block_get_type( block ) != ARENA_INUSE_MAGIC && block_get_type( block ) != ARENA_PENDING_MAGIC)
+    else if (block_get_type( block ) != BLOCK_TYPE_USED && block_get_type( block ) != BLOCK_TYPE_DEAD)
         err = "invalid block header";
     else if (block_get_flags( block ) & BLOCK_FLAG_FREE)
         err = "invalid block flags";
@@ -1159,25 +1151,25 @@ static BOOL validate_used_block( const struct heap *heap, const SUBHEAP *subheap
         const struct block *prev = *((struct block **)block - 1);
         if (!is_valid_free_block( heap, prev ))
             err = "invalid previous block pointer";
-        else if (!(block_get_flags( prev ) & BLOCK_FLAG_FREE) || block_get_type( prev ) != ARENA_FREE_MAGIC)
+        else if (!(block_get_flags( prev ) & BLOCK_FLAG_FREE) || block_get_type( prev ) != BLOCK_TYPE_FREE)
             err = "invalid previous block flags";
         if ((char *)prev + block_get_size( prev ) != (char *)block)
             err = "invalid previous block size";
     }

-    if (!err && block_get_type( block ) == ARENA_PENDING_MAGIC)
+    if (!err && block_get_type( block ) == BLOCK_TYPE_DEAD)
     {
         const char *ptr = (char *)(block + 1), *end = (char *)block + block_get_size( block );
         while (!err && ptr < end)
         {
-            if (*(DWORD *)ptr != ARENA_FREE_FILLER) err = "free block overwritten";
+            if (*(DWORD *)ptr != BLOCK_FILL_FREE) err = "free block overwritten";
             ptr += sizeof(DWORD);
         }
     }
     else if (!err && (flags & HEAP_TAIL_CHECKING_ENABLED))
     {
         const unsigned char *tail = (unsigned char *)block + block_get_size( block ) - block->tail_size;
-        for (i = 0; !err && i < ALIGNMENT; i++) if (tail[i] != ARENA_TAIL_FILLER) err = "invalid block tail";
+        for (i = 0; !err && i < ALIGNMENT; i++) if (tail[i] != BLOCK_FILL_TAIL) err = "invalid block tail";
     }

if (err)
@@ -1266,9 +1258,9 @@ static inline struct block *unsafe_block_from_ptr( const struct heap *heap, cons
     }
     else if ((ULONG_PTR)ptr % ALIGNMENT)
         err = "invalid ptr alignment";
-    else if (block_get_type( block ) == ARENA_PENDING_MAGIC || (block_get_flags( block ) & BLOCK_FLAG_FREE))
+    else if (block_get_type( block ) == BLOCK_TYPE_DEAD || (block_get_flags( block ) & BLOCK_FLAG_FREE))
         err = "already freed block";
-    else if (block_get_type( block ) != ARENA_INUSE_MAGIC)
+    else if (block_get_type( block ) != BLOCK_TYPE_USED)
         err = "invalid block header";
     else if (!contains( base, commit_end - base, block, block_get_size( block ) ))
         err = "invalid block size";
@@ -1342,7 +1334,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 );
+                    if (block_get_type( block ) == BLOCK_TYPE_DEAD) mark_block_free( block + 1, block_get_size( block ) - sizeof(*block), flags );
                     else mark_block_tail( block, flags );
                 }
             }
@@ -1524,7 +1516,7 @@ static NTSTATUS heap_allocate( struct heap *heap, ULONG flags, SIZE_T size, void
     /* read the free block size, changing block type or flags may alter it */
     old_block_size = block_get_size( block );

-    block_set_type( block, ARENA_INUSE_MAGIC );
+    block_set_type( block, BLOCK_TYPE_USED );
     block_set_flags( block, ~0, BLOCK_USER_FLAGS( flags ) );
     shrink_used_block( heap, subheap, block, old_block_size, block_size, size );
     initialize_block( block + 1, size, flags );

[PATCH 4/4] ntdll: Cleanup heap block macro definitions.

[Marvin]

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=126151

Your paranoid android.

=== debian11 (32 bit report) ===

crypt32:
cert.c:4191: Test failed: success
cert.c:4192: Test failed: got 00000000
cert.c:4193: Test failed: got 00000000

[Rémi Bernon (＠rbernon)]

Hmm... I guess it'd be better and more robust, to use IsBadReadPtr or a dedicated exception handler?