Re: [RFC PATCH 2/2] reftable: don't memset() a NULL from failed malloc()

From: René Scharfe <l.s.r@web.de>
Date: Apr 15, 2022, 13:37 UTC
Message-ID: <4321a9dd-bb82-e2fe-5449-395f998378c5@web.de>
In-Reply-To: <RFC-patch-2.2-364d1194a95-20220415T101740Z-avarab@gmail.com>

Am 15.04.22 um 12:21 schrieb Ævar Arnfjörð Bjarmason:

Show 11 quoted lines

> Return NULL instead of possibly feeding a NULL to memset() in
> reftable_calloc(). This issue was noted by GCC 12's -fanalyzer:
>
> 	reftable/publicbasics.c: In function ‘reftable_calloc’:
> 	reftable/publicbasics.c:43:9: error: use of possibly-NULL ‘p’ where non-null expected [CWE-690] [-Werror=analyzer-possible-null-argument]
> 	   43 |         memset(p, 0, sz);
> 	      |         ^~~~~~~~~~~~~~~~
> 	[...]
>
> This bug has been with us ever since this code was added in
> ef8a6c62687 (reftable: utility functions, 2021-10-07).

Not sure it's a bug, or limited to this specific location. AFAICS it's more a general lack of handling of allocation failures in the reftable code. Perhaps it was a conscious decision to let the system deal with them via segfaults?

When the code is used by Git eventually it should use xmalloc and xrealloc instead of calling malloc(3) and realloc(3) directly, to handle allocation failures explicitly, and to support GIT_ALLOC_LIMIT. This will calm down the analyzer and extend the safety to the rest of the reftable code, not just this memset call.

Show 17 quoted lines

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  reftable/publicbasics.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/reftable/publicbasics.c b/reftable/publicbasics.c
> index 0ad7d5c0ff2..a18167f5ab7 100644
> --- a/reftable/publicbasics.c
> +++ b/reftable/publicbasics.c
> @@ -40,6 +40,8 @@ void reftable_free(void *p)
>  void *reftable_calloc(size_t sz)
>  {
>  	void *p = reftable_malloc(sz);
> +	if (!p)
> +		return NULL;
>  	memset(p, 0, sz);

This function is calloc(3) reimplemented, except it can't make use of pre-zero'd blocks of memory and doesn't multiply element size and count for the caller while checking for overflow, making it slower and harder to use securely. :-/

>  	return p;
>  }