Re: [PATCH] revision: fix memory leaks with `struct cmdline_pathspec`
- From
Jeff King <peff@peff.net>
- Date
- Sep 20, 2017, 20:25 UTC
- Message-ID
- <20170920202552.kkwhigmv7lq6cj3y@sigill.intra.peff.net>
- In-Reply-To
- <1505936846-2195-4-git-send-email-martin.agren@gmail.com>
On Wed, Sep 20, 2017 at 09:47:26PM +0200, Martin Ågren wrote:
Show 6 quoted lines
> 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...
Show 7 quoted lines
> @@ -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)