On Fri, Oct 4, 2024 at 12:59 AM Patrick Steinhardt <ps@xxxxxx> wrote: > 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@xxxxxx> > --- > 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; Seems sensible; it covers both problems described by the commit message.
