Re: [PATCH] revision: fix memory leaks with `struct cmdline_pathspec`

[Jeff King <peff@peff.net>]

On Wed, Sep 20, 2017 at 09:47:26PM +0200, Martin Ågren wrote:

> We don't free the array `prune_data.path` or the individual strings it
> points to. Do so by introducing and using `free_cmdline_pathspec()`. To
> be able to safely free the strings, always use `xstrdup()` when
> assigning them. That does mean we allocate more memory than we used to,
> but it also means it is clear who owns the strings and that we can stop
> leaking those that we do allocate.

Hmm. From this description (and from looking at the patch), it seems
like we could just skip the allocation.

The missing piece of the puzzle is that sometimes we call
append_prune_data() to append from argv, and sometimes we use
read_pathspec_from_stdin().

So we may literally have a mix-and-match of allocated and unallocated
entries, and the only sane way to resolve that is by making them all
allocated.

So I think this solves the problem, but I couldn't help notice...

> @@ -1682,7 +1682,7 @@ static void append_prune_data(struct cmdline_pathspec *prune, const char **av)
>  {
>  	while (*av) {
>  		ALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);
> -		prune->path[prune->nr++] = *(av++);
> +		prune->path[prune->nr++] = xstrdup(*(av++));
>  	}

Isn't this whole thing just an argv_array, and this is argv_array_pushv?
We even NULL-terminate it manually later on!

So rather than increasing the line count by adding
free_cmdline_pathspec, I think we could actually _reduce_ it by
converting to an argv array, as below. And then adding in your free
would be one extra line.

diff --git a/revision.c b/revision.c
index 94a5e98525..5c58b3fb2b 100644
--- a/revision.c
+++ b/revision.c
@@ -20,6 +20,7 @@
 #include "cache-tree.h"
 #include "bisect.h"
 #include "packfile.h"
+#include "argv-array.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -1612,31 +1613,15 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
 	return 0;
 }
 
-struct cmdline_pathspec {
-	int alloc;
-	int nr;
-	const char **path;
-};
-
-static void append_prune_data(struct cmdline_pathspec *prune, const char **av)
-{
-	while (*av) {
-		ALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);
-		prune->path[prune->nr++] = *(av++);
-	}
-}
-
 static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,
-				     struct cmdline_pathspec *prune)
+				     struct argv_array *prune)
 {
-	while (strbuf_getline(sb, stdin) != EOF) {
-		ALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);
-		prune->path[prune->nr++] = xstrdup(sb->buf);
-	}
+	while (strbuf_getline(sb, stdin) != EOF)
+		argv_array_push(prune, sb->buf);
 }
 
 static void read_revisions_from_stdin(struct rev_info *revs,
-				      struct cmdline_pathspec *prune)
+				      struct argv_array *prune)
 {
 	struct strbuf sb;
 	int seen_dashdash = 0;
@@ -2201,10 +2186,9 @@ static void NORETURN diagnose_missing_default(const char *def)
 int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)
 {
 	int i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0, revarg_opt;
-	struct cmdline_pathspec prune_data;
+	struct argv_array prune_data = ARGV_ARRAY_INIT;
 	const char *submodule = NULL;
 
-	memset(&prune_data, 0, sizeof(prune_data));
 	if (opt)
 		submodule = opt->submodule;
 
@@ -2220,7 +2204,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			argv[i] = NULL;
 			argc = i;
 			if (argv[i + 1])
-				append_prune_data(&prune_data, argv + i + 1);
+				argv_array_pushv(&prune_data, argv + i + 1);
 			seen_dashdash = 1;
 			break;
 		}
@@ -2281,14 +2265,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			for (j = i; j < argc; j++)
 				verify_filename(revs->prefix, argv[j], j == i);
 
-			append_prune_data(&prune_data, argv + i);
+			argv_array_pushv(&prune_data, argv + i);
 			break;
 		}
 		else
 			got_rev_arg = 1;
 	}
 
-	if (prune_data.nr) {
+	if (prune_data.argc) {
 		/*
 		 * If we need to introduce the magic "a lone ':' means no
 		 * pathspec whatsoever", here is the place to do so.
@@ -2303,10 +2287,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		 *	call init_pathspec() to set revs->prune_data here.
 		 * }
 		 */
-		ALLOC_GROW(prune_data.path, prune_data.nr + 1, prune_data.alloc);
-		prune_data.path[prune_data.nr++] = NULL;
 		parse_pathspec(&revs->prune_data, 0, 0,
-			       revs->prefix, prune_data.path);
+			       revs->prefix, prune_data.argv);
 	}
 
 	if (revs->def == NULL)