On Sun, Dec 26 2021, René Scharfe wrote:
> Am 23.12.21 um 19:59 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@xxxxxx> 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?
I don't know why it's there, but I suppose it's for the same reason as
PCREv2's integration, whose rabbit hole starts at 513f2b0bbd4 (grep:
make PCRE2 aware of custom allocator, 2019-10-16).
I.e. for building it as part of git.git we can just replace
malloc()/free(), but for other uses as a general linkable library you'd
want to replace it at runtime.
