Re: [PATCH v5 05/25] reftable/basics: handle allocation failures in `parse_names()`

> On Wed, Oct 02, 2024 at 06:07:14PM -0400, Eric Sunshine wrote:
> > Am I reading this correctly? Presumably, `names_len` can be non-zero
> > here, right? And we now check for names==NULL to detect an allocation
> > failure...
> > ... and then we potentially index into names[] because `names_len` is
> > non-zero, thus crash because `names` is NULL.
>
> Good catch! I think we should queue something like the below on top of
> what we already have in `next` now.
>
> -- >8 --
>
> Subject: [PATCH] reftable/basics: fix segfault when growing `names` array fails
>
> When growing the `names` array fails we would end up with a `NULL`
> pointer. This causes two problems:
>
>   - We would run into a segfault because we try to free names that we
>     have assigned to the array already.
>
>   - We lose track of the old array and cannot free its contents.
>
> Fix this issue by using a temporary variable. Like this we do not
> clobber the old array that we tried to reallocate, which will remain
> valid when a call to realloc(3P) fails.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/basics.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/reftable/basics.c b/reftable/basics.c
> @@ -152,9 +152,11 @@ char **parse_names(char *buf, int size)
> -                       REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap);
> -                       if (!names)
> -                               goto err;
> +                       char **names_grown = names;
> +                       REFTABLE_ALLOC_GROW(names_grown, names_len + 1, names_cap);
> +                       if (!names_grown)
> +                               goto err;
> +                       names = names_grown;