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?
diff --git a/reftable/publicbasics.c b/reftable/publicbasics.c
index 0ad7d5c0ff..438a1b0a7d 100644
--- a/reftable/publicbasics.c
+++ b/reftable/publicbasics.c
@@ -19,14 +19,14 @@ void *reftable_malloc(size_t sz)
{
if (reftable_malloc_ptr)
return (*reftable_malloc_ptr)(sz);
- return malloc(sz);
+ return xmalloc(sz);
}
void *reftable_realloc(void *p, size_t sz)
{
if (reftable_realloc_ptr)
return (*reftable_realloc_ptr)(p, sz);
- return realloc(p, sz);
+ return xrealloc(p, sz);
}
void reftable_free(void *p)
