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@xxxxxx> 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@xxxxxx>
> > ---
> > 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@xxxxxx>
---
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++])
