Re: [PATCH v5 16/16] reftable: be more paranoid about 0-length memcpy calls
- From
René Scharfe <l.s.r@web.de>
- Date
- Dec 26, 2021, 20:51 UTC
- Message-ID
- <e288fad3-8a83-448c-d701-47c56bc258e3@web.de>
- In-Reply-To
- <xmqqo857b11h.fsf@gitster.g>
Am 23.12.21 um 19:59 schrieb Junio C Hamano:
Show 30 quoted lines
> 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?
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)