diff mbox series

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

Message ID ad028020df717a57879cf3fdf5a3648bfebd2c78.1727158127.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series reftable: handle allocation errors | expand

Commit Message

Patrick Steinhardt Sept. 24, 2024, 6:32 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

René Scharfe Sept. 24, 2024, 10:19 p.m. UTC | #1 
Am 24.09.24 um 08:32 schrieb Patrick Steinhardt:
> diff --git a/reftable/basics.c b/reftable/basics.c
> index 3350bbffa23..82bfb807631 100644
> --- a/reftable/basics.c
> +++ b/reftable/basics.c
> @@ -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] = 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++)
> +		free(names[i]);
> +	free(names);

These are allocated with reftable_realloc() under the hood, so they'd
better be passed to reftable_free() instead, no?

René
Patrick Steinhardt Sept. 26, 2024, 12:09 p.m. UTC | #2 
On Wed, Sep 25, 2024 at 12:19:29AM +0200, René Scharfe wrote:
> Am 24.09.24 um 08:32 schrieb Patrick Steinhardt:
> > diff --git a/reftable/basics.c b/reftable/basics.c
> > index 3350bbffa23..82bfb807631 100644
> > --- a/reftable/basics.c
> > +++ b/reftable/basics.c
> > @@ -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] = 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++)
> > +		free(names[i]);
> > +	free(names);
> 
> These are allocated with reftable_realloc() under the hood, so they'd
> better be passed to reftable_free() instead, no?

Oh yeah, good catch! I'll wait a bit longer for feedback before sending
out a revised version.

Patrick
diff mbox series

Patch

diff --git a/reftable/basics.c b/reftable/basics.c
index 3350bbffa23..82bfb807631 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] = 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++)
+		free(names[i]);
+	free(names);
+	return NULL;
 }
 
 int names_equal(const char **a, const char **b)
diff --git a/reftable/basics.h b/reftable/basics.h
index f107e148605..69adeab2e4b 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 ce0a35216ba..498fae846d7 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 e5556ebf527..1fa77b6faff 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");