Re: [PATCH] reftable: use xmalloc() and xrealloc()

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

On Sat, Apr 06, 2024 at 10:37:55PM +0200, René Scharfe wrote:
> malloc(3) and realloc(3) can fail and return NULL.  None of the reftable
> code checks for that possibility and would happily dereference NULL
> pointers.  Use xmalloc() and xrealloc() instead like in the rest of Git
> to report allocation errors and exit cleanly, and to also honor the
> environment variable GIT_ALLOC_LIMIT.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  reftable/publicbasics.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/reftable/publicbasics.c b/reftable/publicbasics.c
> index 44b84a125e..f33a65df34 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)

These are part of the library interfaces that should ideally not be tied
to the Git code base at all so that they can theoretically be reused by
another project like libgit2. So I think that instead of rewriting the
generic fallbacks we should call `reftable_set_alloc()` somewhen early
in Git's startup code.

It does raise the question what to do about the generic fallbacks. We
could start calling `abort()` when we observe allocation failures. It's
not exactly nice behaviour in a library though, where the caller may in
fact want to handle this case. But it may at least be better than
failing on a `NULL` pointer exception somewhere down the road. So it
might be the best alternative for now. We could then conver the reftable
library over time to handle allocation failures and, once that's done,
we can eventually drop such a call to `abort()`.

Cc'ing Han-Wen's new mail address as he no longer works at Google.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]