-- v2: ntdll: Remove redundant WARN_ON(heap) check. ntdll: Validate blocks in the heap pending free request list. ntdll: Validate subheap's owner heap when validating heap. ntdll: Check for delayed free block in heap_validate_ptr.
From: Jinoh Kang jinoh.kang.kr@gmail.com
Today, the heap does not catch double free when both HEAP_VALIDATE and HEAP_FREE_CHECKING_ENABLED are on, since validate_used_block() accepts BLOCK_TYPE_DEAD as a valid (allocated) block type.
Fix this by adding an explicit check that rejects BLOCK_TYPE_DEAD in heap_validate_ptr. --- dlls/ntdll/heap.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index aafbbd0f523..523344fe0f5 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -1067,7 +1067,8 @@ static BOOL validate_free_block( const struct heap *heap, const SUBHEAP *subheap }
-static BOOL validate_used_block( const struct heap *heap, const SUBHEAP *subheap, const struct block *block ) +static BOOL validate_used_block( const struct heap *heap, const SUBHEAP *subheap, const struct block *block, + unsigned int expected_block_type ) { const char *err = NULL, *base = subheap_base( subheap ), *commit_end = subheap_commit_end( subheap ); DWORD flags = heap->flags; @@ -1078,6 +1079,8 @@ static BOOL validate_used_block( const struct heap *heap, const SUBHEAP *subheap err = "invalid block BLOCK_ALIGN"; else if (block_get_type( block ) != BLOCK_TYPE_USED && block_get_type( block ) != BLOCK_TYPE_DEAD) err = "invalid block header"; + else if (expected_block_type && block_get_type( block ) != expected_block_type) + err = "invalid block type"; else if (block_get_flags( block ) & BLOCK_FLAG_FREE) err = "invalid block flags"; else if (!contains( base, commit_end - base, block, block_get_size( block ) )) @@ -1138,7 +1141,7 @@ 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 ); + return validate_used_block( heap, subheap, block, BLOCK_TYPE_USED ); }
static BOOL heap_validate( const struct heap *heap ) @@ -1164,7 +1167,7 @@ static BOOL heap_validate( const struct heap *heap ) } else { - if (!validate_used_block( heap, subheap, block )) return FALSE; + if (!validate_used_block( heap, subheap, block, 0 )) return FALSE; } } }
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- dlls/ntdll/heap.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 523344fe0f5..3c892e7fffc 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -338,8 +338,10 @@ static inline struct block *next_block( const SUBHEAP *subheap, const struct blo return (struct block *)next; }
-static inline BOOL check_subheap( const SUBHEAP *subheap ) +static inline BOOL check_subheap( const SUBHEAP *subheap, const struct heap *heap ) { + if (heap && subheap->user_value != heap) return FALSE; + return contains( &subheap->block, subheap->block_size, subheap + 1, subheap->data_size ); }
@@ -453,7 +455,7 @@ static void valgrind_notify_free_all( SUBHEAP *subheap ) struct block *block;
if (!RUNNING_ON_VALGRIND) return; - if (!check_subheap( subheap )) return; + if (!check_subheap( subheap, NULL )) return;
for (block = first_block( subheap ); block; block = next_block( subheap, block )) { @@ -549,7 +551,7 @@ static void heap_dump( const struct heap *heap ) TRACE( " %p: base %p first %p last %p end %p\n", subheap, base, first_block( subheap ), last_block( subheap ), base + subheap_size( subheap ) );
- if (!check_subheap( subheap )) return; + if (!check_subheap( subheap, heap )) return;
overhead += subheap_overhead( subheap ); for (block = first_block( subheap ); block; block = next_block( subheap, block )) @@ -648,7 +650,7 @@ static SUBHEAP *find_subheap( const struct heap *heap, const struct block *block LIST_FOR_EACH_ENTRY( subheap, &heap->subheap_list, SUBHEAP, entry ) { SIZE_T blocks_size = (char *)last_block( subheap ) - (char *)first_block( subheap ); - if (!check_subheap( subheap )) return NULL; + if (!check_subheap( subheap, heap )) return NULL; if (contains( first_block( subheap ), blocks_size, block, sizeof(*block) )) return subheap; /* outside of blocks region, possible corruption or heap_walk */ if (contains( subheap_base( subheap ), subheap_size( subheap ), block, 0 )) return heap_walk ? subheap : NULL; @@ -1152,9 +1154,9 @@ static BOOL heap_validate( const struct heap *heap )
LIST_FOR_EACH_ENTRY( subheap, &heap->subheap_list, SUBHEAP, entry ) { - if (!check_subheap( subheap )) + if (!check_subheap( subheap, heap )) { - ERR( "heap %p, subheap %p corrupted sizes\n", heap, subheap ); + ERR( "heap %p, subheap %p corrupted sizes or user_value\n", heap, subheap ); if (TRACE_ON(heap)) heap_dump( heap ); return FALSE; } @@ -1271,7 +1273,7 @@ static void heap_set_debug_flags( HANDLE handle ) { const char *commit_end = subheap_commit_end( subheap );
- if (!check_subheap( subheap )) break; + if (!check_subheap( subheap, heap )) break;
for (block = first_block( subheap ); block; block = next_block( subheap, block )) {
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- dlls/ntdll/heap.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 3c892e7fffc..9a0f9c57c78 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -1174,6 +1174,29 @@ static BOOL heap_validate( const struct heap *heap ) } }
+ if (heap->pending_free) + { + unsigned int i; + + for (i = 0; i < MAX_FREE_PENDING; i++) + { + if (!(block = heap->pending_free[i])) break; + + subheap = find_subheap( heap, block, FALSE ); + if (!validate_used_block( heap, subheap, block, BLOCK_TYPE_DEAD )) return FALSE; + } + + for (; i < MAX_FREE_PENDING; i++) + { + if ((block = heap->pending_free[i])) + { + WARN("heap %p: unexpected delayed freed block %p at slot %u\n", heap, block, i); + if (TRACE_ON(heap)) heap_dump( heap ); + return FALSE; + } + } + } + LIST_FOR_EACH_ENTRY( large_arena, &heap->large_list, ARENA_LARGE, entry ) if (!validate_large_block( heap, &large_arena->block )) return FALSE;
From: Jinoh Kang jinoh.kang.kr@gmail.com
--- dlls/ntdll/heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 9a0f9c57c78..876948c6269 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -1136,7 +1136,7 @@ static BOOL heap_validate_ptr( const struct heap *heap, const void *ptr ) { if (!find_large_block( heap, block )) { - if (WARN_ON(heap)) WARN("heap %p, ptr %p: block region not found\n", heap, ptr ); + WARN("heap %p, ptr %p: block region not found\n", heap, ptr ); return FALSE; }
Rémi Bernon (@rbernon) commented about dlls/ntdll/heap.c:
struct block *block; if (!RUNNING_ON_VALGRIND) return;
- if (!check_subheap( subheap )) return;
- if (!check_subheap( subheap, NULL )) return;
I think it's not much more change to pass the heap pointer here too, and then remove the `heap &&` in `check_subheap`.
Rémi Bernon (@rbernon) commented about dlls/ntdll/heap.c:
} }
- if (heap->pending_free)
- {
unsigned int i;for (i = 0; i < MAX_FREE_PENDING; i++){if (!(block = heap->pending_free[i])) break;subheap = find_subheap( heap, block, FALSE );
You're using `find_subheap` but then it may be `NULL` if it's invalid, and you should fail early instead of using the `NULL` subheap pointer in `validate_used_block`.
-
Jinoh Kang -
Jinoh Kang (＠iamahuman)
-
Rémi Bernon (＠rbernon)