[PATCH 0/5] MR71: ntdll: Simplify heap validation functions.

[Rémi Bernon]

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

[PATCH 1/5] ntdll: Use heap_validate_ptr in validate_block_pointer.

[Rémi Bernon]

From: Rémi Bernon <rbernon(a)codeweavers.com>

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
 dlls/ntdll/heap.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
index 255a3850c70..7ccaf5748e2 100644
--- a/dlls/ntdll/heap.c
+++ b/dlls/ntdll/heap.c
@@ -1437,14 +1437,13 @@ static BOOL HEAP_ValidateInUseArena( const SUBHEAP *subheap, const ARENA_INUSE *
 }
 
 
-static BOOL heap_validate_ptr( HEAP *heap, const void *ptr )
+static BOOL heap_validate_ptr( HEAP *heap, const void *ptr, SUBHEAP **subheap )
 {
     const ARENA_INUSE *arena = (const ARENA_INUSE *)ptr - 1;
     const ARENA_LARGE *large_arena;
-    SUBHEAP *subheap;
 
-    if (!(subheap = find_subheap( heap, arena )) ||
-        ((const char *)arena < (char *)subheap->base + subheap->headerSize))
+    if (!(*subheap = find_subheap( heap, arena )) ||
+        ((const char *)arena < (char *)(*subheap)->base + (*subheap)->headerSize))
     {
         if (!(large_arena = find_large_block( heap, ptr )))
         {
@@ -1455,7 +1454,7 @@ static BOOL heap_validate_ptr( HEAP *heap, const void *ptr )
         return validate_large_arena( heap, large_arena, QUIET );
     }
 
-    return HEAP_ValidateInUseArena( subheap, arena, QUIET );
+    return HEAP_ValidateInUseArena( *subheap, arena, QUIET );
 }
 
 static BOOL heap_validate( HEAP *heap, BOOL quiet )
@@ -1506,24 +1505,20 @@ static BOOL validate_block_pointer( HEAP *heap, SUBHEAP **ret_subheap, const ARE
     SUBHEAP *subheap;
     BOOL ret = FALSE;
 
+    if (heap->flags & HEAP_VALIDATE) return heap_validate_ptr( heap, arena + 1, ret_subheap );
+
     if (!(*ret_subheap = subheap = find_subheap( heap, arena )))
     {
-        ARENA_LARGE *large_arena = find_large_block( heap, arena + 1 );
-
-        if (!large_arena)
+        if (!find_large_block( heap, arena + 1 ))
         {
             WARN( "Heap %p: pointer %p is not inside heap\n", heap, arena + 1 );
             return FALSE;
         }
-        if ((heap->flags & HEAP_VALIDATE) && !validate_large_arena( heap, large_arena, QUIET ))
-            return FALSE;
         return TRUE;
     }
 
     if ((const char *)arena < (char *)subheap->base + subheap->headerSize)
         WARN( "Heap %p: pointer %p is inside subheap %p header\n", subheap->heap, arena + 1, subheap );
-    else if (subheap->heap->flags & HEAP_VALIDATE)  /* do the full validation */
-        ret = HEAP_ValidateInUseArena( subheap, arena, QUIET );
     else if ((ULONG_PTR)arena % ALIGNMENT != ARENA_OFFSET)
         WARN( "Heap %p: unaligned arena pointer %p\n", subheap->heap, arena );
     else if (arena->magic == ARENA_PENDING_MAGIC)
@@ -2102,6 +2097,7 @@ SIZE_T WINAPI RtlSizeHeap( HANDLE heap, ULONG flags, const void *ptr )
  */
 BOOLEAN WINAPI RtlValidateHeap( HANDLE heap, ULONG flags, const void *ptr )
 {
+    SUBHEAP *subheap;
     HEAP *heapPtr;
     BOOLEAN ret;
 
@@ -2110,7 +2106,7 @@ BOOLEAN WINAPI RtlValidateHeap( HANDLE heap, ULONG flags, const void *ptr )
     else
     {
         heap_lock( heapPtr, flags );
-        if (ptr) ret = heap_validate_ptr( heapPtr, ptr );
+        if (ptr) ret = heap_validate_ptr( heapPtr, ptr, &subheap );
         else ret = heap_validate( heapPtr, QUIET );
         heap_unlock( heapPtr, flags );
     }
-- 
GitLab


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

[PATCH 2/5] ntdll: Simplify validate_block_pointer.

[Rémi Bernon]

From: Rémi Bernon <rbernon(a)codeweavers.com>

And rename it to unsafe_block_from_ptr.

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
 dlls/ntdll/heap.c | 84 +++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 47 deletions(-)

diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
index 7ccaf5748e2..295b215b3b2 100644
--- a/dlls/ntdll/heap.c
+++ b/dlls/ntdll/heap.c
@@ -930,7 +930,7 @@ static ARENA_LARGE *find_large_block( const HEAP *heap, const void *ptr )
 /***********************************************************************
  *           validate_large_arena
  */
-static BOOL validate_large_arena( HEAP *heap, const ARENA_LARGE *arena, BOOL quiet )
+static BOOL validate_large_arena( const HEAP *heap, const ARENA_LARGE *arena, BOOL quiet )
 {
     DWORD flags = heap->flags;
 
@@ -1437,7 +1437,7 @@ static BOOL HEAP_ValidateInUseArena( const SUBHEAP *subheap, const ARENA_INUSE *
 }
 
 
-static BOOL heap_validate_ptr( HEAP *heap, const void *ptr, SUBHEAP **subheap )
+static BOOL heap_validate_ptr( const HEAP *heap, const void *ptr, SUBHEAP **subheap )
 {
     const ARENA_INUSE *arena = (const ARENA_INUSE *)ptr - 1;
     const ARENA_LARGE *large_arena;
@@ -1494,48 +1494,43 @@ static BOOL heap_validate( HEAP *heap, BOOL quiet )
     return TRUE;
 }
 
-
-/***********************************************************************
- *           validate_block_pointer
- *
- * Minimum validation needed to catch bad parameters in heap functions.
- */
-static BOOL validate_block_pointer( HEAP *heap, SUBHEAP **ret_subheap, const ARENA_INUSE *arena )
+static inline struct block *unsafe_block_from_ptr( const HEAP *heap, const void *ptr, SUBHEAP **subheap )
 {
-    SUBHEAP *subheap;
-    BOOL ret = FALSE;
+    struct block *block = (struct block *)ptr - 1;
+    const char *err = NULL, *base, *commit_end;
+    SIZE_T blocks_size;
 
-    if (heap->flags & HEAP_VALIDATE) return heap_validate_ptr( heap, arena + 1, ret_subheap );
+    if (heap->flags & HEAP_VALIDATE)
+    {
+        if (!heap_validate_ptr( heap, ptr, subheap )) return NULL;
+        return block;
+    }
 
-    if (!(*ret_subheap = subheap = find_subheap( heap, arena )))
+    if ((*subheap = find_subheap( heap, block )))
     {
-        if (!find_large_block( heap, arena + 1 ))
-        {
-            WARN( "Heap %p: pointer %p is not inside heap\n", heap, arena + 1 );
-            return FALSE;
-        }
-        return TRUE;
-    }
-
-    if ((const char *)arena < (char *)subheap->base + subheap->headerSize)
-        WARN( "Heap %p: pointer %p is inside subheap %p header\n", subheap->heap, arena + 1, subheap );
-    else if ((ULONG_PTR)arena % ALIGNMENT != ARENA_OFFSET)
-        WARN( "Heap %p: unaligned arena pointer %p\n", subheap->heap, arena );
-    else if (arena->magic == ARENA_PENDING_MAGIC)
-        WARN( "Heap %p: block %p used after free\n", subheap->heap, arena + 1 );
-    else if (arena->magic != ARENA_INUSE_MAGIC)
-        WARN( "Heap %p: invalid in-use arena magic %08x for %p\n", subheap->heap, arena->magic, arena );
-    else if (arena->size & ARENA_FLAG_FREE)
-        ERR( "Heap %p: bad flags %08x for in-use arena %p\n",
-             subheap->heap, arena->size & ~ARENA_SIZE_MASK, arena );
-    else if ((const char *)(arena + 1) + (arena->size & ARENA_SIZE_MASK) > (const char *)subheap->base + subheap->size ||
-             (const char *)(arena + 1) + (arena->size & ARENA_SIZE_MASK) < (const char *)(arena + 1))
-        ERR( "Heap %p: bad size %08x for in-use arena %p\n",
-             subheap->heap, arena->size & ARENA_SIZE_MASK, arena );
-    else
-        ret = TRUE;
+        base = subheap_base( *subheap );
+        commit_end = subheap_commit_end( *subheap );
+        blocks_size = (char *)last_block( *subheap ) - (char *)first_block( *subheap );
+    }
 
-    return ret;
+    if (!*subheap)
+    {
+        if (find_large_block( heap, ptr )) return block;
+        err = "block region not found";
+    }
+    else if ((ULONG_PTR)ptr % ALIGNMENT)
+        err = "invalid ptr alignment";
+    else if (!contains( first_block( *subheap ), blocks_size, block, sizeof(*block) ))
+        err = "invalid block pointer";
+    else if (block_get_type( block ) == ARENA_PENDING_MAGIC || (block_get_flags( block ) & ARENA_FLAG_FREE))
+        err = "already freed block";
+    else if (block_get_type( block ) != ARENA_INUSE_MAGIC)
+        err = "invalid block header";
+    else if (!contains( base, commit_end - base, block, block_get_size( block ) ))
+        err = "invalid block size";
+
+    if (err) WARN( "heap %p, block %p: %s\n", heap, block, err );
+    return err ? NULL : block;
 }
 
 static DWORD heap_flags_from_global_flag( DWORD flag )
@@ -1826,9 +1821,7 @@ static NTSTATUS heap_free( HEAP *heap, void *ptr )
     /* Inform valgrind we are trying to free memory, so it can throw up an error message */
     notify_free( ptr );
 
-    block = (ARENA_INUSE *)ptr - 1;
-    if (!validate_block_pointer( heap, &subheap, block )) return STATUS_INVALID_PARAMETER;
-
+    if (!(block = unsafe_block_from_ptr( heap, ptr, &subheap ))) return STATUS_INVALID_PARAMETER;
     if (!subheap) free_large_block( heap, ptr );
     else HEAP_MakeInUseBlockFree( subheap, block );
 
@@ -1870,9 +1863,7 @@ static NTSTATUS heap_reallocate( HEAP *heap, ULONG flags, void *ptr, SIZE_T size
     if (rounded_size < size) return STATUS_NO_MEMORY;  /* overflow */
     if (rounded_size < HEAP_MIN_DATA_SIZE) rounded_size = HEAP_MIN_DATA_SIZE;
 
-    pArena = (ARENA_INUSE *)ptr - 1;
-    if (!validate_block_pointer( heap, &subheap, pArena )) return STATUS_INVALID_PARAMETER;
-
+    if (!(pArena = unsafe_block_from_ptr( heap, ptr, &subheap ))) return STATUS_INVALID_PARAMETER;
     if (!subheap)
     {
         if (!(*ret = realloc_large_block( heap, flags, ptr, size ))) return STATUS_NO_MEMORY;
@@ -2056,8 +2047,7 @@ static NTSTATUS heap_size( HEAP *heap, const void *ptr, SIZE_T *size )
     const ARENA_INUSE *block;
     SUBHEAP *subheap;
 
-    block = (const ARENA_INUSE *)ptr - 1;
-    if (!validate_block_pointer( heap, &subheap, block )) return STATUS_INVALID_PARAMETER;
+    if (!(block = unsafe_block_from_ptr( heap, ptr, &subheap ))) return STATUS_INVALID_PARAMETER;
     if (!subheap)
     {
         const ARENA_LARGE *large_arena = (const ARENA_LARGE *)ptr - 1;
-- 
GitLab


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

[PATCH 3/5] ntdll: Simplify validate_large_arena.

[Rémi Bernon]

From: Rémi Bernon <rbernon(a)codeweavers.com>

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
 dlls/ntdll/heap.c | 73 ++++++++++++-----------------------------------
 1 file changed, 19 insertions(+), 54 deletions(-)

diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
index 295b215b3b2..504fd3fa743 100644
--- a/dlls/ntdll/heap.c
+++ b/dlls/ntdll/heap.c
@@ -926,66 +926,31 @@ static ARENA_LARGE *find_large_block( const HEAP *heap, const void *ptr )
     return NULL;
 }
 
-
-/***********************************************************************
- *           validate_large_arena
- */
-static BOOL validate_large_arena( const HEAP *heap, const ARENA_LARGE *arena, BOOL quiet )
+static BOOL validate_large_arena( const HEAP *heap, const ARENA_LARGE *arena )
 {
-    DWORD flags = heap->flags;
+    const char *err = NULL;
 
-    if ((ULONG_PTR)arena % page_size)
-    {
-        if (quiet == NOISY)
-        {
-            ERR( "Heap %p: invalid large arena pointer %p\n", heap, arena );
-            if (TRACE_ON(heap)) heap_dump( heap );
-        }
-        else if (WARN_ON(heap))
-        {
-            WARN( "Heap %p: unaligned arena pointer %p\n", heap, arena );
-            if (TRACE_ON(heap)) heap_dump( heap );
-        }
-        return FALSE;
-    }
-    if (arena->size != ARENA_LARGE_SIZE || arena->magic != ARENA_LARGE_MAGIC)
-    {
-        if (quiet == NOISY)
-        {
-            ERR( "Heap %p: invalid large arena %p values %x/%x\n",
-                 heap, arena, arena->size, arena->magic );
-            if (TRACE_ON(heap)) heap_dump( heap );
-        }
-        else if (WARN_ON(heap))
-        {
-            WARN( "Heap %p: invalid large arena %p values %x/%x\n",
-                  heap, arena, arena->size, arena->magic );
-            if (TRACE_ON(heap)) heap_dump( heap );
-        }
-        return FALSE;
-    }
-    if (arena->data_size > arena->block_size - sizeof(*arena))
-    {
-        ERR( "Heap %p: invalid large arena %p size %lx/%lx\n",
-             heap, arena, arena->data_size, arena->block_size );
-        return FALSE;
-    }
-    if (flags & HEAP_TAIL_CHECKING_ENABLED)
+    if ((ULONG_PTR)arena & COMMIT_MASK)
+        err = "invalid block alignment";
+    else if (arena->size != ARENA_LARGE_SIZE || arena->magic != ARENA_LARGE_MAGIC)
+        err = "invalid block header";
+    else if (!contains( arena, arena->block_size, arena + 1, arena->data_size ))
+        err = "invalid block size";
+    else if (heap->flags & HEAP_TAIL_CHECKING_ENABLED)
     {
         SIZE_T i, unused = arena->block_size - sizeof(*arena) - arena->data_size;
         const unsigned char *data = (const unsigned char *)(arena + 1) + arena->data_size;
+        for (i = 0; i < unused && !err; i++) if (data[i] != ARENA_TAIL_FILLER) err = "invalid block tail";
+    }
 
-        for (i = 0; i < unused; i++)
-        {
-            if (data[i] == ARENA_TAIL_FILLER) continue;
-            ERR("Heap %p: block %p tail overwritten at %p (byte %lu/%lu == 0x%02x)\n",
-                heap, arena + 1, data + i, i, unused, data[i] );
-            return FALSE;
-        }
+    if (err)
+    {
+        ERR( "heap %p, block %p: %s\n", heap, arena, err );
+        if (TRACE_ON(heap)) heap_dump( heap );
     }
-    return TRUE;
-}
 
+    return !err;
+}
 
 /***********************************************************************
  *           HEAP_CreateSubHeap
@@ -1451,7 +1416,7 @@ static BOOL heap_validate_ptr( const HEAP *heap, const void *ptr, SUBHEAP **subh
             return FALSE;
         }
 
-        return validate_large_arena( heap, large_arena, QUIET );
+        return validate_large_arena( heap, large_arena );
     }
 
     return HEAP_ValidateInUseArena( *subheap, arena, QUIET );
@@ -1489,7 +1454,7 @@ static BOOL heap_validate( HEAP *heap, BOOL quiet )
     }
 
     LIST_FOR_EACH_ENTRY( large_arena, &heap->large_list, ARENA_LARGE, entry )
-        if (!validate_large_arena( heap, large_arena, quiet )) return FALSE;
+        if (!validate_large_arena( heap, large_arena )) return FALSE;
 
     return TRUE;
 }
-- 
GitLab


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

[PATCH 4/5] ntdll: Simplify validate_used_block.

[Rémi Bernon]

From: Rémi Bernon <rbernon(a)codeweavers.com>

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
 dlls/ntdll/heap.c | 162 +++++++++++++---------------------------------
 1 file changed, 45 insertions(+), 117 deletions(-)

diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
index 504fd3fa743..a4c3c8b0c9c 100644
--- a/dlls/ntdll/heap.c
+++ b/dlls/ntdll/heap.c
@@ -1271,140 +1271,68 @@ static BOOL HEAP_ValidateFreeArena( SUBHEAP *subheap, ARENA_FREE *pArena )
 }
 
 
-/***********************************************************************
- *           HEAP_ValidateInUseArena
- */
-static BOOL HEAP_ValidateInUseArena( const SUBHEAP *subheap, const ARENA_INUSE *pArena, BOOL quiet )
+static BOOL validate_used_block( const SUBHEAP *subheap, const struct block *block )
 {
-    SIZE_T size;
-    DWORD i, flags = subheap->heap->flags;
-    const char *heapEnd = (const char *)subheap->base + subheap->size;
+    const char *err = NULL, *base = subheap_base( subheap ), *commit_end = subheap_commit_end( subheap );
+    SIZE_T blocks_size = (char *)last_block( subheap ) - (char *)first_block( subheap );
+    const HEAP *heap = subheap->heap;
+    DWORD flags = heap->flags;
+    const struct block *next;
+    int i;
 
-    /* Check for unaligned pointers */
-    if ((ULONG_PTR)pArena % ALIGNMENT != ARENA_OFFSET)
+    if ((ULONG_PTR)(block + 1) % ALIGNMENT)
+        err = "invalid block alignment";
+    else if (!contains( first_block( subheap ), blocks_size, block, sizeof(*block) ))
+        err = "invalid block pointer";
+    else if (block_get_type( block ) != ARENA_INUSE_MAGIC && block_get_type( block ) != ARENA_PENDING_MAGIC)
+        err = "invalid block header";
+    else if (block_get_flags( block ) & ARENA_FLAG_FREE)
+        err = "invalid block flags";
+    else if (!contains( base, commit_end - base, block, block_get_size( block ) ))
+        err = "invalid block size";
+    else if (block->unused_bytes > block_get_size( block ) - sizeof(*block))
+        err = "invalid block unused size";
+    else if ((next = next_block( subheap, block )) && (block_get_flags( next ) & ARENA_FLAG_PREV_FREE))
+        err = "invalid next block flags";
+    else if (block_get_flags( block ) & ARENA_FLAG_PREV_FREE)
     {
-        if ( quiet == NOISY )
-        {
-            ERR( "Heap %p: unaligned arena pointer %p\n", subheap->heap, pArena );
-            if ( TRACE_ON(heap) )
-                heap_dump( subheap->heap );
-        }
-        else if ( WARN_ON(heap) )
-        {
-            WARN( "Heap %p: unaligned arena pointer %p\n", subheap->heap, pArena );
-            if ( TRACE_ON(heap) )
-                heap_dump( subheap->heap );
-        }
-        return FALSE;
+        const struct block *prev = *((struct block **)block - 1);
+        if (!HEAP_IsValidArenaPtr( heap, (struct entry *)prev ))
+            err = "invalid previous block pointer";
+        else if (!(block_get_flags( prev ) & ARENA_FLAG_FREE) || block_get_type( prev ) != ARENA_FREE_MAGIC)
+            err = "invalid previous block flags";
+        if ((char *)prev + block_get_size( prev ) != (char *)block)
+            err = "invalid previous block size";
     }
 
-    /* Check magic number */
-    if (pArena->magic != ARENA_INUSE_MAGIC && pArena->magic != ARENA_PENDING_MAGIC)
-    {
-        if (quiet == NOISY) {
-            ERR("Heap %p: invalid in-use arena magic %08x for %p\n", subheap->heap, pArena->magic, pArena );
-            if (TRACE_ON(heap))
-               heap_dump( subheap->heap );
-        }  else if (WARN_ON(heap)) {
-            WARN("Heap %p: invalid in-use arena magic %08x for %p\n", subheap->heap, pArena->magic, pArena );
-            if (TRACE_ON(heap))
-               heap_dump( subheap->heap );
-        }
-        return FALSE;
-    }
-    /* Check size flags */
-    if (pArena->size & ARENA_FLAG_FREE)
-    {
-        ERR("Heap %p: bad flags %08x for in-use arena %p\n",
-            subheap->heap, pArena->size & ~ARENA_SIZE_MASK, pArena );
-        return FALSE;
-    }
-    /* Check arena size */
-    size = pArena->size & ARENA_SIZE_MASK;
-    if ((const char *)(pArena + 1) + size > heapEnd ||
-        (const char *)(pArena + 1) + size < (const char *)(pArena + 1))
-    {
-        ERR("Heap %p: bad size %08lx for in-use arena %p\n", subheap->heap, size, pArena );
-        return FALSE;
-    }
-    /* Check next arena PREV_FREE flag */
-    if (((const char *)(pArena + 1) + size < heapEnd) &&
-        (*(const DWORD *)((const char *)(pArena + 1) + size) & ARENA_FLAG_PREV_FREE))
-    {
-        ERR("Heap %p: in-use arena %p next block %p has PREV_FREE flag %x\n",
-            subheap->heap, pArena, (const char *)(pArena + 1) + size,*(const DWORD *)((const char *)(pArena + 1) + size) );
-        return FALSE;
-    }
-    /* Check prev free arena */
-    if (pArena->size & ARENA_FLAG_PREV_FREE)
+    if (!err && block_get_type( block ) == ARENA_PENDING_MAGIC)
     {
-        const ARENA_FREE *pPrev = *((const ARENA_FREE * const*)pArena - 1);
-        /* Check prev pointer */
-        if (!HEAP_IsValidArenaPtr( subheap->heap, pPrev ))
-        {
-            ERR("Heap %p: bad back ptr %p for arena %p\n",
-                subheap->heap, pPrev, pArena );
-            return FALSE;
-        }
-        /* Check that prev arena is free */
-        if (!(pPrev->size & ARENA_FLAG_FREE) ||
-            (pPrev->magic != ARENA_FREE_MAGIC))
+        const char *ptr = (char *)(block + 1), *end = (char *)block + block_get_size( block );
+        while (!err && ptr < end)
         {
-            ERR("Heap %p: prev arena %p invalid for in-use %p\n",
-                subheap->heap, pPrev, pArena );
-            return FALSE;
-        }
-        /* Check that prev arena is really the previous block */
-        if ((const char *)(pPrev + 1) + (pPrev->size & ARENA_SIZE_MASK) != (const char *)pArena)
-        {
-            ERR("Heap %p: prev arena %p is not prev for in-use %p\n",
-                subheap->heap, pPrev, pArena );
-            return FALSE;
+            if (*(DWORD *)ptr != ARENA_FREE_FILLER) err = "free block overwritten";
+            ptr += sizeof(DWORD);
         }
     }
-    /* Check unused size */
-    if (pArena->unused_bytes > size)
+    else if (!err && (flags & HEAP_TAIL_CHECKING_ENABLED))
     {
-        ERR("Heap %p: invalid unused size %08x/%08lx\n", subheap->heap, pArena->unused_bytes, size );
-        return FALSE;
+        const unsigned char *tail = (unsigned char *)block + block_get_size( block ) - block->unused_bytes;
+        for (i = 0; !err && i < block->unused_bytes; i++) if (tail[i] != ARENA_TAIL_FILLER) err = "invalid block tail";
     }
-    /* Check unused bytes */
-    if (pArena->magic == ARENA_PENDING_MAGIC)
-    {
-        const DWORD *ptr = (const DWORD *)(pArena + 1);
-        const DWORD *end = (const DWORD *)((const char *)ptr + size);
 
-        while (ptr < end)
-        {
-            if (*ptr != ARENA_FREE_FILLER)
-            {
-                ERR("Heap %p: free block %p overwritten at %p by %08x\n",
-                    subheap->heap, pArena + 1, ptr, *ptr );
-                if (!*ptr) { heap_dump( subheap->heap ); DbgBreakPoint(); }
-                return FALSE;
-            }
-            ptr++;
-        }
-    }
-    else if (flags & HEAP_TAIL_CHECKING_ENABLED)
+    if (err)
     {
-        const unsigned char *data = (const unsigned char *)(pArena + 1) + size - pArena->unused_bytes;
-
-        for (i = 0; i < pArena->unused_bytes; i++)
-        {
-            if (data[i] == ARENA_TAIL_FILLER) continue;
-            ERR("Heap %p: block %p tail overwritten at %p (byte %u/%u == 0x%02x)\n",
-                subheap->heap, pArena + 1, data + i, i, pArena->unused_bytes, data[i] );
-            return FALSE;
-        }
+        ERR( "heap %p, block %p: %s\n", heap, block, err );
+        if (TRACE_ON(heap)) heap_dump( heap );
     }
-    return TRUE;
+
+    return !err;
 }
 
 
 static BOOL heap_validate_ptr( const HEAP *heap, const void *ptr, SUBHEAP **subheap )
 {
-    const ARENA_INUSE *arena = (const ARENA_INUSE *)ptr - 1;
+    const struct block *arena = (struct block *)ptr - 1;
     const ARENA_LARGE *large_arena;
 
     if (!(*subheap = find_subheap( heap, arena )) ||
@@ -1419,7 +1347,7 @@ static BOOL heap_validate_ptr( const HEAP *heap, const void *ptr, SUBHEAP **subh
         return validate_large_arena( heap, large_arena );
     }
 
-    return HEAP_ValidateInUseArena( *subheap, arena, QUIET );
+    return validate_used_block( *subheap, arena );
 }
 
 static BOOL heap_validate( HEAP *heap, BOOL quiet )
@@ -1447,7 +1375,7 @@ static BOOL heap_validate( HEAP *heap, BOOL quiet )
             }
             else
             {
-                if (!HEAP_ValidateInUseArena( subheap, (ARENA_INUSE *)ptr, NOISY )) return FALSE;
+                if (!validate_used_block( subheap, (ARENA_INUSE *)ptr )) return FALSE;
                 ptr += sizeof(ARENA_INUSE) + (*(DWORD *)ptr & ARENA_SIZE_MASK);
             }
         }
-- 
GitLab


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

[PATCH 5/5] ntdll: Simplify validate_free_block.

[Rémi Bernon]

From: Rémi Bernon <rbernon(a)codeweavers.com>

Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com>
---
 dlls/ntdll/heap.c | 170 +++++++++++++++-------------------------------
 1 file changed, 55 insertions(+), 115 deletions(-)

diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
index a4c3c8b0c9c..a23ab1a4c95 100644
--- a/dlls/ntdll/heap.c
+++ b/dlls/ntdll/heap.c
@@ -1143,131 +1143,71 @@ static ARENA_FREE *HEAP_FindFreeBlock( HEAP *heap, SIZE_T size,
 }
 
 
-/***********************************************************************
- *           HEAP_IsValidArenaPtr
- *
- * Check that the pointer is inside the range possible for arenas.
- */
-static BOOL HEAP_IsValidArenaPtr( const HEAP *heap, const ARENA_FREE *ptr )
+static BOOL is_valid_free_block( const HEAP *heap, const struct block *block )
 {
+    const SUBHEAP *subheap;
     unsigned int i;
-    const SUBHEAP *subheap = find_subheap( heap, ptr );
-    if (!subheap) return FALSE;
-    if ((const char *)ptr >= (const char *)subheap->base + subheap->headerSize) return TRUE;
+
+    if (!(subheap = find_subheap( heap, block ))) return FALSE;
+    if ((char *)block >= (char *)subheap->base + subheap->headerSize) return TRUE;
     if (subheap != &heap->subheap) return FALSE;
-    for (i = 0; i < HEAP_NB_FREE_LISTS; i++)
-        if (ptr == &heap->freeList[i].arena) return TRUE;
+    for (i = 0; i < HEAP_NB_FREE_LISTS; i++) if (block == (struct block *)&heap->freeList[i].arena) return TRUE;
     return FALSE;
 }
 
-
-/***********************************************************************
- *           HEAP_ValidateFreeArena
- */
-static BOOL HEAP_ValidateFreeArena( SUBHEAP *subheap, ARENA_FREE *pArena )
+static BOOL validate_free_block( const SUBHEAP *subheap, const struct block *block )
 {
-    DWORD flags = subheap->heap->flags;
-    SIZE_T size;
-    ARENA_FREE *prev, *next;
-    char *heapEnd = (char *)subheap->base + subheap->size;
-
-    /* Check for unaligned pointers */
-    if ((ULONG_PTR)pArena % ALIGNMENT != ARENA_OFFSET)
-    {
-        ERR("Heap %p: unaligned arena pointer %p\n", subheap->heap, pArena );
-        return FALSE;
-    }
+    const char *err = NULL, *base = subheap_base( subheap ), *commit_end = subheap_commit_end( subheap );
+    SIZE_T blocks_size = (char *)last_block( subheap ) - (char *)first_block( subheap );
+    const struct entry *entry = (struct entry *)block;
+    const struct block *prev, *next;
+    HEAP *heap = subheap->heap;
+    DWORD flags = heap->flags;
 
-    /* Check magic number */
-    if (pArena->magic != ARENA_FREE_MAGIC)
-    {
-        ERR("Heap %p: invalid free arena magic %08x for %p\n", subheap->heap, pArena->magic, pArena );
-        return FALSE;
-    }
-    /* Check size flags */
-    if (!(pArena->size & ARENA_FLAG_FREE) ||
-        (pArena->size & ARENA_FLAG_PREV_FREE))
-    {
-        ERR("Heap %p: bad flags %08x for free arena %p\n",
-            subheap->heap, pArena->size & ~ARENA_SIZE_MASK, pArena );
-        return FALSE;
-    }
-    /* Check arena size */
-    size = pArena->size & ARENA_SIZE_MASK;
-    if ((char *)(pArena + 1) + size > heapEnd)
-    {
-        ERR("Heap %p: bad size %08lx for free arena %p\n", subheap->heap, size, pArena );
-        return FALSE;
-    }
-    /* Check that next pointer is valid */
-    next = LIST_ENTRY( pArena->entry.next, ARENA_FREE, entry );
-    if (!HEAP_IsValidArenaPtr( subheap->heap, next ))
-    {
-        ERR("Heap %p: bad next ptr %p for arena %p\n",
-            subheap->heap, next, pArena );
-        return FALSE;
-    }
-    /* Check that next arena is free */
-    if (!(next->size & ARENA_FLAG_FREE) || (next->magic != ARENA_FREE_MAGIC))
-    {
-        ERR("Heap %p: next arena %p invalid for %p\n",
-            subheap->heap, next, pArena );
-        return FALSE;
-    }
-    /* Check that prev pointer is valid */
-    prev = LIST_ENTRY( pArena->entry.prev, ARENA_FREE, entry );
-    if (!HEAP_IsValidArenaPtr( subheap->heap, prev ))
-    {
-        ERR("Heap %p: bad prev ptr %p for arena %p\n",
-            subheap->heap, prev, pArena );
-        return FALSE;
-    }
-    /* Check that prev arena is free */
-    if (!(prev->size & ARENA_FLAG_FREE) || (prev->magic != ARENA_FREE_MAGIC))
-    {
-	/* this often means that the prev arena got overwritten
-	 * by a memory write before that prev arena */
-        ERR("Heap %p: prev arena %p invalid for %p\n",
-            subheap->heap, prev, pArena );
-        return FALSE;
-    }
-    /* Check that next block has PREV_FREE flag */
-    if ((char *)(pArena + 1) + size < heapEnd)
-    {
-        if (!(*(DWORD *)((char *)(pArena + 1) + size) & ARENA_FLAG_PREV_FREE))
-        {
-            ERR("Heap %p: free arena %p next block has no PREV_FREE flag\n",
-                subheap->heap, pArena );
-            return FALSE;
-        }
-        /* Check next block back pointer */
-        if (*((ARENA_FREE **)((char *)(pArena + 1) + size) - 1) != pArena)
+    if ((ULONG_PTR)(block + 1) % ALIGNMENT)
+        err = "invalid block alignment";
+    else if (!contains( first_block( subheap ), blocks_size, block, sizeof(*block) ))
+        err = "invalid block pointer";
+    else if (block_get_type( block ) != ARENA_FREE_MAGIC)
+        err = "invalid block header";
+    else if (!(block_get_flags( block ) & ARENA_FLAG_FREE) || (block_get_flags( block ) & ARENA_FLAG_PREV_FREE))
+        err = "invalid block flags";
+    else if (!contains( base, subheap_size( subheap ), block, block_get_size( block ) ))
+        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 ) & ARENA_FLAG_FREE) || block_get_type( next ) != ARENA_FREE_MAGIC)
+        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 ) & ARENA_FLAG_FREE) || block_get_type( prev ) != ARENA_FREE_MAGIC)
+        err = "invalid previous free block header";
+    else if ((next = next_block( subheap, (struct block *)block )))
+    {
+        if (!(block_get_flags( next ) & ARENA_FLAG_PREV_FREE))
+            err = "invalid next block flags";
+        if (*((struct block **)next - 1) != block)
+            err = "invalid next block back pointer";
+    }
+
+    if (!err && (flags & HEAP_FREE_CHECKING_ENABLED))
+    {
+        const char *ptr = (char *)(entry + 1), *end = (char *)block + block_get_size( block );
+        if (end > commit_end) end = commit_end;
+        while (!err && ptr < end)
         {
-            ERR("Heap %p: arena %p has wrong back ptr %p\n",
-                subheap->heap, pArena,
-                *((ARENA_FREE **)((char *)(pArena+1) + size) - 1));
-            return FALSE;
+            if (*(DWORD *)ptr != ARENA_FREE_FILLER) err = "free block overwritten";
+            ptr += sizeof(DWORD);
         }
     }
-    if (flags & HEAP_FREE_CHECKING_ENABLED)
-    {
-        DWORD *ptr = (DWORD *)(pArena + 1);
-        char *end = (char *)(pArena + 1) + size;
 
-        if (end >= heapEnd) end = (char *)subheap->base + subheap->commitSize;
-        else end -= sizeof(ARENA_FREE *);
-        while (ptr < (DWORD *)end)
-        {
-            if (*ptr != ARENA_FREE_FILLER)
-            {
-                ERR("Heap %p: free block %p overwritten at %p by %08x\n",
-                    subheap->heap, (ARENA_INUSE *)pArena + 1, ptr, *ptr );
-                return FALSE;
-            }
-            ptr++;
-        }
+    if (err)
+    {
+        ERR( "heap %p, block %p: %s\n", heap, block, err );
+        if (TRACE_ON(heap)) heap_dump( heap );
     }
-    return TRUE;
+
+    return !err;
 }
 
 
@@ -1297,7 +1237,7 @@ static BOOL validate_used_block( const SUBHEAP *subheap, const struct block *blo
     else if (block_get_flags( block ) & ARENA_FLAG_PREV_FREE)
     {
         const struct block *prev = *((struct block **)block - 1);
-        if (!HEAP_IsValidArenaPtr( heap, (struct entry *)prev ))
+        if (!is_valid_free_block( heap, prev ))
             err = "invalid previous block pointer";
         else if (!(block_get_flags( prev ) & ARENA_FLAG_FREE) || block_get_type( prev ) != ARENA_FREE_MAGIC)
             err = "invalid previous block flags";
@@ -1353,7 +1293,7 @@ static BOOL heap_validate_ptr( const HEAP *heap, const void *ptr, SUBHEAP **subh
 static BOOL heap_validate( HEAP *heap, BOOL quiet )
 {
     const ARENA_LARGE *large_arena;
-    SUBHEAP *subheap;
+    const SUBHEAP *subheap;
 
     LIST_FOR_EACH_ENTRY( subheap, &heap->subheap_list, SUBHEAP, entry )
     {
@@ -1370,7 +1310,7 @@ static BOOL heap_validate( HEAP *heap, BOOL quiet )
         {
             if (*(DWORD *)ptr & ARENA_FLAG_FREE)
             {
-                if (!HEAP_ValidateFreeArena( subheap, (ARENA_FREE *)ptr )) return FALSE;
+                if (!validate_free_block( subheap, (ARENA_INUSE *)ptr )) return FALSE;
                 ptr += sizeof(ARENA_FREE) + (*(DWORD *)ptr & ARENA_SIZE_MASK);
             }
             else
-- 
GitLab

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