| Message ID | bdfddbebce9f77959fd9544cd5ba3496d5b9dccf.1727866394.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | eef7bcdafe0037f14a96c564ace899342b9ed0fb |
| Headers | show |
| Series | reftable: handle allocation errors | expand |
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; > }
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++])
On Fri, Oct 4, 2024 at 12:59 AM Patrick Steinhardt <ps@pks.im> 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@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; Seems sensible; it covers both problems described by the commit message.
diff --git a/reftable/basics.c b/reftable/basics.c index 3350bbffa2..ea53cf102a 100644 --- a/reftable/basics.c +++ b/reftable/basics.c @@ -135,14 +135,14 @@ size_t names_length(const char **names) return p - names; } -void parse_names(char *buf, int size, char ***namesp) +char **parse_names(char *buf, int size) { char **names = NULL; size_t names_cap = 0; size_t names_len = 0; - char *p = buf; char *end = buf + size; + while (p < end) { char *next = strchr(p, '\n'); if (next && next < end) { @@ -152,14 +152,26 @@ void parse_names(char *buf, int size, char ***namesp) } if (p < next) { REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap); - names[names_len++] = xstrdup(p); + if (!names) + goto err; + + 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]); + reftable_free(names); + return NULL; } int names_equal(const char **a, const char **b) diff --git a/reftable/basics.h b/reftable/basics.h index f107e14860..69adeab2e4 100644 --- a/reftable/basics.h +++ b/reftable/basics.h @@ -38,9 +38,12 @@ size_t binsearch(size_t sz, int (*f)(size_t k, void *args), void *args); */ void free_names(char **a); -/* parse a newline separated list of names. `size` is the length of the buffer, - * without terminating '\0'. Empty names are discarded. */ -void parse_names(char *buf, int size, char ***namesp); +/* + * Parse a newline separated list of names. `size` is the length of the buffer, + * without terminating '\0'. Empty names are discarded. Returns a `NULL` + * pointer when allocations fail. + */ +char **parse_names(char *buf, int size); /* compares two NULL-terminated arrays of strings. */ int names_equal(const char **a, const char **b); diff --git a/reftable/stack.c b/reftable/stack.c index ce0a35216b..498fae846d 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -108,7 +108,11 @@ static int fd_read_lines(int fd, char ***namesp) } buf[size] = 0; - parse_names(buf, size, namesp); + *namesp = parse_names(buf, size); + if (!*namesp) { + err = REFTABLE_OUT_OF_MEMORY_ERROR; + goto done; + } done: reftable_free(buf); diff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c index e5556ebf52..1fa77b6faf 100644 --- a/t/unit-tests/t-reftable-basics.c +++ b/t/unit-tests/t-reftable-basics.c @@ -72,13 +72,14 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED) if_test ("parse_names works for basic input") { char in1[] = "line\n"; char in2[] = "a\nb\nc"; - char **out = NULL; - parse_names(in1, strlen(in1), &out); + char **out = parse_names(in1, strlen(in1)); + check(out != NULL); check_str(out[0], "line"); check(!out[1]); free_names(out); - parse_names(in2, strlen(in2), &out); + out = parse_names(in2, strlen(in2)); + check(out != NULL); check_str(out[0], "a"); check_str(out[1], "b"); check_str(out[2], "c"); @@ -88,8 +89,8 @@ int cmd_main(int argc UNUSED, const char *argv[] UNUSED) if_test ("parse_names drops empty string") { char in[] = "a\n\nb\n"; - char **out = NULL; - parse_names(in, strlen(in), &out); + char **out = parse_names(in, strlen(in)); + check(out != NULL); check_str(out[0], "a"); /* simply '\n' should be dropped as empty string */ check_str(out[1], "b");
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> --- reftable/basics.c | 20 ++++++++++++++++---- reftable/basics.h | 9 ++++++--- reftable/stack.c | 6 +++++- t/unit-tests/t-reftable-basics.c | 11 ++++++----- 4 files changed, 33 insertions(+), 13 deletions(-)