Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Apr 15 2022, René Scharfe wrote:

> Am 15.04.22 um 12:21 schrieb Ævar Arnfjörð Bjarmason:
>> Return NULL instead of possibly feeding a NULL to memset() in
>> reftable_calloc(). This issue was noted by GCC 12's -fanalyzer:
>>
>> 	reftable/publicbasics.c: In function ‘reftable_calloc’:
>> 	reftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]
>> 	   43 |         memset(p, 0, sz);
>> 	      |         ^~~~~~~~~~~~~~~~
>> 	[...]
>>
>> This bug has been with us ever since this code was added in
>> ef8a6c62687 (reftable: utility functions, 2021-10-07).
>
> Not sure it's a bug, or limited to this specific location.  AFAICS it's
> more a general lack of handling of allocation failures in the reftable
> code.  Perhaps it was a conscious decision to let the system deal with
> them via segfaults?

I think it's just buggy, it tries to deal with malloc returning NULL in
other contexts.

> When the code is used by Git eventually it should use xmalloc and
> xrealloc instead of calling malloc(3) and realloc(3) directly, to
> handle allocation failures explicitly, and to support GIT_ALLOC_LIMIT.
> This will calm down the analyzer and extend the safety to the rest of
> the reftable code, not just this memset call.

Yeah, its whole custom malloc handling should go away.

>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  reftable/publicbasics.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/reftable/publicbasics.c b/reftable/publicbasics.c
>> index 0ad7d5c0ff2..a18167f5ab7 100644
>> --- a/reftable/publicbasics.c
>> +++ b/reftable/publicbasics.c
>> @@ -40,6 +40,8 @@ void reftable_free(void *p)
>>  void *reftable_calloc(size_t sz)
>>  {
>>  	void *p = reftable_malloc(sz);
>> +	if (!p)
>> +		return NULL;
>>  	memset(p, 0, sz);
>
> This function is calloc(3) reimplemented, except it can't make use of
> pre-zero'd blocks of memory and doesn't multiply element size and count
> for the caller while checking for overflow, making it slower and harder
> to use securely. :-/

*nod*, this is really just re-arranging the deck chairs.

Maybe Han-Wen had something in mind, but I really don't see the point of
having it use malloc wrappers at all, as opposed to just having the
linker point it to the right "malloc".

So if it needs to be used outside of git.git it would just need the
trivial xmalloc() etc. wrappers.