{"thread":{"id":"46793","subject":"[PATCH] revision: fix memory leaks with `struct cmdline_pathspec`","startedAt":"2017-09-20T19:48:27Z","lastAt":"2017-09-21T05:10:14Z","messageCount":12,"participants":["Martin Ågren","Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"328491","messageId":"1505936846-2195-4-git-send-email-martin.agren@gmail.com","threadId":"46793","inReplyTo":null,"subject":"[PATCH] revision: fix memory leaks with `struct cmdline_pathspec`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-20T19:47:26Z","receivedAt":"2017-09-20T19:48:27Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We don't free the array `prune_data.path` or the individual strings it\npoints to. Do so by introducing and using `free_cmdline_pathspec()`. To\nbe able to safely free the strings, always use `xstrdup()` when\nassigning them. That does mean we allocate more memory than we used to,\nbut it also means it is clear who owns the strings and that we can stop\nleaking those that we do allocate.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n revision.c | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex f9a90d7..dfb6a6c 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1682,7 +1682,7 @@ static void append_prune_data(struct cmdline_pathspec *prune, const char **av)\n {\n \twhile (*av) {\n \t\tALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);\n-\t\tprune->path[prune->nr++] = *(av++);\n+\t\tprune->path[prune->nr++] = xstrdup(*(av++));\n \t}\n }\n \n@@ -1695,6 +1695,18 @@ static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,\n \t}\n }\n \n+static void free_cmdline_pathspec(struct cmdline_pathspec *prune)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < prune->nr; i++)\n+\t\tfree((void *)prune->path[i]);\n+\n+\tFREE_AND_NULL(prune->path);\n+\tprune->nr = 0;\n+\tprune->alloc = 0;\n+}\n+\n static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\t\t      struct cmdline_pathspec *prune)\n {\n@@ -2392,6 +2404,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\tprune_data.path[prune_data.nr++] = NULL;\n \t\tparse_pathspec(&revs->prune_data, 0, 0,\n \t\t\t       revs->prefix, prune_data.path);\n+\t\tfree_cmdline_pathspec(&prune_data);\n \t}\n \n \tif (revs->def == NULL)\n-- \n2.14.1\n\n"},{"id":"328499","messageId":"20170920202552.kkwhigmv7lq6cj3y@sigill.intra.peff.net","threadId":"46793","inReplyTo":"1505936846-2195-4-git-send-email-martin.agren@gmail.com","subject":"Re: [PATCH] revision: fix memory leaks with `struct cmdline_pathspec`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-20T20:25:52Z","receivedAt":"2017-09-20T20:25:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 20, 2017 at 09:47:26PM +0200, Martin Ågren wrote:\n\n> We don't free the array `prune_data.path` or the individual strings it\n> points to. Do so by introducing and using `free_cmdline_pathspec()`. To\n> be able to safely free the strings, always use `xstrdup()` when\n> assigning them. That does mean we allocate more memory than we used to,\n> but it also means it is clear who owns the strings and that we can stop\n> leaking those that we do allocate.\n\nHmm. From this description (and from looking at the patch), it seems\nlike we could just skip the allocation.\n\nThe missing piece of the puzzle is that sometimes we call\nappend_prune_data() to append from argv, and sometimes we use\nread_pathspec_from_stdin().\n\nSo we may literally have a mix-and-match of allocated and unallocated\nentries, and the only sane way to resolve that is by making them all\nallocated.\n\nSo I think this solves the problem, but I couldn't help notice...\n\n> @@ -1682,7 +1682,7 @@ static void append_prune_data(struct cmdline_pathspec *prune, const char **av)\n>  {\n>  \twhile (*av) {\n>  \t\tALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);\n> -\t\tprune->path[prune->nr++] = *(av++);\n> +\t\tprune->path[prune->nr++] = xstrdup(*(av++));\n>  \t}\n\nIsn't this whole thing just an argv_array, and this is argv_array_pushv?\nWe even NULL-terminate it manually later on!\n\nSo rather than increasing the line count by adding\nfree_cmdline_pathspec, I think we could actually _reduce_ it by\nconverting to an argv array, as below. And then adding in your free\nwould be one extra line.\n\ndiff --git a/revision.c b/revision.c\nindex 94a5e98525..5c58b3fb2b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -20,6 +20,7 @@\n #include \"cache-tree.h\"\n #include \"bisect.h\"\n #include \"packfile.h\"\n+#include \"argv-array.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -1612,31 +1613,15 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \treturn 0;\n }\n \n-struct cmdline_pathspec {\n-\tint alloc;\n-\tint nr;\n-\tconst char **path;\n-};\n-\n-static void append_prune_data(struct cmdline_pathspec *prune, const char **av)\n-{\n-\twhile (*av) {\n-\t\tALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);\n-\t\tprune->path[prune->nr++] = *(av++);\n-\t}\n-}\n-\n static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,\n-\t\t\t\t     struct cmdline_pathspec *prune)\n+\t\t\t\t     struct argv_array *prune)\n {\n-\twhile (strbuf_getline(sb, stdin) != EOF) {\n-\t\tALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);\n-\t\tprune->path[prune->nr++] = xstrdup(sb->buf);\n-\t}\n+\twhile (strbuf_getline(sb, stdin) != EOF)\n+\t\targv_array_push(prune, sb->buf);\n }\n \n static void read_revisions_from_stdin(struct rev_info *revs,\n-\t\t\t\t      struct cmdline_pathspec *prune)\n+\t\t\t\t      struct argv_array *prune)\n {\n \tstruct strbuf sb;\n \tint seen_dashdash = 0;\n@@ -2201,10 +2186,9 @@ static void NORETURN diagnose_missing_default(const char *def)\n int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)\n {\n \tint i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0, revarg_opt;\n-\tstruct cmdline_pathspec prune_data;\n+\tstruct argv_array prune_data = ARGV_ARRAY_INIT;\n \tconst char *submodule = NULL;\n \n-\tmemset(&prune_data, 0, sizeof(prune_data));\n \tif (opt)\n \t\tsubmodule = opt->submodule;\n \n@@ -2220,7 +2204,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\targv[i] = NULL;\n \t\t\targc = i;\n \t\t\tif (argv[i + 1])\n-\t\t\t\tappend_prune_data(&prune_data, argv + i + 1);\n+\t\t\t\targv_array_pushv(&prune_data, argv + i + 1);\n \t\t\tseen_dashdash = 1;\n \t\t\tbreak;\n \t\t}\n@@ -2281,14 +2265,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tfor (j = i; j < argc; j++)\n \t\t\t\tverify_filename(revs->prefix, argv[j], j == i);\n \n-\t\t\tappend_prune_data(&prune_data, argv + i);\n+\t\t\targv_array_pushv(&prune_data, argv + i);\n \t\t\tbreak;\n \t\t}\n \t\telse\n \t\t\tgot_rev_arg = 1;\n \t}\n \n-\tif (prune_data.nr) {\n+\tif (prune_data.argc) {\n \t\t/*\n \t\t * If we need to introduce the magic \"a lone ':' means no\n \t\t * pathspec whatsoever\", here is the place to do so.\n@@ -2303,10 +2287,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t *\tcall init_pathspec() to set revs->prune_data here.\n \t\t * }\n \t\t */\n-\t\tALLOC_GROW(prune_data.path, prune_data.nr + 1, prune_data.alloc);\n-\t\tprune_data.path[prune_data.nr++] = NULL;\n \t\tparse_pathspec(&revs->prune_data, 0, 0,\n-\t\t\t       revs->prefix, prune_data.path);\n+\t\t\t       revs->prefix, prune_data.argv);\n \t}\n \n \tif (revs->def == NULL)\n"},{"id":"328500","messageId":"20170920203659.xqy76bg5nfabvbfx@sigill.intra.peff.net","threadId":"46793","inReplyTo":"20170920202552.kkwhigmv7lq6cj3y@sigill.intra.peff.net","subject":"[PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-20T20:36:59Z","receivedAt":"2017-09-20T20:37:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 20, 2017 at 04:25:52PM -0400, Jeff King wrote:\n\n> Isn't this whole thing just an argv_array, and this is argv_array_pushv?\n> We even NULL-terminate it manually later on!\n> \n> So rather than increasing the line count by adding\n> free_cmdline_pathspec, I think we could actually _reduce_ it by\n> converting to an argv array, as below. And then adding in your free\n> would be one extra line.\n\nHere it is with a commit message, and that final free added.\n\nSorry for stealing your patch, but I didn't want to suggest \"couldn't\nyou replace this with argv_array\" without actually seeing if it was\npossible. At which point the patch was pretty much done.\n\n-- >8 --\nSubject: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array\n\nWe assemble an array of strings in a custom struct,\nNULL-terminate the result, and then pass it to\nparse_pathspec().\n\nBut then we never free the array or the individual strings\n(nor can we do the latter, as they are heap-allocated when\nthey come from stdin but not when they come from the\npassed-in argv).\n\nLet's swap this out for an argv_array. It does the same\nthing with fewer lines of code, and it's safe to call\nargv_array_clear() at the end to avoid a memory leak.\n\nReported-by: Martin Ågren <martin.agren@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n revision.c | 39 +++++++++++----------------------------\n 1 file changed, 11 insertions(+), 28 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex f9a90d71d2..1520f69d93 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -21,6 +21,7 @@\n #include \"bisect.h\"\n #include \"packfile.h\"\n #include \"worktree.h\"\n+#include \"argv-array.h\"\n \n volatile show_early_output_fn_t show_early_output;\n \n@@ -1672,31 +1673,15 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \treturn 0;\n }\n \n-struct cmdline_pathspec {\n-\tint alloc;\n-\tint nr;\n-\tconst char **path;\n-};\n-\n-static void append_prune_data(struct cmdline_pathspec *prune, const char **av)\n-{\n-\twhile (*av) {\n-\t\tALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);\n-\t\tprune->path[prune->nr++] = *(av++);\n-\t}\n-}\n-\n static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,\n-\t\t\t\t     struct cmdline_pathspec *prune)\n+\t\t\t\t     struct argv_array *prune)\n {\n-\twhile (strbuf_getline(sb, stdin) != EOF) {\n-\t\tALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);\n-\t\tprune->path[prune->nr++] = xstrdup(sb->buf);\n-\t}\n+\twhile (strbuf_getline(sb, stdin) != EOF)\n+\t\targv_array_push(prune, sb->buf);\n }\n \n static void read_revisions_from_stdin(struct rev_info *revs,\n-\t\t\t\t      struct cmdline_pathspec *prune)\n+\t\t\t\t      struct argv_array *prune)\n {\n \tstruct strbuf sb;\n \tint seen_dashdash = 0;\n@@ -2286,10 +2271,9 @@ static void NORETURN diagnose_missing_default(const char *def)\n int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt)\n {\n \tint i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0, revarg_opt;\n-\tstruct cmdline_pathspec prune_data;\n+\tstruct argv_array prune_data = ARGV_ARRAY_INIT;\n \tconst char *submodule = NULL;\n \n-\tmemset(&prune_data, 0, sizeof(prune_data));\n \tif (opt)\n \t\tsubmodule = opt->submodule;\n \n@@ -2305,7 +2289,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\targv[i] = NULL;\n \t\t\targc = i;\n \t\t\tif (argv[i + 1])\n-\t\t\t\tappend_prune_data(&prune_data, argv + i + 1);\n+\t\t\t\targv_array_pushv(&prune_data, argv + i + 1);\n \t\t\tseen_dashdash = 1;\n \t\t\tbreak;\n \t\t}\n@@ -2366,14 +2350,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\tfor (j = i; j < argc; j++)\n \t\t\t\tverify_filename(revs->prefix, argv[j], j == i);\n \n-\t\t\tappend_prune_data(&prune_data, argv + i);\n+\t\t\targv_array_pushv(&prune_data, argv + i);\n \t\t\tbreak;\n \t\t}\n \t\telse\n \t\t\tgot_rev_arg = 1;\n \t}\n \n-\tif (prune_data.nr) {\n+\tif (prune_data.argc) {\n \t\t/*\n \t\t * If we need to introduce the magic \"a lone ':' means no\n \t\t * pathspec whatsoever\", here is the place to do so.\n@@ -2388,11 +2372,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t *\tcall init_pathspec() to set revs->prune_data here.\n \t\t * }\n \t\t */\n-\t\tALLOC_GROW(prune_data.path, prune_data.nr + 1, prune_data.alloc);\n-\t\tprune_data.path[prune_data.nr++] = NULL;\n \t\tparse_pathspec(&revs->prune_data, 0, 0,\n-\t\t\t       revs->prefix, prune_data.path);\n+\t\t\t       revs->prefix, prune_data.argv);\n \t}\n+\targv_array_clear(&prune_data);\n \n \tif (revs->def == NULL)\n \t\trevs->def = opt ? opt->def : NULL;\n-- \n2.14.1.1040.gcaf8795f39\n\n"},{"id":"328508","messageId":"20170920224826.GH27425@aiede.mtv.corp.google.com","threadId":"46793","inReplyTo":"20170920203659.xqy76bg5nfabvbfx@sigill.intra.peff.net","subject":"Re: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-20T22:48:26Z","receivedAt":"2017-09-20T22:48:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> Subject: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array\n>\n> We assemble an array of strings in a custom struct,\n> NULL-terminate the result, and then pass it to\n> parse_pathspec().\n>\n> But then we never free the array or the individual strings\n> (nor can we do the latter, as they are heap-allocated when\n> they come from stdin but not when they come from the\n> passed-in argv).\n\nTo be devil's advocate for a moment: why don't we use UNLEAK on the\npaths passed in from stdin?\n\nIt's true that there can be an unbounded number of them, but they all\ncoexist in memory anyway.  They are not generated dynamically on the\nfly.  Being able to free them doesn't have any obvious advantage over\nusing exit().\n\nExcept... is the idea that this allows the strings from stdin to be\nfreed sooner, as soon as they have been parsed into a \"struct\npathspec\"?\n\nThat sounds appealing.  The only risk would be if \"struct pathspec\"\nholds on to a pointer to one of these strings.\n\nFortunately parse_pathspec() is careful to strdup any strings it\nborrows (though it is not documented to do so).\n\nIn other words, proposed changes:\n\n 1. Could the commit message describe what effect this would have on\n    maximum heap usage, if any?  (In qualitative terms is fine, though\n    actual numbers would be even better if it's easy to get them.)\n    That would make it easier to justify not using UNLEAK.\n\n 2. Can parse_pathspec get a comment in pathspec.h saying that it\n    defensively copies anything it needs from args so the caller is\n    free to modify or free it?  That way, it should be more obvious\n    to people in the future modifying parse_pathspec() that callers\n    may rely on that.  (The current API comment describes argv as\n    \"command line arguments\", which I fear would send the opposite\n    message to implementors.)\n\n> Let's swap this out for an argv_array. It does the same\n> thing with fewer lines of code, and it's safe to call\n> argv_array_clear() at the end to avoid a memory leak.\n>\n> Reported-by: Martin Ågren <martin.agren@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  revision.c | 39 +++++++++++----------------------------\n>  1 file changed, 11 insertions(+), 28 deletions(-)\n\nThe code looks good.\n\nThanks,\nJonathan\n"},{"id":"328529","messageId":"20170921030424.akqaou7tqj2updgr@sigill.intra.peff.net","threadId":"46793","inReplyTo":"20170920224826.GH27425@aiede.mtv.corp.google.com","subject":"Re: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-21T03:04:24Z","receivedAt":"2017-09-21T03:04:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 20, 2017 at 03:48:26PM -0700, Jonathan Nieder wrote:\n\n> > Subject: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array\n> >\n> > We assemble an array of strings in a custom struct,\n> > NULL-terminate the result, and then pass it to\n> > parse_pathspec().\n> >\n> > But then we never free the array or the individual strings\n> > (nor can we do the latter, as they are heap-allocated when\n> > they come from stdin but not when they come from the\n> > passed-in argv).\n> \n> To be devil's advocate for a moment: why don't we use UNLEAK on the\n> paths passed in from stdin?\n> \n> It's true that there can be an unbounded number of them, but they all\n> coexist in memory anyway.  They are not generated dynamically on the\n> fly.  Being able to free them doesn't have any obvious advantage over\n> using exit().\n>\n> Except... is the idea that this allows the strings from stdin to be\n> freed sooner, as soon as they have been parsed into a \"struct\n> pathspec\"?\n\nWell, no...the idea is that this is a function which leaks a bunch of\nmemory, and we shouldn't have to think hard about how often its leak can\nbe triggered or how severe it is. We should just fix it.\n\nI agree with your analysis that we're likely only to leak one set of\n--stdin input per program invocation (modulo some caller re-opening\nstdin). And I also agree that it's a waste of memory to hold onto leaked\nheap that cannot be used.\n\nBut mostly I am fundamentally against using UNLEAK() in a case like\nthis, because it does not match either of the properties which justified\nadding UNLEAK() in the first place:\n\n  1. We are about to exit the program, so the \"leak\" is only caused by\n     the memory going out of scope at that exit.\n\n     By contrast, the revision machinery may be called many times in the\n     same program.\n\n  2. The memory remains useful until around the time of program exit.\n\n     This most certainly does not, as it would not even be reachable.\n\nSo while it may not do _too_ much harm, aside from increasing peak heap\nunnecessarily, it's not a precedent I'd like to set. And certainly not\nwhen we can fix the leak and reduce the code size at the same time.\n\n> That sounds appealing.  The only risk would be if \"struct pathspec\"\n> holds on to a pointer to one of these strings.\n> \n> Fortunately parse_pathspec() is careful to strdup any strings it\n> borrows (though it is not documented to do so).\n\nA lot of the early interfaces in Git had really confusing\nmemory-ownership semantics. I think we've been slowly moving over the\nyears towards interfaces that spend a bit on extra copies to give simple\nand consistent semantics (in fact, it was one of the reasons I added\nargv_array in the first place).\n\nSo yes, it's good to double check that parse_pathspec() doesn't want to\nhold on to the pointers. But I also think that should be the default\nwe're striving for in our APIs.\n\n> In other words, proposed changes:\n> \n>  1. Could the commit message describe what effect this would have on\n>     maximum heap usage, if any?  (In qualitative terms is fine, though\n>     actual numbers would be even better if it's easy to get them.)\n>     That would make it easier to justify not using UNLEAK.\n\nWhat wording are you looking for? It was a leak, and now it's gone.  The\nsize of the leak depends on how much you feed to --stdin. IMHO using\nUNLEAK is totally inappropriate for this case, and doesn't even seem\nlike an alternative worth rejecting.\n\n>  2. Can parse_pathspec get a comment in pathspec.h saying that it\n>     defensively copies anything it needs from args so the caller is\n>     free to modify or free it?  That way, it should be more obvious\n>     to people in the future modifying parse_pathspec() that callers\n>     may rely on that.  (The current API comment describes argv as\n>     \"command line arguments\", which I fear would send the opposite\n>     message to implementors.)\n\nI certainly agree that the pathspec interface could use better\ndocumentation. Patches welcome? :)\n\n-Peff\n"},{"id":"328531","messageId":"20170921034900.GB88098@aiede.mtv.corp.google.com","threadId":"46793","inReplyTo":"20170921030424.akqaou7tqj2updgr@sigill.intra.peff.net","subject":"Re: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-21T03:49:00Z","receivedAt":"2017-09-21T03:50:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> But mostly I am fundamentally against using UNLEAK() in a case like\n> this, because it does not match either of the properties which justified\n> adding UNLEAK() in the first place:\n>\n>   1. We are about to exit the program, so the \"leak\" is only caused by\n>      the memory going out of scope at that exit.\n>\n>      By contrast, the revision machinery may be called many times in the\n>      same program.\n>\n>   2. The memory remains useful until around the time of program exit.\n>\n>      This most certainly does not, as it would not even be reachable.\n[...]\n> On Wed, Sep 20, 2017 at 03:48:26PM -0700, Jonathan Nieder wrote:\n\n>> In other words, proposed changes:\n>>\n>>  1. Could the commit message describe what effect this would have on\n>>     maximum heap usage, if any?  (In qualitative terms is fine, though\n>>     actual numbers would be even better if it's easy to get them.)\n>>     That would make it easier to justify not using UNLEAK.\n>\n> What wording are you looking for? It was a leak, and now it's gone.  The\n> size of the leak depends on how much you feed to --stdin. IMHO using\n> UNLEAK is totally inappropriate for this case, and doesn't even seem\n> like an alternative worth rejecting.\n>\n>>  2. Can parse_pathspec get a comment in pathspec.h saying that it\n>>     defensively copies anything it needs from args so the caller is\n>>     free to modify or free it?  That way, it should be more obvious\n>>     to people in the future modifying parse_pathspec() that callers\n>>     may rely on that.  (The current API comment describes argv as\n>>     \"command line arguments\", which I fear would send the opposite\n>>     message to implementors.)\n>\n> I certainly agree that the pathspec interface could use better\n> documentation. Patches welcome? :)\n\nI think I failed at communicating here.  That is not what I meant at\nall.\n\nThe context is that Git (especially older parts of it) suffers from a\npretty severe lack of API documentation.  For newcomers that is\nespecially obvious --- long-time git contributors, on the other hand,\nmay get used to patterns and common interfaces and may have trouble\nseeing just how bad the lack of clearly communicated API contracts is.\n\nThere is a bit of a \"broken window\" problem, too: authors of one-off\npatches may reasonably assume from existing code that this is just the\nway it is and, lacking examples of how to document an API, add more\nunderdocumented API contracts.\n\nThe patch I am replying to tightens the contract for parse_pathspec().\nI am not asking for comprehensive documentation of that function ---\nthat would be clearly off-topic for the patch.  Instead, I am saying\nthat we should document what we are newly requiring of the function in\nthe patch.  That way, the documented contract becomes clearer over\ntime as people document what they are relying on.  I think of that as\ngenerally a good practice.\n\nIn other words, it was not at all obvious that \"(2) The memory remains\nuseful until around the time of program exit\" did not hold.  To a\ncasual reader it instead looks like (2) does hold, and there's no\ndocumentation short of delving into the implementation of\nparse_pathspec() to convince a reader otherwise.  The documentation\nthat is present leads to the opposite conclusion.\n\nThe assertion (1) that this allocation is going to happen multiple\ntimes in a program isn't true either.  As you noted, we only read\nstdin once.  But that doesn't matter as long as (2) doesn't hold.\n\nTBH saying that I should write the one-sentence doc patch feels like\na cop-out.  Just like it is not sustainable for those reviewers that\nare interested in good test coverage to be the only ones who write\ntests, I think we cannot avoid treating documentation of API contracts\nas a shared responsibility.\n\nThanks and hope that clarifies,\nJonathan\n"},{"id":"328533","messageId":"CAN0heSoWnUXSjP1HtRTKpBiOc9JNzZN_+m1O3_Wn+rE43MpPhg@mail.gmail.com","threadId":"46793","inReplyTo":"20170920203659.xqy76bg5nfabvbfx@sigill.intra.peff.net","subject":"Re: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-21T03:57:39Z","receivedAt":"2017-09-21T03:57:44Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 20 September 2017 at 22:36, Jeff King <peff@peff.net> wrote:\n> On Wed, Sep 20, 2017 at 04:25:52PM -0400, Jeff King wrote:\n>\n>> Isn't this whole thing just an argv_array, and this is argv_array_pushv?\n>> We even NULL-terminate it manually later on!\n>>\n>> So rather than increasing the line count by adding\n>> free_cmdline_pathspec, I think we could actually _reduce_ it by\n>> converting to an argv array, as below. And then adding in your free\n>> would be one extra line.\n>\n> Here it is with a commit message, and that final free added.\n>\n> Sorry for stealing your patch, but I didn't want to suggest \"couldn't\n> you replace this with argv_array\" without actually seeing if it was\n> possible. At which point the patch was pretty much done.\n\nNo need to be sorry. I wasn't aware of the argv_array thing, thanks for\nseeing the bigger picture. I haven't looked into the details, or your\nand Jonathan's discussion, but just from a cursory look this looks way\nbetter. It reuses existing infrastructure, and then this:\n\n>  revision.c | 39 +++++++++++----------------------------\n>  1 file changed, 11 insertions(+), 28 deletions(-)\n\nMartin\n"},{"id":"328534","messageId":"xmqqingclmh3.fsf@gitster.mtv.corp.google.com","threadId":"46793","inReplyTo":"20170920203659.xqy76bg5nfabvbfx@sigill.intra.peff.net","subject":"Re: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-21T04:11:36Z","receivedAt":"2017-09-21T04:11:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array\n>\n> We assemble an array of strings in a custom struct,\n> NULL-terminate the result, and then pass it to\n> parse_pathspec().\n>\n> But then we never free the array or the individual strings\n> (nor can we do the latter, as they are heap-allocated when\n> they come from stdin but not when they come from the\n> passed-in argv).\n>\n> Let's swap this out for an argv_array. It does the same\n> thing with fewer lines of code, and it's safe to call\n> argv_array_clear() at the end to avoid a memory leak.\n>\n> Reported-by: Martin Ågren <martin.agren@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  revision.c | 39 +++++++++++----------------------------\n>  1 file changed, 11 insertions(+), 28 deletions(-)\n\nMakes sense.  Thanks for cleaning up the mess I made.\n"},{"id":"328536","messageId":"20170921044112.GB91069@aiede.mtv.corp.google.com","threadId":"46793","inReplyTo":"20170921030424.akqaou7tqj2updgr@sigill.intra.peff.net","subject":"Re: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-21T04:41:12Z","receivedAt":"2017-09-21T04:41:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Wed, Sep 20, 2017 at 03:48:26PM -0700, Jonathan Nieder wrote:\n>> Jeff King wrote:\n\n>>> Subject: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array\n>>>\n>>> We assemble an array of strings in a custom struct,\n>>> NULL-terminate the result, and then pass it to\n>>> parse_pathspec().\n>>>\n>>> But then we never free the array or the individual strings\n>>> (nor can we do the latter, as they are heap-allocated when\n>>> they come from stdin but not when they come from the\n>>> passed-in argv).\n[...]\n>> Except... is the idea that this allows the strings from stdin to be\n>> freed sooner, as soon as they have been parsed into a \"struct\n>> pathspec\"?\n>\n> Well, no...the idea is that this is a function which leaks a bunch of\n> memory, and we shouldn't have to think hard about how often its leak can\n> be triggered or how severe it is. We should just fix it.\n\nI forgot to say: thanks for writing such a pleasant patch.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\n[...]\n> I certainly agree that the pathspec interface could use better\n> documentation. Patches welcome? :)\n\nHere's such a patch.\n\n-- 8< --\nSubject: pathspec doc: parse_pathspec does not maintain references to args\n\nThe command line arguments passed to main() are valid for the life of\na program, but the same is not true for all other argv-style arrays\n(e.g.  when a caller creates an argv_array).  Clarify that\nparse_pathspec does not rely on the argv passed to it to remain valid.\n\nThis makes it easier to tell that callers like \"git rev-list --stdin\"\nare safe and ensures that that is more likely to remain true as the\nimplementation of parse_pathspec evolves.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n pathspec.c | 4 ----\n pathspec.h | 7 +++++++\n 2 files changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex e2a23ebc96..cdefdc7cc0 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -526,10 +526,6 @@ static void NORETURN unsupported_magic(const char *pattern,\n \t    pattern, sb.buf);\n }\n \n-/*\n- * Given command line arguments and a prefix, convert the input to\n- * pathspec. die() if any magic in magic_mask is used.\n- */\n void parse_pathspec(struct pathspec *pathspec,\n \t\t    unsigned magic_mask, unsigned flags,\n \t\t    const char *prefix, const char **argv)\ndiff --git a/pathspec.h b/pathspec.h\nindex 60e6500401..6420d1080a 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -70,6 +70,13 @@ struct pathspec {\n  */\n #define PATHSPEC_LITERAL_PATH (1<<6)\n \n+/*\n+ * Given command line arguments and a prefix, convert the input to\n+ * pathspec. die() if any magic in magic_mask is used.\n+ *\n+ * Any arguments used are copied. It is safe for the caller to modify\n+ * or free 'prefix' and 'args' after calling this function.\n+ */\n extern void parse_pathspec(struct pathspec *pathspec,\n \t\t\t   unsigned magic_mask,\n \t\t\t   unsigned flags,\n-- \n2.14.1.821.g8fa685d3b7\n\n"},{"id":"328537","messageId":"20170921044800.2m4vi5zd2nyer3bc@sigill.intra.peff.net","threadId":"46793","inReplyTo":"20170921034900.GB88098@aiede.mtv.corp.google.com","subject":"Re: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-21T04:48:00Z","receivedAt":"2017-09-21T04:48:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 20, 2017 at 08:49:00PM -0700, Jonathan Nieder wrote:\n\n> The patch I am replying to tightens the contract for parse_pathspec().\n\nI disagree with this bit. In my view that was already the contract for\nparse_pathspec(), and it is simply poorly documented. You can see it\nbeing required elsewhere already (e.g., in line_log_init(), and quite\nprobably in other places that feed custom argv-arrays to\nsetup_revisions).\n\n> In other words, it was not at all obvious that \"(2) The memory remains\n> useful until around the time of program exit\" did not hold.  To a\n> casual reader it instead looks like (2) does hold, and there's no\n> documentation short of delving into the implementation of\n> parse_pathspec() to convince a reader otherwise.  The documentation\n> that is present leads to the opposite conclusion.\n\nI guess the point I was trying to make is that I _didn't_ have that\nopposite conclusion. A sane API does not create hidden memory ownership\ndependencies (though also as I said, it does not hurt to double check if\nyou think it may be wrong, and certainly documenting also does not\nhurt).\n\n> The assertion (1) that this allocation is going to happen multiple\n> times in a program isn't true either.  As you noted, we only read\n> stdin once.  But that doesn't matter as long as (2) doesn't hold.\n\nI don't think I asserted that this allocation happens multiple times. I\n_did_ assert that the revision machinery may be called multiple times,\nand I'd rather fix the leak than have people figure out the maximum\nnumber of bytes we might leak in a given program. In other words, the\nmore important point of what I wrote is that we should not be reaching\nfor UNLEAK() at all in a function like this, even if you might be able\nto justify it after analysis.\n\n> TBH saying that I should write the one-sentence doc patch feels like\n> a cop-out.  Just like it is not sustainable for those reviewers that\n> are interested in good test coverage to be the only ones who write\n> tests, I think we cannot avoid treating documentation of API contracts\n> as a shared responsibility.\n\nHopefully the first paragraph I wrote above explains why I don't see\nthis as changing the contract at all (and therefore making the\ndocumentation update orthogonal).\n\nI agree that it's a good practice to leave the whole codebase a little\nprettier than when you started, even if it's not strictly related to the\nsmall change you want to make. And I hope my record of patches shows\nthat I follow through on that practice as a general rule.\n\nBut I also think it's easy for reviewers to ask for those orthogonal\nchanges, when it would be a similar amount of work for them to\ncommunicate their suggestion in the form of a patch.\n\n-Peff\n"},{"id":"328538","messageId":"20170921045014.yuln2edqybwusyrk@sigill.intra.peff.net","threadId":"46793","inReplyTo":"20170921044112.GB91069@aiede.mtv.corp.google.com","subject":"Re: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-21T04:50:14Z","receivedAt":"2017-09-21T04:50:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 20, 2017 at 09:41:12PM -0700, Jonathan Nieder wrote:\n\n> > Well, no...the idea is that this is a function which leaks a bunch of\n> > memory, and we shouldn't have to think hard about how often its leak can\n> > be triggered or how severe it is. We should just fix it.\n> \n> I forgot to say: thanks for writing such a pleasant patch.\n> \n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nYou're welcome. :)\n\n> Here's such a patch.\n>\n> -- 8< --\n> Subject: pathspec doc: parse_pathspec does not maintain references to args\n> [...]\n\nYay!\n\nThis looks great to me. I think the whole pathspec API could use better\ndocumentation, but this is certainly a good start.\n\n-Peff\n"},{"id":"328543","messageId":"xmqq7ewsljrl.fsf@gitster.mtv.corp.google.com","threadId":"46793","inReplyTo":"20170921044112.GB91069@aiede.mtv.corp.google.com","subject":"Re: [PATCH] revision: replace \"struct cmdline_pathspec\" with argv_array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-21T05:10:06Z","receivedAt":"2017-09-21T05:10:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> diff --git a/pathspec.c b/pathspec.c\n> index e2a23ebc96..cdefdc7cc0 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -526,10 +526,6 @@ static void NORETURN unsupported_magic(const char *pattern,\n>  \t    pattern, sb.buf);\n>  }\n>  \n> -/*\n> - * Given command line arguments and a prefix, convert the input to\n> - * pathspec. die() if any magic in magic_mask is used.\n> - */\n>  void parse_pathspec(struct pathspec *pathspec,\n>  \t\t    unsigned magic_mask, unsigned flags,\n>  \t\t    const char *prefix, const char **argv)\n> diff --git a/pathspec.h b/pathspec.h\n> index 60e6500401..6420d1080a 100644\n> --- a/pathspec.h\n> +++ b/pathspec.h\n> @@ -70,6 +70,13 @@ struct pathspec {\n>   */\n>  #define PATHSPEC_LITERAL_PATH (1<<6)\n>  \n> +/*\n> + * Given command line arguments and a prefix, convert the input to\n> + * pathspec. die() if any magic in magic_mask is used.\n> + *\n> + * Any arguments used are copied. It is safe for the caller to modify\n> + * or free 'prefix' and 'args' after calling this function.\n> + */\n>  extern void parse_pathspec(struct pathspec *pathspec,\n>  \t\t\t   unsigned magic_mask,\n>  \t\t\t   unsigned flags,\n\nObviously the extra text is better than not having any, but I\nsomehow found \"Any arguments used\" a bit unsatisfactory.  magic_mask\nand flags are probably also copied ;-) but I wonder if *pathspec is\nalso copied?  The second sentence that singles out 'prefix' and 'args'\nhelps to remove such a confusion from a clueless reader like me, and\nmakes me wonder if can take advantage of the existence of them in a\nmore direct way.\n\n\tIt is safe for the caller to modify or free 'prefix' and\n\t'args' after calling this function, as copies of them are\n\tstored in the pathspec structure.\n\nor something like that, perhaps.  Adding \"(which is freed by calling\nclear_pathspec())\" at the end might even better, as that function\ndoes not currently have any docstring.\n\n"}]}