Re: [PATCH 1/4] reftable: avoid leaks on realloc error

[Patrick Steinhardt <ps@pks.im>]

On Wed, Dec 25, 2024 at 07:38:29PM +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() and reftable_buf_add() avoid leaking by restoring the
> original pointer value on failure, but all other callers seem to be OK
> with losing the old allocation.  Add a new variant of the macro,
> REFTABLE_ALLOC_GROW_OR_NULL, which plugs the leak and zeros the
> allocation counter.  Use it for those callers.

Hm, okay. I find it a bit curious to discern those two macros from each
other as all callers need to handle OOM errors anyway, so doing the safe
thing should likely be our default here and all callsites that don't
should be adapted, shouldn't they?

In the case of `reftable_buf_add()` I kind of doubt the usefulness of
handling the error just to keep the old pointer intact, as all callsites
will ultimately error out anyway. But in the case of `parse_names()` we
do in fact want to handle the case specially so that we can free any
names we have already parsed, so that case makes sense indeed.

So there is merit in having two separate wrappers, but it would be nice
if `REFTABLE_ALLOC_GROW()` would be doing the "right thing" for most
cases while the above two callsites would be adapted to use a wrapper
that requires a bit more thought to use correctly. For example something
like `REFTABLE_TRY_ALLOC_GROW()` or similar.

Patrick