Re: [PATCH v5 16/16] reftable: be more paranoid about 0-length memcpy calls

> Am 23.12.21 um 19:59 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>>>> @@ -569,7 +572,8 @@ static int reftable_obj_record_decode(void *rec, struct strbuf key,
>>>>>  	uint64_t last;
>>>>>  	int j;
>>>>>  	r->hash_prefix = reftable_malloc(key.len);
>>>>> -	memcpy(r->hash_prefix, key.buf, key.len);
>>>>> +	if (key.len)
>>>>> +		memcpy(r->hash_prefix, key.buf, key.len);
>>>>>  	r->hash_prefix_len = key.len;
>>>>>
>>>>>  	if (val_type == 0) {
>>>>
>>>> I am not sure why any of these are needed.
>>>> ...
>>> I don't know about the first two, but in the third case dst (i.e.
>>> r->hash_prefix) might be NULL if key.len == 0, reftable_malloc is malloc
>>> (which it is, because reftable_set_alloc is never called) and malloc(0)
>>> returns NULL (which it might do according to
>>> https://www.man7.org/linux/man-pages/man3/malloc.3.html).
>>>
>>> malloc can return NULL on failure, too, of course, and none of the
>>> reftable_malloc callers check for that.  That seems a bit too
>>> optimistic.
>>
>> Yeah, I agree that the real bug in this code is that the returned
>> value of malloc() is not checked.  But checking if key.len is zero
>> before using memcpy() would not help fix it, or hide it.  So I am
>> not sure if that is a counter-argument against "this check is
>> pointless".
>
> It's not -- I was just trying to find a scenario which would cause a
> Coverity warning that could be silenced by such a zero check.
>
> We can use xmalloc and xrealloc to handle allocation failures and to get
> valid pointers for zero-sized allocations, like in the patch below.
>
> I don't understand why reftable_set_alloc exists, though -- is there
> really a use case that requires changing the allocator at runtime?