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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Oct 02, 2024 at 06:07:14PM -0400, Eric Sunshine wrote:
> On Wed, Oct 2, 2024 at 6:56 AM Patrick Steinhardt <ps@pks.im> wrote:
> > 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...
> 
> > +                       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;
> >  }

Good catch! I think we should queue something like the below on top of
what we already have in `next` now.

Patrick

-- >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
index c8396dc525..df49cc8ef2 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -152,9 +152,11 @@ char **parse_names(char *buf, int size)
 			next = end;
 		}
 		if (p < next) {
-			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;
 
 			names[names_len] = reftable_strdup(p);
 			if (!names[names_len++])