Re: [PATCH 2/4] reftable: fix allocation count on realloc error

[Patrick Steinhardt <ps@pks.im>]

On Wed, Dec 25, 2024 at 07:38:35PM +0100, René Scharfe wrote:
> When realloc(3) fails, it returns NULL and keeps the original allocation
> intact.  REFTABLE_ALLOC_GROW overwrites both the original pointer and
> the allocation count variable in that case, simultaneously leaking the
> original allocation and misrepresenting the number of storable items.
> 
> parse_names() avoids the leak by keeping the original pointer if
> reallocation fails, but still increase the allocation count in such a
> case as if it succeeded.  That's OK, because the error handling code
> just frees everything and doesn't look at names_cap anymore.
> 
> reftable_buf_add() does the same, but here it is a problem as it leaves
> the reftable_buf in a broken state, with ->alloc being roughly twice as
> big as the actually allocated memory, allowing out-of-bounds writes in
> subsequent calls.
> 
> Reimplement REFTABLE_ALLOC_GROW to avoid leaks, keep allocation counts
> in sync and still signal failures to callers while avoiding code
> duplication in callers.  Make it an expression that evaluates to 0 if no
> reallocation is needed or it succeeded and 1 on failure while keeping
> the original pointer and allocation counter values.
> 
> Adjust REFTABLE_ALLOC_GROW_OR_NULL to the new calling convention for
> REFTABLE_ALLOC_GROW, but keep its support for non-size_t alloc variables
> for now.

Okay, this change goes into the direction I was wondering about with the
preceding commit, good.

> diff --git a/reftable/basics.h b/reftable/basics.h
> index 259f4c274c..fa5d75868b 100644
> --- a/reftable/basics.h
> +++ b/reftable/basics.h
> @@ -120,22 +120,35 @@ char *reftable_strdup(const char *str);
>  #define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))
>  #define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))
>  #define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))
> -#define REFTABLE_ALLOC_GROW(x, nr, alloc) \
> -	do { \
> -		if ((nr) > alloc) { \
> -			alloc = 2 * (alloc) + 1; \
> -			if (alloc < (nr)) \
> -				alloc = (nr); \
> -			REFTABLE_REALLOC_ARRAY(x, alloc); \
> -		} \
> -	} while (0)
> +
> +static inline void *reftable_alloc_grow(void *p, size_t nelem, size_t elsize,
> +					size_t *allocp)
> +{
> +	void *new_p;
> +	size_t alloc = *allocp * 2 + 1;
> +	if (alloc < nelem)
> +		alloc = nelem;
> +	new_p = reftable_realloc(p, st_mult(elsize, alloc));
> +	if (!new_p)
> +		return p;
> +	*allocp = alloc;
> +	return new_p;
> +}
> +
> +#define REFTABLE_ALLOC_GROW(x, nr, alloc) ( \
> +	(nr) > (alloc) && ( \
> +		(x) = reftable_alloc_grow((x), (nr), sizeof(*(x)), &(alloc)), \
> +		(nr) > (alloc) \
> +	) \
> +)

Do we even need this macro? I don't really think it serves much of a
purpose anymore.

Patrick