[PATCH 0/6] MR55: ntdll: Factor out heap status handling and locking.
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/heap.c | 84 +++++++++++++++++++++++------------------------ 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index d7ac44a4247..10140fed339 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -181,7 +181,7 @@ typedef struct tagHEAP static HEAP *processHeap; /* main process heap */ -static BOOL HEAP_IsRealArena( HEAP *heapPtr, DWORD flags, LPCVOID block, BOOL quiet ); +static BOOL HEAP_IsRealArena( HEAP *heapPtr, LPCVOID block, BOOL quiet ); /* mark a block of memory as free for debugging purposes */ static inline void mark_block_free( void *ptr, SIZE_T size, DWORD flags ) @@ -470,21 +470,27 @@ static HEAP *HEAP_GetPtr( HANDLE heap /* [in] Handle to the heap */ ) { HEAP *heapPtr = heap; + BOOL valid = TRUE; + if (!heapPtr || (heapPtr->magic != HEAP_MAGIC)) { ERR("Invalid heap %p!\n", heap ); return NULL; } - if ((heapPtr->flags & HEAP_VALIDATE_ALL) && !HEAP_IsRealArena( heapPtr, 0, NULL, NOISY )) + if (heapPtr->flags & HEAP_VALIDATE_ALL) { - if (TRACE_ON(heap)) + heap_lock( heapPtr, 0 ); + valid = HEAP_IsRealArena( heapPtr, NULL, NOISY ); + heap_unlock( heapPtr, 0 ); + + if (!valid && TRACE_ON(heap)) { HEAP_Dump( heapPtr ); assert( FALSE ); } - return NULL; } - return heapPtr; + + return valid ? heapPtr : NULL; } @@ -1346,64 +1352,58 @@ static BOOL HEAP_ValidateInUseArena( const SUBHEAP *subheap, const ARENA_INUSE * * TRUE: Success * FALSE: Failure */ -static BOOL HEAP_IsRealArena( HEAP *heapPtr, /* [in] ptr to the heap */ - DWORD flags, /* [in] Bit flags that control access during operation */ +static BOOL HEAP_IsRealArena( HEAP *heap, /* [in] ptr to the heap */ LPCVOID block, /* [in] Optional pointer to memory block to validate */ BOOL quiet ) /* [in] Flag - if true, HEAP_ValidateInUseArena * does not complain */ { SUBHEAP *subheap; - BOOL ret = FALSE; const ARENA_LARGE *large_arena; - heap_lock( heapPtr, flags ); - if (block) /* only check this single memory block */ { const ARENA_INUSE *arena = (const ARENA_INUSE *)block - 1; - if (!(subheap = HEAP_FindSubHeap( heapPtr, arena )) || + if (!(subheap = HEAP_FindSubHeap( heap, arena )) || ((const char *)arena < (char *)subheap->base + subheap->headerSize)) { - if (!(large_arena = find_large_block( heapPtr, block ))) + if (!(large_arena = find_large_block( heap, block ))) { if (quiet == NOISY) - ERR("Heap %p: block %p is not inside heap\n", heapPtr, block ); + ERR("Heap %p: block %p is not inside heap\n", heap, block ); else if (WARN_ON(heap)) - WARN("Heap %p: block %p is not inside heap\n", heapPtr, block ); + WARN("Heap %p: block %p is not inside heap\n", heap, block ); + return FALSE; } - else ret = validate_large_arena( heapPtr, large_arena, quiet ); + + return validate_large_arena( heap, large_arena, quiet ); } - else ret = HEAP_ValidateInUseArena( subheap, arena, quiet ); - goto done; + + return HEAP_ValidateInUseArena( subheap, arena, quiet ); } - LIST_FOR_EACH_ENTRY( subheap, &heapPtr->subheap_list, SUBHEAP, entry ) + LIST_FOR_EACH_ENTRY( subheap, &heap->subheap_list, SUBHEAP, entry ) { char *ptr = (char *)subheap->base + subheap->headerSize; while (ptr < (char *)subheap->base + subheap->size) { if (*(DWORD *)ptr & ARENA_FLAG_FREE) { - if (!HEAP_ValidateFreeArena( subheap, (ARENA_FREE *)ptr )) goto done; + if (!HEAP_ValidateFreeArena( subheap, (ARENA_FREE *)ptr )) return FALSE; ptr += sizeof(ARENA_FREE) + (*(DWORD *)ptr & ARENA_SIZE_MASK); } else { - if (!HEAP_ValidateInUseArena( subheap, (ARENA_INUSE *)ptr, NOISY )) goto done; + if (!HEAP_ValidateInUseArena( subheap, (ARENA_INUSE *)ptr, NOISY )) return FALSE; ptr += sizeof(ARENA_INUSE) + (*(DWORD *)ptr & ARENA_SIZE_MASK); } } } - LIST_FOR_EACH_ENTRY( large_arena, &heapPtr->large_list, ARENA_LARGE, entry ) - if (!validate_large_arena( heapPtr, large_arena, quiet )) goto done; + LIST_FOR_EACH_ENTRY( large_arena, &heap->large_list, ARENA_LARGE, entry ) + if (!validate_large_arena( heap, large_arena, quiet )) return FALSE; - ret = TRUE; - -done: - heap_unlock( heapPtr, flags ); - return ret; + return TRUE; } @@ -2076,23 +2076,23 @@ SIZE_T WINAPI RtlSizeHeap( HANDLE heap, ULONG flags, const void *ptr ) /*********************************************************************** * RtlValidateHeap (NTDLL.@) - * - * Determine if a block is a valid allocation from a heap. - * - * PARAMS - * heap [I] Heap that block was allocated from - * flags [I] HEAP_ flags from "winnt.h" - * ptr [I] Block to check - * - * RETURNS - * Success: TRUE. The block was allocated from heap. - * Failure: FALSE, if heap is invalid or ptr was not allocated from it. */ -BOOLEAN WINAPI RtlValidateHeap( HANDLE heap, ULONG flags, LPCVOID ptr ) +BOOLEAN WINAPI RtlValidateHeap( HANDLE heap, ULONG flags, const void *ptr ) { - HEAP *heapPtr = HEAP_GetPtr( heap ); - if (!heapPtr) return FALSE; - return HEAP_IsRealArena( heapPtr, flags, ptr, QUIET ); + HEAP *heapPtr; + BOOLEAN ret; + + if (!(heapPtr = HEAP_GetPtr( heap ))) + ret = FALSE; + else + { + heap_lock( heapPtr, flags ); + ret = HEAP_IsRealArena( heapPtr, ptr, QUIET ); + heap_unlock( heapPtr, flags ); + } + + TRACE( "heap %p, flags %#x, ptr %p, return %u.\n", heap, flags, ptr, !!ret ); + return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/55
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/heap.c | 53 +++++++++++++++++++---------------------------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 10140fed339..cd7e44cc717 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -181,7 +181,7 @@ typedef struct tagHEAP static HEAP *processHeap; /* main process heap */ -static BOOL HEAP_IsRealArena( HEAP *heapPtr, LPCVOID block, BOOL quiet ); +static BOOL heap_validate( HEAP *heap, BOOL quiet ); /* mark a block of memory as free for debugging purposes */ static inline void mark_block_free( void *ptr, SIZE_T size, DWORD flags ) @@ -480,7 +480,7 @@ static HEAP *HEAP_GetPtr( if (heapPtr->flags & HEAP_VALIDATE_ALL) { heap_lock( heapPtr, 0 ); - valid = HEAP_IsRealArena( heapPtr, NULL, NOISY ); + valid = heap_validate( heapPtr, NOISY ); heap_unlock( heapPtr, 0 ); if (!valid && TRACE_ON(heap)) @@ -1344,44 +1344,32 @@ static BOOL HEAP_ValidateInUseArena( const SUBHEAP *subheap, const ARENA_INUSE * } -/*********************************************************************** - * HEAP_IsRealArena [Internal] - * Validates a block is a valid arena. - * - * RETURNS - * TRUE: Success - * FALSE: Failure - */ -static BOOL HEAP_IsRealArena( HEAP *heap, /* [in] ptr to the heap */ - LPCVOID block, /* [in] Optional pointer to memory block to validate */ - BOOL quiet ) /* [in] Flag - if true, HEAP_ValidateInUseArena - * does not complain */ +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 (block) /* only check this single memory block */ + if (!(subheap = HEAP_FindSubHeap( heap, arena )) || + ((const char *)arena < (char *)subheap->base + subheap->headerSize)) { - const ARENA_INUSE *arena = (const ARENA_INUSE *)block - 1; - - if (!(subheap = HEAP_FindSubHeap( heap, arena )) || - ((const char *)arena < (char *)subheap->base + subheap->headerSize)) + if (!(large_arena = find_large_block( heap, ptr ))) { - if (!(large_arena = find_large_block( heap, block ))) - { - if (quiet == NOISY) - ERR("Heap %p: block %p is not inside heap\n", heap, block ); - else if (WARN_ON(heap)) - WARN("Heap %p: block %p is not inside heap\n", heap, block ); - return FALSE; - } - - return validate_large_arena( heap, large_arena, quiet ); + if (WARN_ON(heap)) WARN("heap %p, ptr %p is not inside heap\n", heap, ptr ); + return FALSE; } - return HEAP_ValidateInUseArena( subheap, arena, quiet ); + return validate_large_arena( heap, large_arena, QUIET ); } + return HEAP_ValidateInUseArena( subheap, arena, QUIET ); +} + +static BOOL heap_validate( HEAP *heap, BOOL quiet ) +{ + const ARENA_LARGE *large_arena; + SUBHEAP *subheap; + LIST_FOR_EACH_ENTRY( subheap, &heap->subheap_list, SUBHEAP, entry ) { char *ptr = (char *)subheap->base + subheap->headerSize; @@ -2087,7 +2075,8 @@ BOOLEAN WINAPI RtlValidateHeap( HANDLE heap, ULONG flags, const void *ptr ) else { heap_lock( heapPtr, flags ); - ret = HEAP_IsRealArena( heapPtr, ptr, QUIET ); + if (ptr) ret = heap_validate_ptr( heapPtr, ptr ); + else ret = heap_validate( heapPtr, QUIET ); heap_unlock( heapPtr, flags ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/55
From: Rémi Bernon <rbernon(a)codeweavers.com> Factoring out locking and status handling. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/kernel32/tests/heap.c | 1 - dlls/ntdll/heap.c | 84 ++++++++++++++++---------------------- 2 files changed, 36 insertions(+), 49 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index cc02140f453..2628da9cec3 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -482,7 +482,6 @@ static void test_HeapCreate(void) SetLastError( 0xdeadbeef ); ptr1 = HeapAlloc( heap, 0, 4 * alloc_size ); ok( !ptr1, "HeapAlloc succeeded\n" ); - todo_wine ok( GetLastError() == ERROR_NOT_ENOUGH_MEMORY, "got error %lu\n", GetLastError() ); ret = HeapFree( heap, 0, ptr1 ); ok( ret, "HeapFree failed, error %lu\n", GetLastError() ); diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index cd7e44cc717..f3dd6ab6c67 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -349,6 +349,12 @@ static void heap_unlock( HEAP *heap, ULONG flags ) RtlLeaveCriticalSection( &heap->cs ); } +static void heap_set_status( const HEAP *heap, ULONG flags, NTSTATUS status ) +{ + if (status == STATUS_NO_MEMORY && (flags & HEAP_GENERATE_EXCEPTIONS)) RtlRaiseStatus( status ); + if (status) RtlSetLastWin32ErrorAndNtStatusFromNtStatus( status ); +} + /*********************************************************************** * HEAP_Dump */ @@ -1648,66 +1654,27 @@ HANDLE WINAPI RtlDestroyHeap( HANDLE heap ) return 0; } - -/*********************************************************************** - * RtlAllocateHeap (NTDLL.@) - * - * Allocate a memory block from a Heap. - * - * PARAMS - * heap [I] Heap to allocate block from - * flags [I] HEAP_ flags from "winnt.h" - * size [I] Size of the memory block to allocate - * - * RETURNS - * Success: A pointer to the newly allocated block - * Failure: NULL. - * - * NOTES - * This call does not SetLastError(). - */ -void * WINAPI DECLSPEC_HOTPATCH RtlAllocateHeap( HANDLE heap, ULONG flags, SIZE_T size ) +static NTSTATUS heap_allocate( HEAP *heap, ULONG flags, SIZE_T size, void **ret ) { ARENA_FREE *pArena; ARENA_INUSE *pInUse; SUBHEAP *subheap; - HEAP *heapPtr = HEAP_GetPtr( heap ); SIZE_T rounded_size; - /* Validate the parameters */ - - if (!heapPtr) return NULL; - flags &= HEAP_GENERATE_EXCEPTIONS | HEAP_NO_SERIALIZE | HEAP_ZERO_MEMORY; - flags |= heapPtr->flags; rounded_size = ROUND_SIZE(size) + HEAP_TAIL_EXTRA_SIZE( flags ); - if (rounded_size < size) /* overflow */ - { - if (flags & HEAP_GENERATE_EXCEPTIONS) RtlRaiseStatus( STATUS_NO_MEMORY ); - return NULL; - } - if (rounded_size < HEAP_MIN_DATA_SIZE) rounded_size = HEAP_MIN_DATA_SIZE; - heap_lock( heapPtr, flags ); + if (rounded_size < size) return STATUS_NO_MEMORY; /* overflow */ + if (rounded_size < HEAP_MIN_DATA_SIZE) rounded_size = HEAP_MIN_DATA_SIZE; if (rounded_size >= HEAP_MIN_LARGE_BLOCK_SIZE && (flags & HEAP_GROWABLE)) { - void *ret = allocate_large_block( heap, flags, size ); - heap_unlock( heapPtr, flags ); - if (!ret && (flags & HEAP_GENERATE_EXCEPTIONS)) RtlRaiseStatus( STATUS_NO_MEMORY ); - TRACE("(%p,%08x,%08lx): returning %p\n", heap, flags, size, ret ); - return ret; + if (!(*ret = allocate_large_block( heap, flags, size ))) return STATUS_NO_MEMORY; + return STATUS_SUCCESS; } /* Locate a suitable free block */ - if (!(pArena = HEAP_FindFreeBlock( heapPtr, rounded_size, &subheap ))) - { - TRACE("(%p,%08x,%08lx): returning NULL\n", - heap, flags, size ); - heap_unlock( heapPtr, flags ); - if (flags & HEAP_GENERATE_EXCEPTIONS) RtlRaiseStatus( STATUS_NO_MEMORY ); - return NULL; - } + if (!(pArena = HEAP_FindFreeBlock( heap, rounded_size, &subheap ))) return STATUS_NO_MEMORY; /* Remove the arena from the free list */ @@ -1730,10 +1697,31 @@ void * WINAPI DECLSPEC_HOTPATCH RtlAllocateHeap( HANDLE heap, ULONG flags, SIZE_ notify_alloc( pInUse + 1, size, flags & HEAP_ZERO_MEMORY ); initialize_block( pInUse + 1, size, pInUse->unused_bytes, flags ); - heap_unlock( heapPtr, flags ); + *ret = pInUse + 1; + return STATUS_SUCCESS; +} + +/*********************************************************************** + * RtlAllocateHeap (NTDLL.@) + */ +void *WINAPI DECLSPEC_HOTPATCH RtlAllocateHeap( HANDLE heap, ULONG flags, SIZE_T size ) +{ + void *ptr = NULL; + NTSTATUS status; + HEAP *heapPtr; + + if (!(heapPtr = HEAP_GetPtr( heap ))) + status = STATUS_INVALID_HANDLE; + else + { + heap_lock( heapPtr, flags ); + status = heap_allocate( heapPtr, heap_get_flags( heapPtr, flags ), size, &ptr ); + heap_unlock( heapPtr, flags ); + } - TRACE("(%p,%08x,%08lx): returning %p\n", heap, flags, size, pInUse + 1 ); - return pInUse + 1; + TRACE( "heap %p, flags %#x, size %#Ix, return %p, status %#x.\n", heap, flags, size, ptr, status ); + heap_set_status( heapPtr, flags, status ); + return ptr; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/55
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=114429 Your paranoid android. === debian11 (32 bit report) === kernel32: heap.c:418: Test succeeded inside todo block: got error 8 === debian11 (32 bit Arabic:Morocco report) === kernel32: heap.c:418: Test succeeded inside todo block: got error 8 === debian11 (32 bit German report) === kernel32: heap.c:418: Test succeeded inside todo block: got error 8 === debian11 (32 bit French report) === kernel32: heap.c:418: Test succeeded inside todo block: got error 8 === debian11 (32 bit Hebrew:Israel report) === kernel32: heap.c:418: Test succeeded inside todo block: got error 8 === debian11 (32 bit Hindi:India report) === kernel32: heap.c:418: Test succeeded inside todo block: got error 8 === debian11 (32 bit Japanese:Japan report) === kernel32: heap.c:418: Test succeeded inside todo block: got error 8 === debian11 (32 bit Chinese:China report) === kernel32: heap.c:418: Test succeeded inside todo block: got error 8 ntdll: virtual: Timeout === debian11 (32 bit WoW report) === kernel32: heap.c:418: Test succeeded inside todo block: got error 8 === debian11 (64 bit WoW report) === kernel32: heap.c:418: Test succeeded inside todo block: got error 8
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/kernel32/tests/heap.c | 2 ++ dlls/ntdll/heap.c | 71 +++++++++++++++----------------------- 2 files changed, 30 insertions(+), 43 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 2628da9cec3..8a7260303d3 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -229,6 +229,8 @@ static void test_HeapCreate(void) /* test some border cases */ + ret = HeapFree( NULL, 0, NULL ); + ok( ret, "HeapFree failed, error %lu\n", GetLastError() ); ret = HeapFree( heap, 0, NULL ); ok( ret, "HeapFree failed, error %lu\n", GetLastError() ); #if 0 /* crashes */ diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index f3dd6ab6c67..1a4d5b1a2bb 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -1725,60 +1725,45 @@ void *WINAPI DECLSPEC_HOTPATCH RtlAllocateHeap( HANDLE heap, ULONG flags, SIZE_T } -/*********************************************************************** - * RtlFreeHeap (NTDLL.@) - * - * Free a memory block allocated with RtlAllocateHeap(). - * - * PARAMS - * heap [I] Heap that block was allocated from - * flags [I] HEAP_ flags from "winnt.h" - * ptr [I] Block to free - * - * RETURNS - * Success: TRUE, if ptr is NULL or was freed successfully. - * Failure: FALSE. - */ -BOOLEAN WINAPI DECLSPEC_HOTPATCH RtlFreeHeap( HANDLE heap, ULONG flags, void *ptr ) +static NTSTATUS heap_free( HEAP *heap, void *ptr ) { - ARENA_INUSE *pInUse; + ARENA_INUSE *block; SUBHEAP *subheap; - HEAP *heapPtr; - /* Validate the parameters */ + /* Inform valgrind we are trying to free memory, so it can throw up an error message */ + notify_free( ptr ); - if (!ptr) return TRUE; /* freeing a NULL ptr isn't an error in Win2k */ + block = (ARENA_INUSE *)ptr - 1; + if (!validate_block_pointer( heap, &subheap, block )) return STATUS_INVALID_PARAMETER; - heapPtr = HEAP_GetPtr( heap ); - if (!heapPtr) - { - RtlSetLastWin32ErrorAndNtStatusFromNtStatus( STATUS_INVALID_HANDLE ); - return FALSE; - } + if (!subheap) free_large_block( heap, ptr ); + else HEAP_MakeInUseBlockFree( subheap, block ); - heap_lock( heapPtr, flags ); + return STATUS_SUCCESS; +} - /* Inform valgrind we are trying to free memory, so it can throw up an error message */ - notify_free( ptr ); +/*********************************************************************** + * RtlFreeHeap (NTDLL.@) + */ +BOOLEAN WINAPI DECLSPEC_HOTPATCH RtlFreeHeap( HANDLE heap, ULONG flags, void *ptr ) +{ + NTSTATUS status; + HEAP *heapPtr; - /* Some sanity checks */ - pInUse = (ARENA_INUSE *)ptr - 1; - if (!validate_block_pointer( heapPtr, &subheap, pInUse )) goto error; + if (!ptr) return TRUE; - if (!subheap) - free_large_block( heapPtr, ptr ); + if (!(heapPtr = HEAP_GetPtr( heap ))) + status = STATUS_INVALID_PARAMETER; else - HEAP_MakeInUseBlockFree( subheap, pInUse ); - - heap_unlock( heapPtr, flags ); - TRACE("(%p,%08x,%p): returning TRUE\n", heap, flags, ptr ); - return TRUE; + { + heap_lock( heapPtr, flags ); + status = heap_free( heapPtr, ptr ); + heap_unlock( heapPtr, flags ); + } -error: - heap_unlock( heapPtr, flags ); - RtlSetLastWin32ErrorAndNtStatusFromNtStatus( STATUS_INVALID_PARAMETER ); - TRACE("(%p,%08x,%p): returning FALSE\n", heap, flags, ptr ); - return FALSE; + TRACE( "heap %p, flags %#x, ptr %p, return %u, status %#x.\n", heap, flags, ptr, !status, status ); + heap_set_status( heapPtr, flags, status ); + return !status; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/55
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=114430 Your paranoid android. === debian11 (32 bit report) === kernel32: heap.c:420: Test succeeded inside todo block: got error 8 === debian11 (32 bit Arabic:Morocco report) === kernel32: heap.c:420: Test succeeded inside todo block: got error 8 === debian11 (32 bit German report) === kernel32: heap.c:420: Test succeeded inside todo block: got error 8 === debian11 (32 bit French report) === kernel32: heap.c:420: Test succeeded inside todo block: got error 8 === debian11 (32 bit Hebrew:Israel report) === kernel32: heap.c:420: Test succeeded inside todo block: got error 8 === debian11 (32 bit Hindi:India report) === kernel32: heap.c:420: Test succeeded inside todo block: got error 8 === debian11 (32 bit Japanese:Japan report) === kernel32: heap.c:420: Test succeeded inside todo block: got error 8 === debian11 (32 bit Chinese:China report) === kernel32: heap.c:420: Test succeeded inside todo block: got error 8 === debian11 (32 bit WoW report) === kernel32: heap.c:420: Test succeeded inside todo block: got error 8 === debian11 (64 bit WoW report) === kernel32: heap.c:420: Test succeeded inside todo block: got error 8
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/kernel32/tests/heap.c | 1 + dlls/ntdll/heap.c | 100 +++++++++++++++---------------------- 2 files changed, 40 insertions(+), 61 deletions(-) diff --git a/dlls/kernel32/tests/heap.c b/dlls/kernel32/tests/heap.c index 8a7260303d3..45a22adcf2e 100644 --- a/dlls/kernel32/tests/heap.c +++ b/dlls/kernel32/tests/heap.c @@ -413,6 +413,7 @@ static void test_HeapCreate(void) /* shrinking a very large block decommits pages and fail to grow in place */ ptr1 = HeapReAlloc( heap, HEAP_REALLOC_IN_PLACE_ONLY, ptr, alloc_size * 3 / 2 ); ok( ptr1 == ptr, "HeapReAlloc HEAP_REALLOC_IN_PLACE_ONLY failed, error %lu\n", GetLastError() ); + SetLastError( 0xdeadbeef ); ptr1 = HeapReAlloc( heap, HEAP_REALLOC_IN_PLACE_ONLY, ptr, 2 * alloc_size ); todo_wine ok( ptr1 != ptr, "HeapReAlloc HEAP_REALLOC_IN_PLACE_ONLY succeeded\n" ); diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 1a4d5b1a2bb..2d03cd59729 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -333,7 +333,7 @@ static RTL_CRITICAL_SECTION_DEBUG process_heap_cs_debug = static inline ULONG heap_get_flags( const HEAP *heap, ULONG flags ) { - flags &= HEAP_GENERATE_EXCEPTIONS | HEAP_NO_SERIALIZE | HEAP_ZERO_MEMORY; + flags &= HEAP_GENERATE_EXCEPTIONS | HEAP_NO_SERIALIZE | HEAP_ZERO_MEMORY | HEAP_REALLOC_IN_PLACE_ONLY; return heap->flags | flags; } @@ -1767,53 +1767,23 @@ BOOLEAN WINAPI DECLSPEC_HOTPATCH RtlFreeHeap( HANDLE heap, ULONG flags, void *pt } -/*********************************************************************** - * RtlReAllocateHeap (NTDLL.@) - * - * Change the size of a memory block allocated with RtlAllocateHeap(). - * - * PARAMS - * heap [I] Heap that block was allocated from - * flags [I] HEAP_ flags from "winnt.h" - * ptr [I] Block to resize - * size [I] Size of the memory block to allocate - * - * RETURNS - * Success: A pointer to the resized block (which may be different). - * Failure: NULL. - */ -PVOID WINAPI RtlReAllocateHeap( HANDLE heap, ULONG flags, PVOID ptr, SIZE_T size ) +static NTSTATUS heap_reallocate( HEAP *heap, ULONG flags, void *ptr, SIZE_T size, void **ret ) { ARENA_INUSE *pArena; - HEAP *heapPtr; SUBHEAP *subheap; SIZE_T oldBlockSize, oldActualSize, rounded_size; - void *ret; - - if (!ptr) return NULL; - if (!(heapPtr = HEAP_GetPtr( heap ))) - { - RtlSetLastWin32ErrorAndNtStatusFromNtStatus( STATUS_INVALID_HANDLE ); - return NULL; - } - - /* Validate the parameters */ - - flags &= HEAP_GENERATE_EXCEPTIONS | HEAP_NO_SERIALIZE | HEAP_ZERO_MEMORY | - HEAP_REALLOC_IN_PLACE_ONLY; - flags |= heapPtr->flags; - heap_lock( heapPtr, flags ); rounded_size = ROUND_SIZE(size) + HEAP_TAIL_EXTRA_SIZE(flags); - if (rounded_size < size) goto oom; /* overflow */ + 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( heapPtr, &subheap, pArena )) goto error; + if (!validate_block_pointer( heap, &subheap, pArena )) return STATUS_INVALID_PARAMETER; + if (!subheap) { - if (!(ret = realloc_large_block( heapPtr, flags, ptr, size ))) goto oom; - goto done; + if (!(*ret = realloc_large_block( heap, flags, ptr, size ))) return STATUS_NO_MEMORY; + return STATUS_SUCCESS; } /* Check if we need to grow the block */ @@ -1826,12 +1796,12 @@ PVOID WINAPI RtlReAllocateHeap( HANDLE heap, ULONG flags, PVOID ptr, SIZE_T size if (rounded_size >= HEAP_MIN_LARGE_BLOCK_SIZE && (flags & HEAP_GROWABLE)) { - if (flags & HEAP_REALLOC_IN_PLACE_ONLY) goto oom; - if (!(ret = allocate_large_block( heapPtr, flags, size ))) goto oom; - memcpy( ret, pArena + 1, oldActualSize ); + if (flags & HEAP_REALLOC_IN_PLACE_ONLY) return STATUS_NO_MEMORY; + if (!(*ret = allocate_large_block( heap, flags, size ))) return STATUS_NO_MEMORY; + memcpy( *ret, pArena + 1, oldActualSize ); notify_free( pArena + 1 ); HEAP_MakeInUseBlockFree( subheap, pArena ); - goto done; + return STATUS_SUCCESS; } if ((pNext < (char *)subheap->base + subheap->size) && (*(DWORD *)pNext & ARENA_FLAG_FREE) && @@ -1841,7 +1811,7 @@ PVOID WINAPI RtlReAllocateHeap( HANDLE heap, ULONG flags, PVOID ptr, SIZE_T size ARENA_FREE *pFree = (ARENA_FREE *)pNext; list_remove( &pFree->entry ); pArena->size += (pFree->size & ARENA_SIZE_MASK) + sizeof(*pFree); - if (!HEAP_Commit( subheap, pArena, rounded_size )) goto oom; + if (!HEAP_Commit( subheap, pArena, rounded_size )) return STATUS_NO_MEMORY; notify_realloc( pArena + 1, oldActualSize, size ); HEAP_ShrinkBlock( subheap, pArena, rounded_size ); } @@ -1851,9 +1821,8 @@ PVOID WINAPI RtlReAllocateHeap( HANDLE heap, ULONG flags, PVOID ptr, SIZE_T size ARENA_INUSE *pInUse; SUBHEAP *newsubheap; - if ((flags & HEAP_REALLOC_IN_PLACE_ONLY) || - !(pNew = HEAP_FindFreeBlock( heapPtr, rounded_size, &newsubheap ))) - goto oom; + if (flags & HEAP_REALLOC_IN_PLACE_ONLY) return STATUS_NO_MEMORY; + if (!(pNew = HEAP_FindFreeBlock( heap, rounded_size, &newsubheap ))) return STATUS_NO_MEMORY; /* Build the in-use arena */ @@ -1894,24 +1863,33 @@ PVOID WINAPI RtlReAllocateHeap( HANDLE heap, ULONG flags, PVOID ptr, SIZE_T size /* Return the new arena */ - ret = pArena + 1; -done: - heap_unlock( heapPtr, flags ); - TRACE("(%p,%08x,%p,%08lx): returning %p\n", heap, flags, ptr, size, ret ); - return ret; + *ret = pArena + 1; + return STATUS_SUCCESS; +} -oom: - heap_unlock( heapPtr, flags ); - if (flags & HEAP_GENERATE_EXCEPTIONS) RtlRaiseStatus( STATUS_NO_MEMORY ); - RtlSetLastWin32ErrorAndNtStatusFromNtStatus( STATUS_NO_MEMORY ); - TRACE("(%p,%08x,%p,%08lx): returning NULL\n", heap, flags, ptr, size ); - return NULL; +/*********************************************************************** + * RtlReAllocateHeap (NTDLL.@) + */ +void *WINAPI RtlReAllocateHeap( HANDLE heap, ULONG flags, void *ptr, SIZE_T size ) +{ + void *ret = NULL; + NTSTATUS status; + HEAP *heapPtr; -error: - heap_unlock( heapPtr, flags ); - RtlSetLastWin32ErrorAndNtStatusFromNtStatus( STATUS_INVALID_PARAMETER ); - TRACE("(%p,%08x,%p,%08lx): returning NULL\n", heap, flags, ptr, size ); - return NULL; + if (!ptr) return NULL; + + if (!(heapPtr = HEAP_GetPtr( heap ))) + status = STATUS_INVALID_HANDLE; + else + { + heap_lock( heapPtr, flags ); + status = heap_reallocate( heapPtr, heap_get_flags( heapPtr, flags ), ptr, size, &ret ); + heap_unlock( heapPtr, flags ); + } + + TRACE( "heap %p, flags %#x, ptr %p, size %#Ix, return %p, status %#x.\n", heap, flags, ptr, size, ret, status ); + heap_set_status( heap, flags, status ); + return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/55
From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/ntdll/heap.c | 71 ++++++++++++++++++++--------------------------- 1 file changed, 30 insertions(+), 41 deletions(-) diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 2d03cd59729..51567d0552b 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -1958,58 +1958,47 @@ BOOLEAN WINAPI RtlUnlockHeap( HANDLE heap ) } -/*********************************************************************** - * RtlSizeHeap (NTDLL.@) - * - * Get the actual size of a memory block allocated from a Heap. - * - * PARAMS - * heap [I] Heap that block was allocated from - * flags [I] HEAP_ flags from "winnt.h" - * ptr [I] Block to get the size of - * - * RETURNS - * Success: The size of the block. - * Failure: -1, heap or ptr are invalid. - * - * NOTES - * The size may be bigger than what was passed to RtlAllocateHeap(). - */ -SIZE_T WINAPI RtlSizeHeap( HANDLE heap, ULONG flags, const void *ptr ) +static NTSTATUS heap_size( HEAP *heap, const void *ptr, SIZE_T *size ) { - SIZE_T ret; - const ARENA_INUSE *pArena; + const ARENA_INUSE *block; SUBHEAP *subheap; - HEAP *heapPtr = HEAP_GetPtr( heap ); - - if (!heapPtr) - { - RtlSetLastWin32ErrorAndNtStatusFromNtStatus( STATUS_INVALID_HANDLE ); - return ~(SIZE_T)0; - } - - heap_lock( heapPtr, flags ); - pArena = (const ARENA_INUSE *)ptr - 1; - if (!validate_block_pointer( heapPtr, &subheap, pArena )) - { - RtlSetLastWin32ErrorAndNtStatusFromNtStatus( STATUS_INVALID_PARAMETER ); - ret = ~(SIZE_T)0; - } - else if (!subheap) + block = (const ARENA_INUSE *)ptr - 1; + if (!validate_block_pointer( heap, &subheap, block )) return STATUS_INVALID_PARAMETER; + if (!subheap) { const ARENA_LARGE *large_arena = (const ARENA_LARGE *)ptr - 1; - ret = large_arena->data_size; + *size = large_arena->data_size; } else { - ret = (pArena->size & ARENA_SIZE_MASK) - pArena->unused_bytes; + *size = (block->size & ARENA_SIZE_MASK) - block->unused_bytes; } - heap_unlock( heapPtr, flags ); + return STATUS_SUCCESS; +} - TRACE("(%p,%08x,%p): returning %08lx\n", heap, flags, ptr, ret ); - return ret; +/*********************************************************************** + * RtlSizeHeap (NTDLL.@) + */ +SIZE_T WINAPI RtlSizeHeap( HANDLE heap, ULONG flags, const void *ptr ) +{ + SIZE_T size = ~(SIZE_T)0; + NTSTATUS status; + HEAP *heapPtr; + + if (!(heapPtr = HEAP_GetPtr( heap ))) + status = STATUS_INVALID_PARAMETER; + else + { + heap_lock( heapPtr, flags ); + status = heap_size( heapPtr, ptr, &size ); + heap_unlock( heapPtr, flags ); + } + + TRACE( "heap %p, flags %#x, ptr %p, return %#Ix, status %#x.\n", heap, flags, ptr, size, status ); + heap_set_status( heapPtr, flags, status ); + return size; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/55
participants (2)
-
Marvin -
Rémi Bernon