diff mbox series

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

Message ID bdfddbebce9f77959fd9544cd5ba3496d5b9dccf.1727866394.git.ps@pks.im (mailing list archive)
State Accepted
Commit eef7bcdafe0037f14a96c564ace899342b9ed0fb
Headers show
Series reftable: handle allocation errors | expand

Commit Message

Patrick Steinhardt Oct. 2, 2024, 10:55 a.m. UTC 
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(-)

Comments

Eric Sunshine Oct. 2, 2024, 10:07 p.m. UTC | #1 
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;
>  }
diff mbox series

Patch

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");