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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 2, 2024, 22:07 UTC
Message-ID: <CAPig+cT7nni11bGmNyYCQZ0P0QZ_HG-izPtsymq==sUOkOn-Lw@mail.gmail.com>
In-Reply-To: <bdfddbebce9f77959fd9544cd5ba3496d5b9dccf.1727866394.git.ps@pks.im>

On Wed, Oct 2, 2024 at 6:56 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 12 quoted lines

> Handle allocation failures in `parse_names()` by returning `NULL` in
> case any allocation fails. While at it, refactor the function to return
> the array directly instead of assigning it to an out-pointer.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/reftable/basics.c b/reftable/basics.c
> @@ -152,14 +152,26 @@ void parse_names(char *buf, int size, char ***namesp)
>                         REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap);
> -                       names[names_len++] = xstrdup(p);
> +                       if (!names)
> +                               goto err;

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...

Show 16 quoted lines

> +                       names[names_len] = reftable_strdup(p);
> +                       if (!names[names_len++])
> +                               goto err;
>                 }
>                 p = next + 1;
>         }
>
>         REFTABLE_REALLOC_ARRAY(names, names_len + 1);
>         names[names_len] = NULL;
> -       *namesp = names;
> +
> +       return names;
> +
> +err:
> +       for (size_t i = 0; i < names_len; i++)
> +               reftable_free(names[i]);

... and then we potentially index into names[] because `names_len` is non-zero, thus crash because `names` is NULL.

> +       reftable_free(names);
> +       return NULL;
>  }