From: phillip.wood123@gmail.com
Date: Thu, 28 Dec 2023 16:48:11 GMT
Subject: Re: [PATCH] mem-pool: fix big allocations
Message-ID: <e1e43a6c-3e06-4453-88a3-f00476132bcd@gmail.com>
In-Reply-To: <c5d35735-10e2-4b71-8fc7-6218e7002549@web.de>

On 28/12/2023 16:05, René Scharfe wrote:
> Am 28.12.23 um 16:10 schrieb Phillip Wood:
>> The diff at the end of
>> this email shows a possible implementation of a check_ptr() macro for
>> the unit test library. I'm wary of adding it though because I'm not sure
>> printing the pointer values is actually very useful most of the
>> time. I'm also concerned that the rules around pointer arithmetic and
>> comparisons mean that many pointer tests such as
>>
>>      check_ptr(pool->mp_block->next_free, <=, pool->mp_block->end);
>>
>> will be undefined if they fail.
> 
> True, the compiler could legally emit mush when it finds out that the
> pointers are for different objects.  And the error being fixed produces
> such unrelated pointer pairs -- oops.
> 
> This check is not important here, we can just drop it.
> 
> mem_pool_contains() has the same problem, by the way.
> 
> Restricting ourselves to only equality comparisons for pointers prevents
> some interesting sanity checks, though.  Casting to intptr_t or
> uintptr_t would allow arbitrary comparisons without risk of undefined
> behavior, though.  Perhaps that would make a check_ptr() macro viable
> and useful.

That certainly helps and the check_ptr() macro in my previous email 
casts the pointers to uintptr_t before comparing them. Maybe I'm 
worrying too much, but my concern is that in a failing comparison it is 
likely one of the pointers is invalid (for example it is the result of 
some undefined pointer arithmetic) and the program is undefined from the 
point the invalid pointer is created. The documentation for check_ptr() 
in my previous mail contains the following example

     For example if `start` and `end` are pointers to the beginning and
     end of an allocation and `offset` is an integer then

         check_ptr(start + offset, <=, end)

     is undefined when `offset` is larger than `end - start`. Rewriting
     the comparison as

         check_uint(offset, <=, end - start)

     avoids undefined behavior when offset is too large, but is still
     undefined if there is a bug that means `start` and `end` do not
     point to the same allocation.

I agree it would be nice to allow arbitrary pointer comparisons but it 
would be good to do it in a way that does not expose us to undefined 
behavior. I'm not sure what the right balance is here.

Best Wishes

Phillip