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".
Thanks.
