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
