Re: [2/4] ntdll: add support for the HEAP_FREE_CHECKING_ENABLED style of heap overrun detection.

[Alexandre Julliard]

Dan Kegel <dank(a)kegel.com> writes:
> @@ -85,6 +85,7 @@ typedef struct
>
> #define ARENA_INUSE_FILLER     0x55
>  #define ARENA_FREE_FILLER      0xaa
> +#define ARENA_PAD_FILLER(address) (((int)(address) & 1) ? 0xee : 0xfe)

Don't cast addresses to int.
> +static PVOID HEAP_ValidateTail( const HEAP *heap, const void *arena, const void *p, SIZE_T size, SIZE_T filler_size )
> +{
> +    unsigned char *tail = ((unsigned char *)p) + size;
> +    int j=0;

Please use correct types. j should be a SIZE_T, tail should be a const
pointer.
> @@ -1477,7 +1561,9 @@ BOOLEAN WINAPI RtlFreeHeap( HANDLE heap, ULONG flags, PVOID ptr )
>      pInUse  = (ARENA_INUSE *)ptr - 1;
>      if (!(subheap = HEAP_FindSubHeap( heapPtr, pInUse )))
>      {
> -        if (!find_large_block( heapPtr, ptr )) goto error;
> +        ARENA_LARGE *arena = find_large_block( heapPtr, ptr );
> +        if (!arena) goto error;
> +        if (!validate_large_arena( heapPtr, arena, NOISY )) goto error;

Validation should only happen when explicitly requested.
> diff --git a/dlls/ntdll/tests/heap.c b/dlls/ntdll/tests/heap.c
> new file mode 100644
> index 0000000..7cfa554
> --- /dev/null
> +++ b/dlls/ntdll/tests/heap.c

The tests should go with the existing heap tests in kernel32.
> +static void test_free_overrun(void)
> +{
> +    if ((pRtlGetNtGlobalFlags() & FLG_HEAP_ENABLE_FREE_CHECK) == 0) {
> +        skip("\
> +Skipping heap free padding overrun test.  To enable:\n\
> +on Wine, use gflags to enable global heap free checking, or set the following key:\n\
> +[HKEY_LOCAL_MACHINE\\System\\CurrentControlSet\\Control\\Session Manager]\n\
> +\"GlobalFlag\"=dword:00000020\n\
> +On Windows, use gflags to enable heap free checking for ntdll_test.exe.\n");
> +        return;

You should create your own heap with checking enabled so that it can be
tested in all cases.

Also the patch is still too large, changes to sensitive areas like the
heap code should be very small and focused.

-- 
Alexandre Julliard
julliard(a)winehq.org

[2/4] ntdll: add support for the HEAP_FREE_CHECKING_ENABLED style of heap overrun detection.

[Dan Kegel]

On Tue, Dec 1, 2009 at 11:22 AM, Alexandre Julliard <julliard(a)winehq.org> wrote:
> You should create your own heap with checking enabled so that it can be
> tested in all cases.

I couldn't figure out how to do that on Windows Vista, but I can sure
do it at least for Wine.

[2/4] ntdll: add support for the HEAP_FREE_CHECKING_ENABLED style of heap overrun detection.

[Dan Kegel]

On Tue, Dec 1, 2009 at 11:22 AM, Alexandre Julliard <julliard(a)winehq.org> wrote:
> Validation should only happen when explicitly requested.

Should I submit a separate patch to remove the existing small arena
validation from RtlFreeHeap and RtlReAllocateHeap, then?

Or is that how Windows XP behaved (validating small heap
blocks but not large ones)?

[2/4] ntdll: add support for the HEAP_FREE_CHECKING_ENABLED style of heap overrun detection.

[Alexandre Julliard]

Dan Kegel <dank(a)kegel.com> writes:
> On Tue, Dec 1, 2009 at 11:22 AM, Alexandre Julliard <julliard(a)winehq.org> wrote:
> > Validation should only happen when explicitly requested.
>
> Should I submit a separate patch to remove the existing small arena
> validation from RtlFreeHeap and RtlReAllocateHeap, then?

No. These are not here as debug aids, but to catch some cases of bad
pointers passed from apps that could cause heap corruption. There's no
evidence that the same thing is necessary for large blocks.

-- 
Alexandre Julliard
julliard(a)winehq.org