[PATCH 0/1] MR4145: ntdll: Define heap block's BLOCK_FLAG_LFH as 0x80.
This seems to be the place of this flag on Windows. I am attaching the test here which suggests that this is the place for the flag. I am not including is in MR as afraid it may be flaky (although currently succeeded on all the testbot machines), as many factors may affect if the block actually ends up LFH or not. The motivation under doing this is that it should help "Binding of Isaac: Rebirth" to avoid random crashes, by making `*(int *)((int *)allocated_ptr_of_size_4 - 1)` negative, which, due to some checks on the value, avoids further (potentially fatal) out of bound memory access. [lfh_flag_test.patch](/uploads/0048642b107783f5758320e06cd3d629/lfh_flag_test.patch) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4145
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/ntdll/heap.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/heap.c b/dlls/ntdll/heap.c index 5ce7f8fad2f..b5e2076ffae 100644 --- a/dlls/ntdll/heap.c +++ b/dlls/ntdll/heap.c @@ -103,12 +103,12 @@ C_ASSERT( sizeof(struct block) == 8 ); #define BLOCK_FLAG_PREV_FREE 0x02 #define BLOCK_FLAG_FREE_LINK 0x03 #define BLOCK_FLAG_LARGE 0x04 -#define BLOCK_FLAG_LFH 0x08 /* block is handled by the LFH frontend */ -#define BLOCK_FLAG_USER_INFO 0x10 /* user flags up to 0xf0 */ -#define BLOCK_FLAG_USER_MASK 0xf0 +#define BLOCK_FLAG_LFH 0x80 /* block is handled by the LFH frontend */ +#define BLOCK_FLAG_USER_INFO 0x08 /* user flags bits 3-6 */ +#define BLOCK_FLAG_USER_MASK 0x78 -#define BLOCK_USER_FLAGS( heap_flags ) (((heap_flags) >> 4) & BLOCK_FLAG_USER_MASK) -#define HEAP_USER_FLAGS( block_flags ) (((block_flags) & BLOCK_FLAG_USER_MASK) << 4) +#define BLOCK_USER_FLAGS( heap_flags ) (((heap_flags) >> 5) & BLOCK_FLAG_USER_MASK) +#define HEAP_USER_FLAGS( block_flags ) (((block_flags) & BLOCK_FLAG_USER_MASK) << 5) /* entry to link free blocks in free lists */ @@ -1946,7 +1946,7 @@ static NTSTATUS heap_allocate_block_lfh( struct heap *heap, ULONG flags, SIZE_T if ((block = find_free_bin_block( heap, flags, block_size, bin ))) { block_set_type( block, BLOCK_TYPE_USED ); - block_set_flags( block, ~BLOCK_FLAG_LFH, BLOCK_USER_FLAGS( flags ) ); + block_set_flags( block, (BYTE)~BLOCK_FLAG_LFH, BLOCK_USER_FLAGS( flags ) ); block->tail_size = block_size - sizeof(*block) - size; initialize_block( block, 0, size, flags ); mark_block_tail( block, flags ); @@ -1971,7 +1971,7 @@ static NTSTATUS heap_free_block_lfh( struct heap *heap, ULONG flags, struct bloc i = block_get_group_index( block ); valgrind_make_writable( block, sizeof(*block) ); block_set_type( block, BLOCK_TYPE_FREE ); - block_set_flags( block, ~BLOCK_FLAG_LFH, BLOCK_FLAG_FREE ); + block_set_flags( block, (BYTE)~BLOCK_FLAG_LFH, BLOCK_FLAG_FREE ); mark_block_free( block + 1, (char *)block + block_size - (char *)(block + 1), flags ); /* if this was the last used block in a group and GROUP_FLAG_FREE was set */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4145
Rémi Bernon (@rbernon) commented about dlls/ntdll/heap.c:
if ((block = find_free_bin_block( heap, flags, block_size, bin ))) { block_set_type( block, BLOCK_TYPE_USED ); - block_set_flags( block, ~BLOCK_FLAG_LFH, BLOCK_USER_FLAGS( flags ) ); + block_set_flags( block, (BYTE)~BLOCK_FLAG_LFH, BLOCK_USER_FLAGS( flags ) );
Do we need the cast? The argument is already a BYTE. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4145#note_49714
On Wed Oct 25 10:17:29 2023 +0000, Rémi Bernon wrote:
Do we need the cast? The argument is already a BYTE. This is because without that I am getting a warning on my gcc 13 about a "negative" value truncation.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4145#note_49732
On Wed Oct 25 13:38:25 2023 +0000, Paul Gofman wrote:
This is because without that I am getting a warning on my gcc 13 about a "negative" value truncation. Eh... does it still complain if the flag constants are unsigned, suffixed with u? I think it'd be better than casts here and there.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4145#note_49737
On Wed Oct 25 14:08:23 2023 +0000, Rémi Bernon wrote:
Eh... does it still complain if the flag constants are unsigned, suffixed with u? I think it'd be better than casts here and there. That were my thoughts as well but unfortunately it still does with u-suffixed flags:
../wine/dlls/ntdll/heap.c:1974:29: warning: conversion from 'unsigned int' to 'BYTE' {aka 'unsigned char'} changes value from '4294967167' to '127' [-Woverflow]
1974 | block_set_flags( block, ~BLOCK_FLAG_LFH, BLOCK_FLAG_FREE );
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4145#note_49738
On Wed Oct 25 14:11:56 2023 +0000, Paul Gofman wrote:
That were my thoughts as well but unfortunately it still does with u-suffixed flags: ``` ../wine/dlls/ntdll/heap.c:1974:29: warning: conversion from 'unsigned int' to 'BYTE' {aka 'unsigned char'} changes value from '4294967167' to '127' [-Woverflow] 1974 | block_set_flags( block, ~BLOCK_FLAG_LFH, BLOCK_FLAG_FREE ); ``` Maybe have constants defined as ((BYTE)0x00) then?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4145#note_49740
On Wed Oct 25 14:15:19 2023 +0000, Rémi Bernon wrote:
Maybe have constants defined as ((BYTE)0x00) then? It doesn't help, it still implicitly casts that to int first and then complains.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4145#note_49752
On Wed Oct 25 14:55:40 2023 +0000, Paul Gofman wrote:
It doesn't help, it still implicitly casts that to int first and then complains. The only way I see is to define BLOCK_ALL_BUT_LFH (BYTE)~BLOCK_FLAG_LFH but it is arguably worse than explicit cast.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4145#note_49753
On Wed Oct 25 15:00:09 2023 +0000, Paul Gofman wrote:
The only way I see is to define BLOCK_ALL_BUT_LFH (BYTE)~BLOCK_FLAG_LFH but it is arguably worse than explicit cast. Yeah, well okay.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4145#note_49755
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4145
participants (3)
-
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon