[PATCH 0/3] MR1894: ntdll: heap verification improvements

[PATCH 1/3] ntdll: Check for delayed free block in heap_validate_ptr.

[Jinoh Kang]

From: Jinoh Kang

Today, the heap does not catch double free when HEAP_VALIDATE is
enabled, since validate_used_block() accepts BLOCK_TYPE_DEAD as a valid
block type.

Fix this by adding a explicit check that rejects BLOCK_TYPE_DEAD in
heap_validate_ptr.
---
 dlls/ntdll/heap.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
index aafbbd0f523..9f786dab6f0 100644
--- a/dlls/ntdll/heap.c
+++ b/dlls/ntdll/heap.c
@@ -1138,7 +1138,18 @@ static BOOL heap_validate_ptr( const struct heap *heap, const void *ptr )
         return validate_large_block( heap, block );
     }

-    return validate_used_block( heap, subheap, block );
+    if (!validate_used_block( heap, subheap, block )) return FALSE;
+
+    /* validate_used_block() has checked the alignment; the block is now safe(r) to dereference.
+     * Check if this an actually used block (instead of delayed freed block)
+     */
+    if (block_get_type( block ) != BLOCK_TYPE_USED)
+    {
+        ERR("heap %p, block %p: invalid block type %#x\n", heap, block, block_get_type( block ));
+        return FALSE;
+    }
+
+    return TRUE;
 }

static BOOL heap_validate( const struct heap *heap )

[PATCH 2/3] ntdll: Validate subheap's owner heap when validating heap.

[Jinoh Kang]

From: Jinoh Kang

---
 dlls/ntdll/heap.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
index 9f786dab6f0..70926109547 100644
--- a/dlls/ntdll/heap.c
+++ b/dlls/ntdll/heap.c
@@ -1138,6 +1138,12 @@ static BOOL heap_validate_ptr( const struct heap *heap, const void *ptr )
         return validate_large_block( heap, block );
     }

+    if (subheap->user_value != heap)
+    {
+        ERR("subheap %p: owner heap mismatch (expected %p, got %p)\n", subheap, heap, subheap->user_value);
+        return FALSE;
+    }
+
     if (!validate_used_block( heap, subheap, block )) return FALSE;

/* validate_used_block() has checked the alignment; the block is now safe(r) to dereference.
@@ -1167,6 +1173,12 @@ static BOOL heap_validate( const struct heap *heap )
             return FALSE;
         }

+        if (subheap->user_value != heap)
+        {
+            ERR("subheap %p: owner heap mismatch (expected %p, got %p)\n", subheap, heap, subheap->user_value);
+            return FALSE;
+        }
+
         for (block = first_block( subheap ); block; block = next_block( subheap, block ))
         {
             if (block_get_flags( block ) & BLOCK_FLAG_FREE)

[PATCH 3/3] ntdll: Validate blocks in the heap pending free request list.

[Jinoh Kang]

From: Jinoh Kang

---
 dlls/ntdll/heap.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c
index 70926109547..1c79d963399 100644
--- a/dlls/ntdll/heap.c
+++ b/dlls/ntdll/heap.c
@@ -1192,6 +1192,34 @@ static BOOL heap_validate( const struct heap *heap )
         }
     }

+    if (heap->pending_free)
+    {
+        unsigned int i, end;
+
+        for (i = 0; i < MAX_FREE_PENDING; i++)
+        {
+            if (!(block = heap->pending_free[i])) break;
+
+            subheap = block_get_subheap( heap, block );
+            if (!validate_used_block( heap, subheap, block )) return FALSE;
+            if (block_get_type( block ) != BLOCK_TYPE_DEAD)
+            {
+                WARN("heap %p, block %p: invalid block type %#x\n", heap, block, block_get_type( block ));
+                return FALSE;
+            }
+        }
+        end = i;
+
+        for (; i < MAX_FREE_PENDING; i++)
+        {
+            if ((block = heap->pending_free[i]))
+            {
+                WARN("heap %p: non-NULL delayed freed block %p at slot %u (list ends at slot %u)\n", heap, block, i, end);
+                return FALSE;
+            }
+        }
+    }
+
     LIST_FOR_EACH_ENTRY( large_arena, &heap->large_list, ARENA_LARGE, entry )
         if (!validate_large_block( heap, &large_arena->block )) return FALSE;

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/ntdll/heap.c:
>      return validate_large_block( heap, block );
>  }
>
> - return validate_used_block( heap, subheap, block );
> - if (!validate_used_block( heap, subheap, block )) return FALSE;
> -
> - /* validate_used_block() has checked the alignment; the block is now safe(r) to dereference.
> -
> * Check if this an actually used block (instead of delayed freed block)
> -
> */
> - if (block_get_type( block ) != BLOCK_TYPE_USED)
> - {
> -
>    ERR("heap %p, block %p: invalid block type %#x\n", heap, block, block_get_type( block ));
> -
>    return FALSE;
> - }
> -
> - return TRUE;

What about adding an `expect_type` parameter to `validate_used_block`, to conditionally check the type? Would be `BLOCK_TYPE_USED` here, `0` in `heap_validate` (to allow both types), and `BLOCK_TYPE_DEAD`, later, when checking the pending free list.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/ntdll/heap.c:
>      return validate_large_block( heap, block );
>  }
>
> - if (subheap->user_value != heap)
> - {
> -
>    ERR("subheap %p: owner heap mismatch (expected %p, got %p)\n", subheap, heap, subheap->user_value);
> -
>    return FALSE;
> - }

This could probably be done in `check_subheap`, extending it to not only check sizes.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/ntdll/heap.c:
>      }
>  }
>
> - if (heap->pending_free)
> - {
> -
>    unsigned int i, end;
> -
> -
>    for (i = 0; i < MAX_FREE_PENDING; i++)
> -
>    {
> -
>        if (!(block = heap->pending_free[i])) break;
> -
> -
>        subheap = block_get_subheap( heap, block );

This should probably call `find_subheap` to be more robust.

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/ntdll/heap.c:
> -
> -
>        subheap = block_get_subheap( heap, block );
> -
>        if (!validate_used_block( heap, subheap, block )) return FALSE;
> -
>        if (block_get_type( block ) != BLOCK_TYPE_DEAD)
> -
>        {
> -
>            WARN("heap %p, block %p: invalid block type %#x\n", heap, block, block_get_type( block ));
> -
>            return FALSE;
> -
>        }
> -
>    }
> -
>    end = i;
> -
> -
>    for (; i < MAX_FREE_PENDING; i++)
> -
>    {
> -
>        if ((block = heap->pending_free[i]))
> -
>        {
> -
>            WARN("heap %p: non-NULL delayed freed block %p at slot %u (list ends at slot %u)\n", heap, block, i, end);

I'm not sure the detailed message with `end` variable is very useful. Just do `if (TRACE_ON(heap)) heap_dump( heap );` instead.