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

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 27, 2024, 10:33 UTC
Message-ID: <Z26CjQpewcyLwfs4@pks.im>
In-Reply-To: <9b2f4baa-b602-4cc5-8dfc-dd941b1d7af6@web.de>

On Wed, Dec 25, 2024 at 07:38:29PM +0100, René Scharfe wrote:

Show 10 quoted lines

> 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