{"thread":{"id":"58942","subject":"[PATCH] am: don't pass strvec to apply_parse_options()","startedAt":"2022-12-13T06:48:11Z","lastAt":"2023-01-07T13:21:35Z","messageCount":24,"participants":["René Scharfe","Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"468972","messageId":"baf93e4a-7f05-857c-e551-09675496c03c@web.de","threadId":"58942","inReplyTo":null,"subject":"[PATCH] am: don't pass strvec to apply_parse_options()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-13T06:47:59Z","receivedAt":"2022-12-13T06:48:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"apply_parse_options() passes the array of argument strings to\nparse_options(), which removes recognized options. The removed strings\nare not freed, though.\n\nMake a copy of the strvec to pass to the function to retain the pointers\nof its strings, so we release them all at the end.\n\nSigned-off-by: René Scharfe \n---\n builtin/am.c | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 30c9b3a9cd..dddf1b9af0 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1476,6 +1476,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \tint res, opts_left;\n \tint force_apply = 0;\n \tint options = 0;\n+\tconst char **apply_argv;\n\n \tif (init_apply_state(&apply_state, the_repository, NULL))\n \t\tBUG(\"init_apply_state() failed\");\n@@ -1483,7 +1484,15 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \tstrvec_push(&apply_opts, \"apply\");\n \tstrvec_pushv(&apply_opts, state->git_apply_opts.v);\n\n-\topts_left = apply_parse_options(apply_opts.nr, apply_opts.v,\n+\t/*\n+\t * Build a copy that apply_parse_options() can rearrange.\n+\t * apply_opts.v keeps referencing the allocated strings for\n+\t * strvec_clear() to release.\n+\t */\n+\tALLOC_ARRAY(apply_argv, apply_opts.nr);\n+\tCOPY_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);\n+\n+\topts_left = apply_parse_options(apply_opts.nr, apply_argv,\n \t\t\t\t\t&apply_state, &force_apply, &options,\n \t\t\t\t\tNULL);\n\n@@ -1513,6 +1522,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \tstrvec_clear(&apply_paths);\n \tstrvec_clear(&apply_opts);\n \tclear_apply_state(&apply_state);\n+\tfree(apply_argv);\n\n \tif (res)\n \t\treturn res;\n--\n2.38.2\n"},{"id":"468973","messageId":"221213.86mt7r4ru2.gmgdl@evledraar.gmail.com","threadId":"58942","inReplyTo":"baf93e4a-7f05-857c-e551-09675496c03c@web.de","subject":"Re: [PATCH] am: don't pass strvec to apply_parse_options()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-13T08:37:24Z","receivedAt":"2022-12-13T08:43:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 13 2022, René Scharfe wrote:\n\n> apply_parse_options() passes the array of argument strings to\n> parse_options(), which removes recognized options. The removed strings\n> are not freed, though.\n>\n> Make a copy of the strvec to pass to the function to retain the pointers\n> of its strings, so we release them all at the end.\n>\n> Signed-off-by: René Scharfe \n> ---\n> builtin/am.c | 12 +++++++++++-\n> 1 file changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 30c9b3a9cd..dddf1b9af0 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1476,6 +1476,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n> \tint res, opts_left;\n> \tint force_apply = 0;\n> \tint options = 0;\n> +\tconst char **apply_argv;\n>\n> \tif (init_apply_state(&apply_state, the_repository, NULL))\n> \t\tBUG(\"init_apply_state() failed\");\n> @@ -1483,7 +1484,15 @@ static int run_apply(const struct am_state *state, const char *index_file)\n> \tstrvec_push(&apply_opts, \"apply\");\n> \tstrvec_pushv(&apply_opts, state->git_apply_opts.v);\n>\n> -\topts_left = apply_parse_options(apply_opts.nr, apply_opts.v,\n> +\t/*\n> +\t * Build a copy that apply_parse_options() can rearrange.\n> +\t * apply_opts.v keeps referencing the allocated strings for\n> +\t * strvec_clear() to release.\n> +\t */\n> +\tALLOC_ARRAY(apply_argv, apply_opts.nr);\n> +\tCOPY_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);\n> +\n> +\topts_left = apply_parse_options(apply_opts.nr, apply_argv,\n> \t\t\t\t\t&apply_state, &force_apply, &options,\n> \t\t\t\t\tNULL);\n>\n> @@ -1513,6 +1522,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n> \tstrvec_clear(&apply_paths);\n> \tstrvec_clear(&apply_opts);\n> \tclear_apply_state(&apply_state);\n> +\tfree(apply_argv);\n>\n> \tif (res)\n> \t\treturn res;\n\nI don't mind this going in, but it really feels like a bit of a dirty\nhack.\n\nWe have widespread leaks all over the place due to this\nidiom. I.e. parse_options() and a couple of other APIs expect that they\ncan munge the \"argv\", which is fine if it arrives via main(), but not if\nwe're editing our own strvecs.\n\nI think less of a hack is to teach the eventual parse_options() that\nwhen it munges it it should free() it. I did that for the revisions API\nin f92dbdbc6a8 (revisions API: don't leak memory on argv elements that\nneed free()-ing, 2022-08-02).\n\nWhat do you think?\n"},{"id":"468995","messageId":"2d0d77a4-f6ac-1fa7-bddb-9083579d8dd7@web.de","threadId":"58942","inReplyTo":"221213.86mt7r4ru2.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] am: don't pass strvec to apply_parse_options()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-13T18:31:13Z","receivedAt":"2022-12-13T18:31:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.12.22 um 09:37 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, Dec 13 2022, René Scharfe wrote:\n>\n>> apply_parse_options() passes the array of argument strings to\n>> parse_options(), which removes recognized options. The removed strings\n>> are not freed, though.\n>>\n>> Make a copy of the strvec to pass to the function to retain the pointers\n>> of its strings, so we release them all at the end.\n>>\n>> Signed-off-by: René Scharfe \n>> ---\n>> builtin/am.c | 12 +++++++++++-\n>> 1 file changed, 11 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index 30c9b3a9cd..dddf1b9af0 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -1476,6 +1476,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n>> \tint res, opts_left;\n>> \tint force_apply = 0;\n>> \tint options = 0;\n>> +\tconst char **apply_argv;\n>>\n>> \tif (init_apply_state(&apply_state, the_repository, NULL))\n>> \t\tBUG(\"init_apply_state() failed\");\n>> @@ -1483,7 +1484,15 @@ static int run_apply(const struct am_state *state, const char *index_file)\n>> \tstrvec_push(&apply_opts, \"apply\");\n>> \tstrvec_pushv(&apply_opts, state->git_apply_opts.v);\n>>\n>> -\topts_left = apply_parse_options(apply_opts.nr, apply_opts.v,\n>> +\t/*\n>> +\t * Build a copy that apply_parse_options() can rearrange.\n>> +\t * apply_opts.v keeps referencing the allocated strings for\n>> +\t * strvec_clear() to release.\n>> +\t */\n>> +\tALLOC_ARRAY(apply_argv, apply_opts.nr);\n>> +\tCOPY_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);\n>> +\n>> +\topts_left = apply_parse_options(apply_opts.nr, apply_argv,\n>> \t\t\t\t\t&apply_state, &force_apply, &options,\n>> \t\t\t\t\tNULL);\n>>\n>> @@ -1513,6 +1522,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n>> \tstrvec_clear(&apply_paths);\n>> \tstrvec_clear(&apply_opts);\n>> \tclear_apply_state(&apply_state);\n>> +\tfree(apply_argv);\n>>\n>> \tif (res)\n>> \t\treturn res;\n>\n> I don't mind this going in, but it really feels like a bit of a dirty\n> hack.\n>\n> We have widespread leaks all over the place due to this\n> idiom. I.e. parse_options() and a couple of other APIs expect that they\n> can munge the \"argv\", which is fine if it arrives via main(), but not if\n> we're editing our own strvecs.\n\nWhere? A quick \"git grep 'parse_options.*nr'\" turns up only this place\nas one that passes a strvec to parse_options.\n\n> I think less of a hack is to teach the eventual parse_options() that\n> when it munges it it should free() it. I did that for the revisions API\n> in f92dbdbc6a8 (revisions API: don't leak memory on argv elements that\n> need free()-ing, 2022-08-02).\n>\n> What do you think?\n\nGenerating string lists and then parsing them is weird. When calls have\nto cross a process boundary then we have no choice, but in-process we\nshouldn't have to lower our request to an intermediate text format. git\nam does it anyway because it writes its options to a file and reads them\nback when it resumes with --continue, IIUC.\n\nI hope that is and will be the only place that uses parse_options() with\na strvec -- and then we don't have to change that function.\n\nIf this pattern is used more widely then we could package the copying\ndone by this patch somehow, e.g. by adding a strvec_parse_options()\nthat wraps the real thing.\n\nIf we have to change parse_options() at all then I'd prefer it to not\nfree() anything (to keep it usable with main()'s parameters), but to\nreorder in a non-destructive way. That would mean keeping the NULL\nsentinel where it is, and making sure all callers use only the returned\nargc to determine which arguments parse_options() didn't recognize.\n\nRené\n"},{"id":"469006","messageId":"221214.86ilie48cv.gmgdl@evledraar.gmail.com","threadId":"58942","inReplyTo":"2d0d77a4-f6ac-1fa7-bddb-9083579d8dd7@web.de","subject":"Re: [PATCH] am: don't pass strvec to apply_parse_options()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-14T08:44:12Z","receivedAt":"2022-12-14T09:56:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Dec 13 2022, René Scharfe wrote:\n\n> Am 13.12.22 um 09:37 schrieb Ævar Arnfjörð Bjarmason:\n>>\n>> On Tue, Dec 13 2022, René Scharfe wrote:\n>>\n>>> apply_parse_options() passes the array of argument strings to\n>>> parse_options(), which removes recognized options. The removed strings\n>>> are not freed, though.\n>>>\n>>> Make a copy of the strvec to pass to the function to retain the pointers\n>>> of its strings, so we release them all at the end.\n>>>\n>>> Signed-off-by: René Scharfe \n>>> ---\n>>> builtin/am.c | 12 +++++++++++-\n>>> 1 file changed, 11 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/builtin/am.c b/builtin/am.c\n>>> index 30c9b3a9cd..dddf1b9af0 100644\n>>> --- a/builtin/am.c\n>>> +++ b/builtin/am.c\n>>> @@ -1476,6 +1476,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n>>> \tint res, opts_left;\n>>> \tint force_apply = 0;\n>>> \tint options = 0;\n>>> +\tconst char **apply_argv;\n>>>\n>>> \tif (init_apply_state(&apply_state, the_repository, NULL))\n>>> \t\tBUG(\"init_apply_state() failed\");\n>>> @@ -1483,7 +1484,15 @@ static int run_apply(const struct am_state *state, const char *index_file)\n>>> \tstrvec_push(&apply_opts, \"apply\");\n>>> \tstrvec_pushv(&apply_opts, state->git_apply_opts.v);\n>>>\n>>> -\topts_left = apply_parse_options(apply_opts.nr, apply_opts.v,\n>>> +\t/*\n>>> +\t * Build a copy that apply_parse_options() can rearrange.\n>>> +\t * apply_opts.v keeps referencing the allocated strings for\n>>> +\t * strvec_clear() to release.\n>>> +\t */\n>>> +\tALLOC_ARRAY(apply_argv, apply_opts.nr);\n>>> +\tCOPY_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);\n>>> +\n>>> +\topts_left = apply_parse_options(apply_opts.nr, apply_argv,\n>>> \t\t\t\t\t&apply_state, &force_apply, &options,\n>>> \t\t\t\t\tNULL);\n>>>\n>>> @@ -1513,6 +1522,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n>>> \tstrvec_clear(&apply_paths);\n>>> \tstrvec_clear(&apply_opts);\n>>> \tclear_apply_state(&apply_state);\n>>> +\tfree(apply_argv);\n>>>\n>>> \tif (res)\n>>> \t\treturn res;\n>>\n>> I don't mind this going in, but it really feels like a bit of a dirty\n>> hack.\n>>\n>> We have widespread leaks all over the place due to this\n>> idiom. I.e. parse_options() and a couple of other APIs expect that they\n>> can munge the \"argv\", which is fine if it arrives via main(), but not if\n>> we're editing our own strvecs.\n>\n> Where? A quick \"git grep 'parse_options.*nr'\" turns up only this place\n> as one that passes a strvec to parse_options.\n\nSorry about the vagueness, this was from memory and I didn't have a full\nlist handy.\n\nFirst, that grep isn't going to give you what you want, since it only\ncatches cases where we're passing the \"strvec\" to the \"parse_options()\"\ndirectly.\n\nBut if you look at e.g. how cmd_stash() invokes push_stash() or\ncmd_describe() invoking cmd_name_rev() you'll see callers where we're\npotentially losing the strvec due to parse_options().\n\n\"Potentially\" because in both those cases we call that API, but in the\nlatter case we've got a missing strvec_clear(), so maybe that's all\nthat's needed (it doesn't always munge the argv).\n\n>> I think less of a hack is to teach the eventual parse_options() that\n>> when it munges it it should free() it. I did that for the revisions API\n>> in f92dbdbc6a8 (revisions API: don't leak memory on argv elements that\n>> need free()-ing, 2022-08-02).\n>>\n>> What do you think?\n>\n> Generating string lists and then parsing them is weird. When calls have\n> to cross a process boundary then we have no choice, but in-process we\n> shouldn't have to lower our request to an intermediate text format. git\n> am does it anyway because it writes its options to a file and reads them\n> back when it resumes with --continue, IIUC.\n\nI agree, but making all those use a nicer API would be a much bigger\nchange.\n\n> I hope that is and will be the only place that uses parse_options() with\n> a strvec -- and then we don't have to change that function.\n>\n> If this pattern is used more widely then we could package the copying\n> done by this patch somehow, e.g. by adding a strvec_parse_options()\n> that wraps the real thing.\n\nThe reason I'm encouraging you to look at some of the other strvec leaks\nis to see if you can think of a pattern that fits these various leaky\ncallers.\n\nSo e.g. a strvec_parse_options() won't work for the ones noted above\nwhere we've lost track of it being a \"struct strvec\" in the first place.\n\nI have a local version somewhere where I did (pseudocode):\n\n\tstruct strvec vec = STRVEC_INIT;\n\tstruct string_list to_free = STRING_LIST_INIT_DUP;\n\n // populate vec...\n for (size_t i = 0; i < vec.nr; i++)\n\t\tstring_list_append_nodup(&to_free, v[i]):\n\t// call parse_options(), or otherwise munge \"vec\"...\n\tfree(strvec_detach(&vec));\n string_list_clear(&to_free, 0);\n\nI.e. you snapshot the members of the \"vec\" before it's munged, and\nfree() those via a \"struct string_list\".\n\nThen you don't strvec_clear(), but instead free() the container, its\nmembers being free'd by the string_list.\n\nHere's a (not yet in-tree) patch that uses that:\nhttps://lore.kernel.org/git/patch-10.10-81f138e460c-20221017T115544Z-avarab@gmail.com/\n\nI think that's more reliable & general than the pattern you're adding\nhere.\n\nIn your version the only reason we're not segfaulting is because the\nparse_options() consumes all the arguments, but that's not something you\ncan generally rely on with parse_options().\n\nAlso, and maybe I'm missing something, but wouldn't this be functionally\nthe same as your implementation:\n\t\n\tdiff --git a/builtin/am.c b/builtin/am.c\n\tindex 30c9b3a9cd7..e65262cbb21 100644\n\t--- a/builtin/am.c\n\t+++ b/builtin/am.c\n\t@@ -1476,11 +1476,13 @@ static int run_apply(const struct am_state *state, const char *index_file)\n\t \tint res, opts_left;\n\t \tint force_apply = 0;\n\t \tint options = 0;\n\t+\tchar *to_free;\n\t \n\t \tif (init_apply_state(&apply_state, the_repository, NULL))\n\t \t\tBUG(\"init_apply_state() failed\");\n\t \n\t \tstrvec_push(&apply_opts, \"apply\");\n\t+\tto_free = (char *)apply_opts.v[0];\n\t \tstrvec_pushv(&apply_opts, state->git_apply_opts.v);\n\t \n\t \topts_left = apply_parse_options(apply_opts.nr, apply_opts.v,\n\t@@ -1489,6 +1491,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n\t \n\t \tif (opts_left != 0)\n\t \t\tdie(\"unknown option passed through to git apply\");\n\t+\tfree(to_free);\n\t \n\t \tif (index_file) {\n\t \t\tapply_state.index_file = index_file;\n\nI.e. we leak the strvec_push() of the \"apply\" literal, but for the rest\nwe append the \"state->git_apply_opts.v\", which we already free()\nelsewhere.\n\nSo actually, aren't we really just fumbling our way towards the \"nodup\"\ninterface that the \"struct string_list\" has, and we should add the same\nto the \"struct strvec\".\n\nThis seems to also fix all the leaks you fixed, but without any of the\ncopying:\n\n\tdiff --git a/builtin/am.c b/builtin/am.c\n\tindex 30c9b3a9cd7..691ec1d152d 100644\n\t--- a/builtin/am.c\n\t+++ b/builtin/am.c\n\t@@ -1471,7 +1471,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)\n\t static int run_apply(const struct am_state *state, const char *index_file)\n\t {\n\t \tstruct strvec apply_paths = STRVEC_INIT;\n\t-\tstruct strvec apply_opts = STRVEC_INIT;\n\t+\tstruct strvec apply_opts = STRVEC_INIT_NODUP;\n\t \tstruct apply_state apply_state;\n\t \tint res, opts_left;\n\t \tint force_apply = 0;\n\tdiff --git a/strvec.c b/strvec.c\n\tindex 61a76ce6cb9..efdc47a5854 100644\n\t--- a/strvec.c\n\t+++ b/strvec.c\n\t@@ -22,7 +22,9 @@ static void strvec_push_nodup(struct strvec *array, const char *value)\n\t \n\t const char *strvec_push(struct strvec *array, const char *value)\n\t {\n\t-\tstrvec_push_nodup(array, xstrdup(value));\n\t+\tconst char *to_push = array->nodup_strings ? value : xstrdup(value);\n\t+\n\t+\tstrvec_push_nodup(array, to_push);\n\t \treturn array->v[array->nr - 1];\n\t }\n\t \n\t@@ -31,6 +33,9 @@ const char *strvec_pushf(struct strvec *array, const char *fmt, ...)\n\t \tva_list ap;\n\t \tstruct strbuf v = STRBUF_INIT;\n\t \n\t+\tif (array->nodup_strings)\n\t+\t\tBUG(\"cannot strvec_pushf() on a 'nodup' strvec\");\n\t+\n\t \tva_start(ap, fmt);\n\t \tstrbuf_vaddf(&v, fmt, ap);\n\t \tva_end(ap);\n\t@@ -67,6 +72,9 @@ void strvec_pop(struct strvec *array)\n\t \n\t void strvec_split(struct strvec *array, const char *to_split)\n\t {\n\t+\tif (array->nodup_strings)\n\t+\t\tBUG(\"cannot strvec_split() on a 'nodup' strvec\");\n\t+\n\t \twhile (isspace(*to_split))\n\t \t\tto_split++;\n\t \tfor (;;) {\n\t@@ -90,7 +98,7 @@ void strvec_clear(struct strvec *array)\n\t \tif (array->v != empty_strvec) {\n\t \t\tint i;\n\t \t\tfor (i = 0; i < array->nr; i++)\n\t-\t\t\tfree((char *)array->v[i]);\n\t+\t\t\tfree(array->nodup_strings ? NULL : (char *)array->v[i]);\n\t \t\tfree(array->v);\n\t \t}\n\t \tstrvec_init(array);\n\tdiff --git a/strvec.h b/strvec.h\n\tindex 9f55c8766ba..ac6e2c04cea 100644\n\t--- a/strvec.h\n\t+++ b/strvec.h\n\t@@ -31,12 +31,18 @@ struct strvec {\n\t \tconst char **v;\n\t \tsize_t nr;\n\t \tsize_t alloc;\n\t+\tunsigned int nodup_strings:1;\n\t };\n\t \n\t #define STRVEC_INIT { \\\n\t \t.v = empty_strvec, \\\n\t }\n\t \n\t+#define STRVEC_INIT_NODUP { \\\n\t+\t.v = empty_strvec, \\\n\t+\t.nodup_strings = 1, \\\n\t+}\n\t+\n\t /**\n\t * Initialize an array. This is no different than assigning from\n\t * `STRVEC_INIT`.\n\nIn any case, for this change (and leak fixes in general), could you\nplease also mark the now-passing leak-free tests. This fails after your\npatch, but passes before.\n\nThe failure is a \"good\" one, as they're now leak-free, but should be\nmarked as such:\n\n\tGIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true \\\n\tmake SANITIZE=leak test T=\"t4256-am-format-flowed.sh t0023-crlf-am.sh t4254-am-corrupt.sh t4257-am-interactive.sh t5403-post-checkout-hook.sh t4152-am-subjects.sh\"\n\n> If we have to change parse_options() at all then I'd prefer it to not\n> free() anything (to keep it usable with main()'s parameters),...\n\nI'm suggesting passing a flag to it to have it free() things it\nNULL's.\n\nMaybe that's a bad idea, but I don't see the incompatibility with\nmain(), for those we just wouldn't pass the flag.\n\nIt does have the inherent limitation that you couldn't mix strvec and\nnon-strvec parameters, i.e. if some of your elements are dup'd but\nothers aren't you'd need to arrange to have them all dup'd.\n\nBut I don't think that's an issue in practice, either we pass the fully\ndup'd version, or main() parameters.\n\n> but to\n> reorder in a non-destructive way. That would mean keeping the NULL\n> sentinel where it is, and making sure all callers use only the returned\n> argc to determine which arguments parse_options() didn't recognize.\n\nI think this would work if parse_options() wasn't clobbering those\nelements in some cases, and replacing them with new ones, but doesn't it\ndo that e.g. in parse_options_step()?\n\nIt also seems like a much more fragile change to try to ensure that\nnothing uses the \"argv\" again, but only looks at the updated\n\"argc\".\n\nBoth that & adding a \"const\" to it would be a huge change across the\ncodebase, so I'd like to avoid them, but at least the \"const\" method\nwould be helped along by the compiler.\n"},{"id":"469081","messageId":"RFC-cover-0.5-00000000000-20221215T090226Z-avarab@gmail.com","threadId":"58942","inReplyTo":"221214.86ilie48cv.gmgdl@evledraar.gmail.com","subject":"[RFC PATCH 0/5] strvec: add a \"nodup\" mode, fix memory leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-15T09:11:06Z","receivedAt":"2022-12-15T09:11:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This is an alternative to René's [1], his already fixes a leak in \"git\nam\", and this could be done later, so I'm submitting it as RFC, but it\ncould also replace it.\n\nI think as this series shows extending the \"strvec\" API to get a\nfeature that works like the existing \"strdup_strings\" that the \"struct\nstring_list\" has can make memory management much simpler.\n\nThe 4/5 here shows cases where we were leaking because our \"v\" was\nclobbered, but where all the strings we were pushing to the \"strvec\"\nwere fixed strings, so we could skip xstrdup()-ing them.\n\nThe 5/5 then shows more complex cases where we have mixed-use,\ni.e. most strings are fixed, but some are not. For those we use a\n\"struct string_list to_free = STRING_LIST_INIT_DUP\", which we then\npush to the \"to_free\" list with \"string_list_append_nodup()\".\n\nThis does make the API slightly more dangerous to use, as it's no\nlonger guaranteed that it owns all the members it points to. But as\nthe \"struct string_list\" has shown this isn't an issue in practice,\nand e.g. SANITIZE=address et al are good about finding double-frees,\nor frees of fixed strings.\n\nA branch & CI for this are found at [2].\n\n1. https://lore.kernel.org/git/baf93e4a-7f05-857c-e551-09675496c03c@web.de/\n2. https://github.com/avar/git/tree/avar/add-and-use-strvec-init-nodup\n\nÆvar Arnfjörð Bjarmason (5):\n builtin/annotate.c: simplify for strvec API\n various: add missing strvec_clear()\n strvec API: add a \"STRVEC_INIT_NODUP\"\n strvec API users: fix leaks by using \"STRVEC_INIT_NODUP\"\n strvec API users: fix more leaks by using \"STRVEC_INIT_NODUP\"\n\n builtin/am.c | 2 +-\n builtin/annotate.c | 17 ++++++++---------\n builtin/describe.c | 28 +++++++++++++++++++++-------\n builtin/stash.c | 8 ++++++--\n builtin/upload-archive.c | 16 ++++++++++++----\n strvec.c | 20 ++++++++++++++++++--\n strvec.h | 30 +++++++++++++++++++++++++++++-\n t/t0023-crlf-am.sh | 1 +\n t/t4152-am-subjects.sh | 2 ++\n t/t4254-am-corrupt.sh | 2 ++\n t/t4256-am-format-flowed.sh | 1 +\n t/t4257-am-interactive.sh | 2 ++\n t/t5003-archive-zip.sh | 1 +\n t/t5403-post-checkout-hook.sh | 1 +\n 14 files changed, 105 insertions(+), 26 deletions(-)\n\n-- \n2.39.0.rc2.1048.g0e5493b8d5b\n\n"},{"id":"469082","messageId":"RFC-patch-2.5-9785934c6d7-20221215T090226Z-avarab@gmail.com","threadId":"58942","inReplyTo":"RFC-cover-0.5-00000000000-20221215T090226Z-avarab@gmail.com","subject":"[RFC PATCH 2/5] various: add missing strvec_clear()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-15T09:11:08Z","receivedAt":"2022-12-15T09:11:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix or partially fix memory leaks that happened because a\nstrvec_clear() was missing. The \"partially\" will be addressed in a\nsubsequent commit, we'll still leak in cases where the function we're\ncalling munges our \"v\" member.\n\nIn the case of \"builtin/describe.c\" let's change it to use the macro\ninitializer rather than the strvec_init() while we're at it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/annotate.c | 5 ++++-\n builtin/describe.c | 8 +++++---\n builtin/stash.c | 6 +++++-\n builtin/upload-archive.c | 7 +++++--\n 4 files changed, 19 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/annotate.c b/builtin/annotate.c\nindex e37b269196f..de58deadfc7 100644\n--- a/builtin/annotate.c\n+++ b/builtin/annotate.c\n@@ -10,9 +10,12 @@\n int cmd_annotate(int argc UNUSED, const char **argv, const char *prefix)\n {\n \tstruct strvec args = STRVEC_INIT;\n+\tint ret;\n \n \tstrvec_pushl(&args, argv[0], \"-c\", NULL);\n \tstrvec_pushv(&args, &argv[1]);\n \n-\treturn cmd_blame(args.nr, args.v, prefix);\n+\tret = cmd_blame(args.nr, args.v, prefix);\n+\tstrvec_clear(&args);\n+\treturn ret;\n }\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex eea1e330c00..cb205f6b561 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -597,9 +597,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \n \tif (contains) {\n \t\tstruct string_list_item *item;\n-\t\tstruct strvec args;\n+\t\tstruct strvec args = STRVEC_INIT;\n+\t\tint ret;\n \n-\t\tstrvec_init(&args);\n \t\tstrvec_pushl(&args, \"name-rev\",\n \t\t\t \"--peel-tag\", \"--name-only\", \"--no-undefined\",\n \t\t\t NULL);\n@@ -616,7 +616,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tstrvec_pushv(&args, argv);\n \t\telse\n \t\t\tstrvec_push(&args, \"HEAD\");\n-\t\treturn cmd_name_rev(args.nr, args.v, prefix);\n+\t\tret = cmd_name_rev(args.nr, args.v, prefix);\n+\t\tstrvec_clear(&args);\n+\t\treturn ret;\n \t}\n \n \thashmap_init(&names, commit_name_neq, NULL, 0);\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex bb0fd861434..e504e22e0b9 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -961,6 +961,7 @@ static int show_stash(int argc, const char **argv, const char *prefix)\n \tret = diff_result_code(&rev.diffopt, 0);\n cleanup:\n \tstrvec_clear(&stash_args);\n+\tstrvec_clear(&revision_args);\n \tfree_stash_info(&info);\n \trelease_revisions(&rev);\n \tif (do_usage)\n@@ -1838,6 +1839,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)\n \t\tOPT_SUBCOMMAND_F(\"save\", &fn, save_stash, PARSE_OPT_NOCOMPLETE),\n \t\tOPT_END()\n \t};\n+\tint ret;\n \n \tgit_config(git_stash_config, NULL);\n \n@@ -1861,5 +1863,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)\n \t/* Assume 'stash push' */\n \tstrvec_push(&args, \"push\");\n \tstrvec_pushv(&args, argv);\n-\treturn !!push_stash(args.nr, args.v, prefix, 1);\n+\tret = !!push_stash(args.nr, args.v, prefix, 1);\n+\tstrvec_clear(&args);\n+\treturn ret;\n }\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 945ee2b4126..6ef0d06ee8b 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -21,6 +21,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n {\n \tstruct strvec sent_argv = STRVEC_INIT;\n \tconst char *arg_cmd = \"argument \";\n+\tint ret;\n \n \tif (argc != 2 || !strcmp(argv[1], \"-h\"))\n \t\tusage(upload_archive_usage);\n@@ -45,8 +46,10 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \t}\n \n \t/* parse all options sent by the client */\n-\treturn write_archive(sent_argv.nr, sent_argv.v, prefix,\n-\t\t\t the_repository, NULL, 1);\n+\tret = write_archive(sent_argv.nr, sent_argv.v, prefix,\n+\t\t\t the_repository, NULL, 1);\n+\tstrvec_clear(&sent_argv);\n+\treturn ret;\n }\n \n __attribute__((format (printf, 1, 2)))\n-- \n2.39.0.rc2.1048.g0e5493b8d5b\n\n"},{"id":"469083","messageId":"RFC-patch-1.5-cde038825d0-20221215T090226Z-avarab@gmail.com","threadId":"58942","inReplyTo":"RFC-cover-0.5-00000000000-20221215T090226Z-avarab@gmail.com","subject":"[RFC PATCH 1/5] builtin/annotate.c: simplify for strvec API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-15T09:11:07Z","receivedAt":"2022-12-15T09:11:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When this code was migrated to \"struct strvec\" (or rather, its\npredecessor API) in 8c2cfa55446 (annotate: use argv_array, 2014-07-16)\nit didn't take full advantage of what we were given:\n\n* We are always passed the name \"annotate\" as argv[0] here, so we\n don't need to re-hardcode it. We've already declared it in \"struct\n cmd_struct commands\" in git.c.\n\n* We are guaranteed to get at least one argument, so rather than\n looping here ourselves let's have strvec_pushv() handle that. If we\n only have one argument we'll pass the terminating NULL to it, making\n it a NOOP.\n\nThis change helps to make the subsequent commit smaller, and as a\nbonus removes the type discrepancy between \"int\" and \"size_t\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/annotate.c | 10 +++-------\n 1 file changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/annotate.c b/builtin/annotate.c\nindex 58ff977a231..e37b269196f 100644\n--- a/builtin/annotate.c\n+++ b/builtin/annotate.c\n@@ -7,16 +7,12 @@\n #include \"builtin.h\"\n #include \"strvec.h\"\n \n-int cmd_annotate(int argc, const char **argv, const char *prefix)\n+int cmd_annotate(int argc UNUSED, const char **argv, const char *prefix)\n {\n \tstruct strvec args = STRVEC_INIT;\n-\tint i;\n \n-\tstrvec_pushl(&args, \"annotate\", \"-c\", NULL);\n-\n-\tfor (i = 1; i < argc; i++) {\n-\t\tstrvec_push(&args, argv[i]);\n-\t}\n+\tstrvec_pushl(&args, argv[0], \"-c\", NULL);\n+\tstrvec_pushv(&args, &argv[1]);\n \n \treturn cmd_blame(args.nr, args.v, prefix);\n }\n-- \n2.39.0.rc2.1048.g0e5493b8d5b\n\n"},{"id":"469084","messageId":"RFC-patch-3.5-16c20baf5ec-20221215T090226Z-avarab@gmail.com","threadId":"58942","inReplyTo":"RFC-cover-0.5-00000000000-20221215T090226Z-avarab@gmail.com","subject":"[RFC PATCH 3/5] strvec API: add a \"STRVEC_INIT_NODUP\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-15T09:11:09Z","receivedAt":"2022-12-15T09:11:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We have various tricky cases where we'll leak a \"struct strvec\" even\nwhen we call strvec_clear(), these happen because we'll call\nsetup_revisions(), parse_options() etc., which will munge our \"v\"\nmember.\n\nThere's various potential ways to deal with that, see the extensive\non-list discussion at [1]. One way would be to pass a flag to ask the\nunderlying API to free() these, as was done for setup_revisions() in\n[2].\n\nBut we don't need that complexity for many common cases, which are\npushing fixed strings to the \"struct strvec\". Let's instead add a flag\nanalogous to the \"strdup_strings\" flag in the \"struct string_list\". A\nsubsequent commit will make use of this API.\n\nImplementation notes: The BUG_unless_dup() is implemented as a macro\nso we'll report the correct line number on BUG(). The \"nodup_strings\"\nflag could have been named a \"strdup_strings\" for consistency with the\n\"struct string_list\" API, but to do so we'd have to be confident that\nwe've spotted all callers that assume that they can memset() a \"struct\nstrvec\" to zero.\n\n1. https://lore.kernel.org/git/221214.86ilie48cv.gmgdl@evledraar.gmail.com/\n2. f92dbdbc6a8 (revisions API: don't leak memory on argv elements that\n need free()-ing, 2022-08-02)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n strvec.c | 20 ++++++++++++++++++--\n strvec.h | 30 +++++++++++++++++++++++++++++-\n 2 files changed, 47 insertions(+), 3 deletions(-)\n\ndiff --git a/strvec.c b/strvec.c\nindex 61a76ce6cb9..721f8e94a50 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -10,6 +10,16 @@ void strvec_init(struct strvec *array)\n \tmemcpy(array, &blank, sizeof(*array));\n }\n \n+void strvec_init_nodup(struct strvec *array)\n+{\n+\tstruct strvec blank = STRVEC_INIT_NODUP;\n+\tmemcpy(array, &blank, sizeof(*array));\n+}\n+\n+#define BUG_unless_dup(array, fn) \\\n+\tif ((array)->nodup_strings) \\\n+\t\tBUG(\"cannot %s() on a 'STRVEC_INIT_NODUP' strvec\", (fn))\n+\n static void strvec_push_nodup(struct strvec *array, const char *value)\n {\n \tif (array->v == empty_strvec)\n@@ -22,7 +32,9 @@ static void strvec_push_nodup(struct strvec *array, const char *value)\n \n const char *strvec_push(struct strvec *array, const char *value)\n {\n-\tstrvec_push_nodup(array, xstrdup(value));\n+\tconst char *to_push = array->nodup_strings ? value : xstrdup(value);\n+\n+\tstrvec_push_nodup(array, to_push);\n \treturn array->v[array->nr - 1];\n }\n \n@@ -31,6 +43,8 @@ const char *strvec_pushf(struct strvec *array, const char *fmt, ...)\n \tva_list ap;\n \tstruct strbuf v = STRBUF_INIT;\n \n+\tBUG_unless_dup(array, \"strvec_pushf\");\n+\n \tva_start(ap, fmt);\n \tstrbuf_vaddf(&v, fmt, ap);\n \tva_end(ap);\n@@ -67,6 +81,8 @@ void strvec_pop(struct strvec *array)\n \n void strvec_split(struct strvec *array, const char *to_split)\n {\n+\tBUG_unless_dup(array, \"strvec_pushf\");\n+\n \twhile (isspace(*to_split))\n \t\tto_split++;\n \tfor (;;) {\n@@ -89,7 +105,7 @@ void strvec_clear(struct strvec *array)\n {\n \tif (array->v != empty_strvec) {\n \t\tint i;\n-\t\tfor (i = 0; i < array->nr; i++)\n+\t\tfor (i = 0; !array->nodup_strings && i < array->nr; i++)\n \t\t\tfree((char *)array->v[i]);\n \t\tfree(array->v);\n \t}\ndiff --git a/strvec.h b/strvec.h\nindex 9f55c8766ba..b122b87b369 100644\n--- a/strvec.h\n+++ b/strvec.h\n@@ -26,29 +26,51 @@ extern const char *empty_strvec[];\n * member contains the actual array; the `nr` member contains the\n * number of elements in the array, not including the terminating\n * NULL.\n+ *\n+ * When using `STRVEC_INIT_NODUP` to initialize it the `nodup_strings'\n+ * member is set, and individual members of the \"struct strvec\" will\n+ * not be free()'d by strvec_clear(). This is for fixed string\n+ * arguments to parse_options() and others that might munge the \"v\"\n+ * itself.\n */\n struct strvec {\n \tconst char **v;\n \tsize_t nr;\n \tsize_t alloc;\n+\tunsigned int nodup_strings:1;\n };\n \n #define STRVEC_INIT { \\\n \t.v = empty_strvec, \\\n }\n \n+#define STRVEC_INIT_NODUP { \\\n+\t.v = empty_strvec, \\\n+\t.nodup_strings = 1, \\\n+}\n+\n /**\n * Initialize an array. This is no different than assigning from\n * `STRVEC_INIT`.\n */\n void strvec_init(struct strvec *);\n \n+/**\n+ * Initialize a \"nodup\" array. This is no different than assigning from\n+ * `STRVEC_INIT_NODUP`.\n+ */\n+void strvec_init_nodup(struct strvec *);\n+\n /* Push a copy of a string onto the end of the array. */\n const char *strvec_push(struct strvec *, const char *);\n \n /**\n * Format a string and push it onto the end of the array. This is a\n * convenience wrapper combining `strbuf_addf` and `strvec_push`.\n+ *\n+ * This is incompatible with arrays initialized with\n+ * `STRVEC_INIT_NODUP`, as pushing the formatted string requires the\n+ * equivalent of an xstrfmt().\n */\n __attribute__((format (printf,2,3)))\n const char *strvec_pushf(struct strvec *, const char *fmt, ...);\n@@ -70,7 +92,13 @@ void strvec_pushv(struct strvec *, const char **);\n */\n void strvec_pop(struct strvec *);\n \n-/* Splits by whitespace; does not handle quoted arguments! */\n+/**\n+ * Splits by whitespace; does not handle quoted arguments!\n+ *\n+ * This is incompatible with arrays initialized with\n+ * `STRVEC_INIT_NODUP`, as pushing the elements requires an xstrndup()\n+ * call.\n+ */\n void strvec_split(struct strvec *, const char *);\n \n /**\n-- \n2.39.0.rc2.1048.g0e5493b8d5b\n\n"},{"id":"469085","messageId":"RFC-patch-5.5-81742a8a6ed-20221215T090226Z-avarab@gmail.com","threadId":"58942","inReplyTo":"RFC-cover-0.5-00000000000-20221215T090226Z-avarab@gmail.com","subject":"[RFC PATCH 5/5] strvec API users: fix more leaks by using \"STRVEC_INIT_NODUP\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-15T09:11:11Z","receivedAt":"2022-12-15T09:11:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"For these cases where most of the the strings we were pushing to the\n\"struct strvec\" were fixed strings, but others are dynamically\nallocated. For the latter we keep around a \"to_free\" list of them.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/describe.c | 22 +++++++++++++++++-----\n builtin/upload-archive.c | 9 +++++++--\n t/t5003-archive-zip.sh | 1 +\n 3 files changed, 25 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex cb205f6b561..eda59ebb19a 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -596,8 +596,10 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--long\", \"--abbrev=0\");\n \n \tif (contains) {\n+\t\tstruct string_list to_free = STRING_LIST_INIT_DUP;\n \t\tstruct string_list_item *item;\n-\t\tstruct strvec args = STRVEC_INIT;\n+\t\tstruct strvec args = STRVEC_INIT_NODUP;\n+\n \t\tint ret;\n \n \t\tstrvec_pushl(&args, \"name-rev\",\n@@ -607,10 +609,19 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tstrvec_push(&args, \"--always\");\n \t\tif (!all) {\n \t\t\tstrvec_push(&args, \"--tags\");\n-\t\t\tfor_each_string_list_item(item, &patterns)\n-\t\t\t\tstrvec_pushf(&args, \"--refs=refs/tags/%s\", item->string);\n-\t\t\tfor_each_string_list_item(item, &exclude_patterns)\n-\t\t\t\tstrvec_pushf(&args, \"--exclude=refs/tags/%s\", item->string);\n+\t\t\tfor_each_string_list_item(item, &patterns) {\n+\t\t\t\tchar *str = xstrfmt(\"--refs=refs/tags/%s\", item->string);\n+\t\t\t\tconst char *item = string_list_append_nodup(&to_free, str)->string;\n+\n+\t\t\t\tstrvec_push(&args, item);\n+\t\t\t}\n+\t\t\tfor_each_string_list_item(item, &exclude_patterns) {\n+\t\t\t\tchar *str = xstrfmt(\"--exclude=refs/tags/%s\", item->string);\n+\n+\t\t\t\tconst char *item = string_list_append_nodup(&to_free, str)->string;\n+\n+\t\t\t\tstrvec_push(&args, item);\n+\t\t\t}\n \t\t}\n \t\tif (argc)\n \t\t\tstrvec_pushv(&args, argv);\n@@ -618,6 +629,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tstrvec_push(&args, \"HEAD\");\n \t\tret = cmd_name_rev(args.nr, args.v, prefix);\n \t\tstrvec_clear(&args);\n+\t\tstring_list_clear(&to_free, 0);\n \t\treturn ret;\n \t}\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 6ef0d06ee8b..95c6cdc4be0 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -19,7 +19,8 @@ static const char deadchild[] =\n \n int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n {\n-\tstruct strvec sent_argv = STRVEC_INIT;\n+\tstruct string_list to_free = STRING_LIST_INIT_DUP;\n+\tstruct strvec sent_argv = STRVEC_INIT_NODUP;\n \tconst char *arg_cmd = \"argument \";\n \tint ret;\n \n@@ -34,6 +35,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \t/* put received options in sent_argv[] */\n \tstrvec_push(&sent_argv, \"git-upload-archive\");\n \tfor (;;) {\n+\t\tstruct string_list_item *item;\n \t\tchar *buf = packet_read_line(0, NULL);\n \t\tif (!buf)\n \t\t\tbreak;\t/* got a flush */\n@@ -42,13 +44,16 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \n \t\tif (!starts_with(buf, arg_cmd))\n \t\t\tdie(\"'argument' token or flush expected\");\n-\t\tstrvec_push(&sent_argv, buf + strlen(arg_cmd));\n+\n+\t\titem = string_list_append(&to_free, buf + strlen(arg_cmd));\n+\t\tstrvec_push(&sent_argv, item->string);\n \t}\n \n \t/* parse all options sent by the client */\n \tret = write_archive(sent_argv.nr, sent_argv.v, prefix,\n \t\t\t the_repository, NULL, 1);\n \tstrvec_clear(&sent_argv);\n+\tstring_list_clear(&to_free, 0);\n \treturn ret;\n }\n \ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex fc499cdff01..cc1ce060558 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -2,6 +2,7 @@\n \n test_description='git archive --format=zip test'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n TEST_CREATE_REPO_NO_TEMPLATE=1\n . ./test-lib.sh\n \n-- \n2.39.0.rc2.1048.g0e5493b8d5b\n\n"},{"id":"469086","messageId":"RFC-patch-4.5-6051c309d0d-20221215T090226Z-avarab@gmail.com","threadId":"58942","inReplyTo":"RFC-cover-0.5-00000000000-20221215T090226Z-avarab@gmail.com","subject":"[RFC PATCH 4/5] strvec API users: fix leaks by using \"STRVEC_INIT_NODUP\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-15T09:11:10Z","receivedAt":"2022-12-15T09:11:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"For these cases where all of the strings we're pushing to the \"struct\nstrvec\" are fixed strings we can fix widespread memory leaks by\nskipping the xstrdup() on strvec_push().\n\nMore in-tree users could benefit from this to save needless\nxstrdup()-ing, but for all of these we were munging the \"v\" member, so\nthe subsequent strvec_clear() wouldn't free the memory.\n\nNow there's no need to free the individual elements, but we'll still\nneed to free the container with the strvec_clear().\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/am.c | 2 +-\n builtin/annotate.c | 2 +-\n builtin/stash.c | 2 +-\n t/t0023-crlf-am.sh | 1 +\n t/t4152-am-subjects.sh | 2 ++\n t/t4254-am-corrupt.sh | 2 ++\n t/t4256-am-format-flowed.sh | 1 +\n t/t4257-am-interactive.sh | 2 ++\n t/t5403-post-checkout-hook.sh | 1 +\n 9 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 30c9b3a9cd7..691ec1d152d 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1471,7 +1471,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)\n static int run_apply(const struct am_state *state, const char *index_file)\n {\n \tstruct strvec apply_paths = STRVEC_INIT;\n-\tstruct strvec apply_opts = STRVEC_INIT;\n+\tstruct strvec apply_opts = STRVEC_INIT_NODUP;\n \tstruct apply_state apply_state;\n \tint res, opts_left;\n \tint force_apply = 0;\ndiff --git a/builtin/annotate.c b/builtin/annotate.c\nindex de58deadfc7..99d97c1a8c0 100644\n--- a/builtin/annotate.c\n+++ b/builtin/annotate.c\n@@ -9,7 +9,7 @@\n \n int cmd_annotate(int argc UNUSED, const char **argv, const char *prefix)\n {\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct strvec args = STRVEC_INIT_NODUP;\n \tint ret;\n \n \tstrvec_pushl(&args, argv[0], \"-c\", NULL);\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex e504e22e0b9..b15dd2ebb3c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1823,7 +1823,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)\n {\n \tpid_t pid = getpid();\n \tconst char *index_file;\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct strvec args = STRVEC_INIT_NODUP;\n \tparse_opt_subcommand_fn *fn = NULL;\n \tstruct option options[] = {\n \t\tOPT_SUBCOMMAND(\"apply\", &fn, apply_stash),\ndiff --git a/t/t0023-crlf-am.sh b/t/t0023-crlf-am.sh\nindex f9bbb91f64e..575805513a3 100755\n--- a/t/t0023-crlf-am.sh\n+++ b/t/t0023-crlf-am.sh\n@@ -2,6 +2,7 @@\n \n test_description='Test am with auto.crlf'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n cat >patchfile <<\\EOF\ndiff --git a/t/t4152-am-subjects.sh b/t/t4152-am-subjects.sh\nindex 4c68245acad..9f2edba1f83 100755\n--- a/t/t4152-am-subjects.sh\n+++ b/t/t4152-am-subjects.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='test subject preservation with format-patch | am'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n make_patches() {\ndiff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\nindex 54be7da1611..45f1d4f95e5 100755\n--- a/t/t4254-am-corrupt.sh\n+++ b/t/t4254-am-corrupt.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='git am with corrupt input'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n make_mbox_with_nul () {\ndiff --git a/t/t4256-am-format-flowed.sh b/t/t4256-am-format-flowed.sh\nindex 2369c4e17ad..1015273bc82 100755\n--- a/t/t4256-am-format-flowed.sh\n+++ b/t/t4256-am-format-flowed.sh\n@@ -2,6 +2,7 @@\n \n test_description='test format=flowed support of git am'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh\nindex aed8f4de3d6..f26d7fd2dbd 100755\n--- a/t/t4257-am-interactive.sh\n+++ b/t/t4257-am-interactive.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='am --interactive tests'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'set up patches to apply' '\ndiff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\nindex 978f240cdac..cfaae547398 100755\n--- a/t/t5403-post-checkout-hook.sh\n+++ b/t/t5403-post-checkout-hook.sh\n@@ -7,6 +7,7 @@ test_description='Test the post-checkout hook.'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\n-- \n2.39.0.rc2.1048.g0e5493b8d5b\n\n"},{"id":"469208","messageId":"0036020f-5fe7-4ad1-a66e-26db9d851035@web.de","threadId":"58942","inReplyTo":"RFC-patch-5.5-81742a8a6ed-20221215T090226Z-avarab@gmail.com","subject":"Re: [RFC PATCH 5/5] strvec API users: fix more leaks by using \"STRVEC_INIT_NODUP\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-17T12:45:42Z","receivedAt":"2022-12-17T12:45:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.12.22 um 10:11 schrieb Ævar Arnfjörð Bjarmason:\n> For these cases where most of the the strings we were pushing to the\n> \"struct strvec\" were fixed strings, but others are dynamically\n> allocated. For the latter we keep around a \"to_free\" list of them.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/describe.c | 22 +++++++++++++++++-----\n> builtin/upload-archive.c | 9 +++++++--\n> t/t5003-archive-zip.sh | 1 +\n> 3 files changed, 25 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index cb205f6b561..eda59ebb19a 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -596,8 +596,10 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n> \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--long\", \"--abbrev=0\");\n>\n> \tif (contains) {\n> +\t\tstruct string_list to_free = STRING_LIST_INIT_DUP;\n> \t\tstruct string_list_item *item;\n> -\t\tstruct strvec args = STRVEC_INIT;\n> +\t\tstruct strvec args = STRVEC_INIT_NODUP;\n> +\n> \t\tint ret;\n>\n> \t\tstrvec_pushl(&args, \"name-rev\",\n> @@ -607,10 +609,19 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n> \t\t\tstrvec_push(&args, \"--always\");\n> \t\tif (!all) {\n> \t\t\tstrvec_push(&args, \"--tags\");\n> -\t\t\tfor_each_string_list_item(item, &patterns)\n> -\t\t\t\tstrvec_pushf(&args, \"--refs=refs/tags/%s\", item->string);\n> -\t\t\tfor_each_string_list_item(item, &exclude_patterns)\n> -\t\t\t\tstrvec_pushf(&args, \"--exclude=refs/tags/%s\", item->string);\n> +\t\t\tfor_each_string_list_item(item, &patterns) {\n> +\t\t\t\tchar *str = xstrfmt(\"--refs=refs/tags/%s\", item->string);\n> +\t\t\t\tconst char *item = string_list_append_nodup(&to_free, str)->string;\n> +\n> +\t\t\t\tstrvec_push(&args, item);\n> +\t\t\t}\n> +\t\t\tfor_each_string_list_item(item, &exclude_patterns) {\n> +\t\t\t\tchar *str = xstrfmt(\"--exclude=refs/tags/%s\", item->string);\n> +\n> +\t\t\t\tconst char *item = string_list_append_nodup(&to_free, str)->string;\n> +\n> +\t\t\t\tstrvec_push(&args, item);\n> +\t\t\t}\n\nHaving to track allocations separately for each option is tedious, as\nwe see here.\n\n> \t\t}\n> \t\tif (argc)\n> \t\t\tstrvec_pushv(&args, argv);\n> @@ -618,6 +629,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n> \t\t\tstrvec_push(&args, \"HEAD\");\n> \t\tret = cmd_name_rev(args.nr, args.v, prefix);\n> \t\tstrvec_clear(&args);\n> +\t\tstring_list_clear(&to_free, 0);\n> \t\treturn ret;\n> \t}\n>\n> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\n> index 6ef0d06ee8b..95c6cdc4be0 100644\n> --- a/builtin/upload-archive.c\n> +++ b/builtin/upload-archive.c\n> @@ -19,7 +19,8 @@ static const char deadchild[] =\n>\n> int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n> {\n> -\tstruct strvec sent_argv = STRVEC_INIT;\n> +\tstruct string_list to_free = STRING_LIST_INIT_DUP;\n> +\tstruct strvec sent_argv = STRVEC_INIT_NODUP;\n> \tconst char *arg_cmd = \"argument \";\n> \tint ret;\n>\n> @@ -34,6 +35,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n> \t/* put received options in sent_argv[] */\n> \tstrvec_push(&sent_argv, \"git-upload-archive\");\n> \tfor (;;) {\n> +\t\tstruct string_list_item *item;\n> \t\tchar *buf = packet_read_line(0, NULL);\n> \t\tif (!buf)\n> \t\t\tbreak;\t/* got a flush */\n> @@ -42,13 +44,16 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n>\n> \t\tif (!starts_with(buf, arg_cmd))\n> \t\t\tdie(\"'argument' token or flush expected\");\n> -\t\tstrvec_push(&sent_argv, buf + strlen(arg_cmd));\n> +\n> +\t\titem = string_list_append(&to_free, buf + strlen(arg_cmd));\n> +\t\tstrvec_push(&sent_argv, item->string);\n> \t}\n>\n> \t/* parse all options sent by the client */\n> \tret = write_archive(sent_argv.nr, sent_argv.v, prefix,\n> \t\t\t the_repository, NULL, 1);\n> \tstrvec_clear(&sent_argv);\n> +\tstring_list_clear(&to_free, 0);\n> \treturn ret;\n> }\n>\n> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\n> index fc499cdff01..cc1ce060558 100755\n> --- a/t/t5003-archive-zip.sh\n> +++ b/t/t5003-archive-zip.sh\n> @@ -2,6 +2,7 @@\n>\n> test_description='git archive --format=zip test'\n>\n> +TEST_PASSES_SANITIZE_LEAK=true\n> TEST_CREATE_REPO_NO_TEMPLATE=1\n> . ./test-lib.sh\n>\n"},{"id":"469209","messageId":"d22fb47a-3f01-6b17-6abb-fdcfde4a55dc@web.de","threadId":"58942","inReplyTo":"RFC-cover-0.5-00000000000-20221215T090226Z-avarab@gmail.com","subject":"Re: [RFC PATCH 0/5] strvec: add a \"nodup\" mode, fix memory leaks","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-17T12:45:44Z","receivedAt":"2022-12-17T12:46:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.12.22 um 10:11 schrieb Ævar Arnfjörð Bjarmason:\n> This is an alternative to René's [1], his already fixes a leak in \"git\n> am\", and this could be done later, so I'm submitting it as RFC, but it\n> could also replace it.\n>\n> I think as this series shows extending the \"strvec\" API to get a\n> feature that works like the existing \"strdup_strings\" that the \"struct\n> string_list\" has can make memory management much simpler.\n>\n> The 4/5 here shows cases where we were leaking because our \"v\" was\n> clobbered, but where all the strings we were pushing to the \"strvec\"\n> were fixed strings, so we could skip xstrdup()-ing them.\n>\n> The 5/5 then shows more complex cases where we have mixed-use,\n> i.e. most strings are fixed, but some are not. For those we use a\n> \"struct string_list to_free = STRING_LIST_INIT_DUP\", which we then\n> push to the \"to_free\" list with \"string_list_append_nodup()\".\n>\n> This does make the API slightly more dangerous to use, as it's no\n> longer guaranteed that it owns all the members it points to. But as\n> the \"struct string_list\" has shown this isn't an issue in practice,\n> and e.g. SANITIZE=address et al are good about finding double-frees,\n> or frees of fixed strings.\n>\n> A branch & CI for this are found at [2].\n>\n> 1. https://lore.kernel.org/git/baf93e4a-7f05-857c-e551-09675496c03c@web.de/\n> 2. https://github.com/avar/git/tree/avar/add-and-use-strvec-init-nodup\n>\n> Ævar Arnfjörð Bjarmason (5):\n> builtin/annotate.c: simplify for strvec API\n> various: add missing strvec_clear()\n> strvec API: add a \"STRVEC_INIT_NODUP\"\n> strvec API users: fix leaks by using \"STRVEC_INIT_NODUP\"\n> strvec API users: fix more leaks by using \"STRVEC_INIT_NODUP\"\n>\n> builtin/am.c | 2 +-\n> builtin/annotate.c | 17 ++++++++---------\n> builtin/describe.c | 28 +++++++++++++++++++++-------\n> builtin/stash.c | 8 ++++++--\n> builtin/upload-archive.c | 16 ++++++++++++----\n> strvec.c | 20 ++++++++++++++++++--\n> strvec.h | 30 +++++++++++++++++++++++++++++-\n> t/t0023-crlf-am.sh | 1 +\n> t/t4152-am-subjects.sh | 2 ++\n> t/t4254-am-corrupt.sh | 2 ++\n> t/t4256-am-format-flowed.sh | 1 +\n> t/t4257-am-interactive.sh | 2 ++\n> t/t5003-archive-zip.sh | 1 +\n> t/t5403-post-checkout-hook.sh | 1 +\n> 14 files changed, 105 insertions(+), 26 deletions(-)\n>\n\nThis complicates strvec to gain functionality that we basically already\nhave with REALLOC_ARRAY. It allows for nice conversions in some cases\n(patch 4), but places that require some kind of double accounting become\nquite nasty (patch 5). I'd prefer to keep strvec simple. Avoiding\nallocations isn't necessary because the number of options to parse is\nlow -- we just need to keep track of and release them at the end.\n\nThe problem here is that parse_options() takes a string list in the form\nof an int paired with a const char ** and modifies this list in place.\nThat's fine if we don't care about the memory ownership of the list and\nits elements, e.g. because we get it from the OS (main() style) or\naccept leakage.\n\nIt's not compatible with the list being a strvec that we don't want to\nleak, though. What we need is something that translates from strvec to\nthe argc/argv convention. If we allow leaks this is trivial: Just use\nthe .v member directly. If we don't then it is still simple: Make a\nshallow copy of the .v member, release it after use. Demo patch below.\n\n builtin/am.c | 5 ++++-\n builtin/annotate.c | 9 ++++++++-\n builtin/describe.c | 8 +++++++-\n builtin/stash.c | 10 +++++++++-\n builtin/upload-archive.c | 11 +++++++++--\n strvec.c | 9 +++++++++\n strvec.h | 7 +++++++\n 7 files changed, 53 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 30c9b3a9cd..a80d40f4be 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1476,14 +1476,16 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \tint res, opts_left;\n \tint force_apply = 0;\n \tint options = 0;\n+\tconst char **apply_argv;\n\n \tif (init_apply_state(&apply_state, the_repository, NULL))\n \t\tBUG(\"init_apply_state() failed\");\n\n \tstrvec_push(&apply_opts, \"apply\");\n \tstrvec_pushv(&apply_opts, state->git_apply_opts.v);\n+\tapply_argv = strvec_clone_nodup(&apply_opts);\n\n-\topts_left = apply_parse_options(apply_opts.nr, apply_opts.v,\n+\topts_left = apply_parse_options(apply_opts.nr, apply_argv,\n \t\t\t\t\t&apply_state, &force_apply, &options,\n \t\t\t\t\tNULL);\n\n@@ -1513,6 +1515,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \tstrvec_clear(&apply_paths);\n \tstrvec_clear(&apply_opts);\n \tclear_apply_state(&apply_state);\n+\tfree(apply_argv);\n\n \tif (res)\n \t\treturn res;\ndiff --git a/builtin/annotate.c b/builtin/annotate.c\nindex 58ff977a23..7e75f0f48d 100644\n--- a/builtin/annotate.c\n+++ b/builtin/annotate.c\n@@ -10,6 +10,8 @@\n int cmd_annotate(int argc, const char **argv, const char *prefix)\n {\n \tstruct strvec args = STRVEC_INIT;\n+\tconst char **blame_argv;\n+\tint ret;\n \tint i;\n\n \tstrvec_pushl(&args, \"annotate\", \"-c\", NULL);\n@@ -17,6 +19,11 @@ int cmd_annotate(int argc, const char **argv, const char *prefix)\n \tfor (i = 1; i < argc; i++) {\n \t\tstrvec_push(&args, argv[i]);\n \t}\n+\tblame_argv = strvec_clone_nodup(&args);\n\n-\treturn cmd_blame(args.nr, args.v, prefix);\n+\tret = cmd_blame(args.nr, blame_argv, prefix);\n+\n+\tstrvec_clear(&args);\n+\tfree(blame_argv);\n+\treturn ret;\n }\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex eea1e330c0..2ef2fbc0d4 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -598,6 +598,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \tif (contains) {\n \t\tstruct string_list_item *item;\n \t\tstruct strvec args;\n+\t\tconst char **name_rev_argv;\n+\t\tint ret;\n\n \t\tstrvec_init(&args);\n \t\tstrvec_pushl(&args, \"name-rev\",\n@@ -616,7 +618,11 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tstrvec_pushv(&args, argv);\n \t\telse\n \t\t\tstrvec_push(&args, \"HEAD\");\n-\t\treturn cmd_name_rev(args.nr, args.v, prefix);\n+\t\tname_rev_argv = strvec_clone_nodup(&args);\n+\t\tret = cmd_name_rev(args.nr, name_rev_argv, prefix);\n+\t\tstrvec_clear(&args);\n+\t\tfree(name_rev_argv);\n+\t\treturn ret;\n \t}\n\n \thashmap_init(&names, commit_name_neq, NULL, 0);\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex bb0fd86143..864b7c573a 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1820,6 +1820,8 @@ static int save_stash(int argc, const char **argv, const char *prefix)\n\n int cmd_stash(int argc, const char **argv, const char *prefix)\n {\n+\tint ret;\n+\tconst char **push_stash_argv;\n \tpid_t pid = getpid();\n \tconst char *index_file;\n \tstruct strvec args = STRVEC_INIT;\n@@ -1861,5 +1863,11 @@ int cmd_stash(int argc, const char **argv, const char *prefix)\n \t/* Assume 'stash push' */\n \tstrvec_push(&args, \"push\");\n \tstrvec_pushv(&args, argv);\n-\treturn !!push_stash(args.nr, args.v, prefix, 1);\n+\tpush_stash_argv = strvec_clone_nodup(&args);\n+\n+\tret = !!push_stash(args.nr, push_stash_argv, prefix, 1);\n+\n+\tstrvec_clear(&args);\n+\tfree(push_stash_argv);\n+\treturn ret;\n }\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 945ee2b412..36ed85e10a 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -19,6 +19,8 @@ static const char deadchild[] =\n\n int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n {\n+\tint ret;\n+\tconst char **upload_archive_argv;\n \tstruct strvec sent_argv = STRVEC_INIT;\n \tconst char *arg_cmd = \"argument \";\n\n@@ -43,10 +45,15 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)\n \t\t\tdie(\"'argument' token or flush expected\");\n \t\tstrvec_push(&sent_argv, buf + strlen(arg_cmd));\n \t}\n+\tupload_archive_argv = strvec_clone_nodup(&sent_argv);\n\n \t/* parse all options sent by the client */\n-\treturn write_archive(sent_argv.nr, sent_argv.v, prefix,\n-\t\t\t the_repository, NULL, 1);\n+\tret = write_archive(sent_argv.nr, upload_archive_argv, prefix,\n+\t\t\t the_repository, NULL, 1);\n+\n+\tstrvec_clear(&sent_argv);\n+\tfree(upload_archive_argv);\n+\treturn ret;\n }\n\n __attribute__((format (printf, 1, 2)))\ndiff --git a/strvec.c b/strvec.c\nindex 61a76ce6cb..5c41c8c506 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -106,3 +106,12 @@ const char **strvec_detach(struct strvec *array)\n \t\treturn ret;\n \t}\n }\n+\n+const char **strvec_clone_nodup(const struct strvec *sv)\n+{\n+\tconst char **ret;\n+\n+\tCALLOC_ARRAY(ret, sv->nr + 1);\n+\tCOPY_ARRAY(ret, sv->v, sv->nr);\n+\treturn ret;\n+}\ndiff --git a/strvec.h b/strvec.h\nindex 9f55c8766b..6234634b00 100644\n--- a/strvec.h\n+++ b/strvec.h\n@@ -88,4 +88,11 @@ void strvec_clear(struct strvec *);\n */\n const char **strvec_detach(struct strvec *);\n\n+/**\n+ * Create a copy of the array that references the original strings.\n+ * The caller is responsible for freeing the memory of that copy,\n+ * but must not free the individual strings.\n+ */\n+const char **strvec_clone_nodup(const struct strvec *);\n+\n #endif /* STRVEC_H */\n"},{"id":"469212","messageId":"5f5e86e9-8c15-7ba3-1a03-0ec9f505c960@web.de","threadId":"58942","inReplyTo":"RFC-patch-3.5-16c20baf5ec-20221215T090226Z-avarab@gmail.com","subject":"Re: [RFC PATCH 3/5] strvec API: add a \"STRVEC_INIT_NODUP\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-17T12:45:52Z","receivedAt":"2022-12-17T12:46:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.12.22 um 10:11 schrieb Ævar Arnfjörð Bjarmason:\n> We have various tricky cases where we'll leak a \"struct strvec\" even\n> when we call strvec_clear(), these happen because we'll call\n> setup_revisions(), parse_options() etc., which will munge our \"v\"\n> member.\n>\n> There's various potential ways to deal with that, see the extensive\n> on-list discussion at [1]. One way would be to pass a flag to ask the\n> underlying API to free() these, as was done for setup_revisions() in\n> [2].\n>\n> But we don't need that complexity for many common cases, which are\n> pushing fixed strings to the \"struct strvec\". Let's instead add a flag\n> analogous to the \"strdup_strings\" flag in the \"struct string_list\". A\n> subsequent commit will make use of this API.\n>\n> Implementation notes: The BUG_unless_dup() is implemented as a macro\n> so we'll report the correct line number on BUG(). The \"nodup_strings\"\n> flag could have been named a \"strdup_strings\" for consistency with the\n> \"struct string_list\" API, but to do so we'd have to be confident that\n> we've spotted all callers that assume that they can memset() a \"struct\n> strvec\" to zero.\n\nIf the two variants would get separate types then the compiler would\ntake care of these checks. Conceptually they are different enough and\nhave different operations; they only share the same struct layout.\n\nBut I doubt a _nodup variant as its own type would pull its weight.\nIt would basically be a trivial wrapper around REALLOC_ARRAY..\n\n>\n> 1. https://lore.kernel.org/git/221214.86ilie48cv.gmgdl@evledraar.gmail.com/\n> 2. f92dbdbc6a8 (revisions API: don't leak memory on argv elements that\n> need free()-ing, 2022-08-02)\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> strvec.c | 20 ++++++++++++++++++--\n> strvec.h | 30 +++++++++++++++++++++++++++++-\n> 2 files changed, 47 insertions(+), 3 deletions(-)\n>\n> diff --git a/strvec.c b/strvec.c\n> index 61a76ce6cb9..721f8e94a50 100644\n> --- a/strvec.c\n> +++ b/strvec.c\n> @@ -10,6 +10,16 @@ void strvec_init(struct strvec *array)\n> \tmemcpy(array, &blank, sizeof(*array));\n> }\n>\n> +void strvec_init_nodup(struct strvec *array)\n> +{\n> +\tstruct strvec blank = STRVEC_INIT_NODUP;\n> +\tmemcpy(array, &blank, sizeof(*array));\n> +}\n> +\n> +#define BUG_unless_dup(array, fn) \\\n> +\tif ((array)->nodup_strings) \\\n> +\t\tBUG(\"cannot %s() on a 'STRVEC_INIT_NODUP' strvec\", (fn))\n> +\n> static void strvec_push_nodup(struct strvec *array, const char *value)\n> {\n> \tif (array->v == empty_strvec)\n> @@ -22,7 +32,9 @@ static void strvec_push_nodup(struct strvec *array, const char *value)\n>\n> const char *strvec_push(struct strvec *array, const char *value)\n> {\n> -\tstrvec_push_nodup(array, xstrdup(value));\n> +\tconst char *to_push = array->nodup_strings ? value : xstrdup(value);\n> +\n> +\tstrvec_push_nodup(array, to_push);\n> \treturn array->v[array->nr - 1];\n> }\n>\n> @@ -31,6 +43,8 @@ const char *strvec_pushf(struct strvec *array, const char *fmt, ...)\n> \tva_list ap;\n> \tstruct strbuf v = STRBUF_INIT;\n>\n> +\tBUG_unless_dup(array, \"strvec_pushf\");\n> +\n> \tva_start(ap, fmt);\n> \tstrbuf_vaddf(&v, fmt, ap);\n> \tva_end(ap);\n> @@ -67,6 +81,8 @@ void strvec_pop(struct strvec *array)\n>\n> void strvec_split(struct strvec *array, const char *to_split)\n> {\n> +\tBUG_unless_dup(array, \"strvec_pushf\");\n> +\n> \twhile (isspace(*to_split))\n> \t\tto_split++;\n> \tfor (;;) {\n> @@ -89,7 +105,7 @@ void strvec_clear(struct strvec *array)\n> {\n> \tif (array->v != empty_strvec) {\n> \t\tint i;\n> -\t\tfor (i = 0; i < array->nr; i++)\n> +\t\tfor (i = 0; !array->nodup_strings && i < array->nr; i++)\n> \t\t\tfree((char *)array->v[i]);\n> \t\tfree(array->v);\n> \t}\n> diff --git a/strvec.h b/strvec.h\n> index 9f55c8766ba..b122b87b369 100644\n> --- a/strvec.h\n> +++ b/strvec.h\n> @@ -26,29 +26,51 @@ extern const char *empty_strvec[];\n> * member contains the actual array; the `nr` member contains the\n> * number of elements in the array, not including the terminating\n> * NULL.\n> + *\n> + * When using `STRVEC_INIT_NODUP` to initialize it the `nodup_strings'\n> + * member is set, and individual members of the \"struct strvec\" will\n> + * not be free()'d by strvec_clear(). This is for fixed string\n> + * arguments to parse_options() and others that might munge the \"v\"\n> + * itself.\n> */\n> struct strvec {\n> \tconst char **v;\n> \tsize_t nr;\n> \tsize_t alloc;\n> +\tunsigned int nodup_strings:1;\n> };\n>\n> #define STRVEC_INIT { \\\n> \t.v = empty_strvec, \\\n> }\n>\n> +#define STRVEC_INIT_NODUP { \\\n> +\t.v = empty_strvec, \\\n> +\t.nodup_strings = 1, \\\n> +}\n> +\n> /**\n> * Initialize an array. This is no different than assigning from\n> * `STRVEC_INIT`.\n> */\n> void strvec_init(struct strvec *);\n>\n> +/**\n> + * Initialize a \"nodup\" array. This is no different than assigning from\n> + * `STRVEC_INIT_NODUP`.\n> + */\n> +void strvec_init_nodup(struct strvec *);\n> +\n> /* Push a copy of a string onto the end of the array. */\n> const char *strvec_push(struct strvec *, const char *);\n>\n> /**\n> * Format a string and push it onto the end of the array. This is a\n> * convenience wrapper combining `strbuf_addf` and `strvec_push`.\n> + *\n> + * This is incompatible with arrays initialized with\n> + * `STRVEC_INIT_NODUP`, as pushing the formatted string requires the\n> + * equivalent of an xstrfmt().\n> */\n> __attribute__((format (printf,2,3)))\n> const char *strvec_pushf(struct strvec *, const char *fmt, ...);\n> @@ -70,7 +92,13 @@ void strvec_pushv(struct strvec *, const char **);\n> */\n> void strvec_pop(struct strvec *);\n>\n> -/* Splits by whitespace; does not handle quoted arguments! */\n> +/**\n> + * Splits by whitespace; does not handle quoted arguments!\n> + *\n> + * This is incompatible with arrays initialized with\n> + * `STRVEC_INIT_NODUP`, as pushing the elements requires an xstrndup()\n> + * call.\n> + */\n> void strvec_split(struct strvec *, const char *);\n>\n> /**\n"},{"id":"469210","messageId":"9a1731aa-e20c-b11e-ba38-349d321a11c3@web.de","threadId":"58942","inReplyTo":"RFC-patch-4.5-6051c309d0d-20221215T090226Z-avarab@gmail.com","subject":"Re: [RFC PATCH 4/5] strvec API users: fix leaks by using \"STRVEC_INIT_NODUP\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-17T12:45:47Z","receivedAt":"2022-12-17T12:46:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.12.22 um 10:11 schrieb Ævar Arnfjörð Bjarmason:\n> For these cases where all of the strings we're pushing to the \"struct\n> strvec\" are fixed strings we can fix widespread memory leaks by\n> skipping the xstrdup() on strvec_push().\n>\n> More in-tree users could benefit from this to save needless\n> xstrdup()-ing, but for all of these we were munging the \"v\" member, so\n> the subsequent strvec_clear() wouldn't free the memory.\n>\n> Now there's no need to free the individual elements, but we'll still\n> need to free the container with the strvec_clear().\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/am.c | 2 +-\n> builtin/annotate.c | 2 +-\n> builtin/stash.c | 2 +-\n> t/t0023-crlf-am.sh | 1 +\n> t/t4152-am-subjects.sh | 2 ++\n> t/t4254-am-corrupt.sh | 2 ++\n> t/t4256-am-format-flowed.sh | 1 +\n> t/t4257-am-interactive.sh | 2 ++\n> t/t5403-post-checkout-hook.sh | 1 +\n> 9 files changed, 12 insertions(+), 3 deletions(-)\n\nLooks nice and easy, though the test updates are a bit distracting.\nThey are all enabled by the change to builtin/am.c, right?\n\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 30c9b3a9cd7..691ec1d152d 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1471,7 +1471,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)\n> static int run_apply(const struct am_state *state, const char *index_file)\n> {\n> \tstruct strvec apply_paths = STRVEC_INIT;\n> -\tstruct strvec apply_opts = STRVEC_INIT;\n> +\tstruct strvec apply_opts = STRVEC_INIT_NODUP;\n> \tstruct apply_state apply_state;\n> \tint res, opts_left;\n> \tint force_apply = 0;\n> diff --git a/builtin/annotate.c b/builtin/annotate.c\n> index de58deadfc7..99d97c1a8c0 100644\n> --- a/builtin/annotate.c\n> +++ b/builtin/annotate.c\n> @@ -9,7 +9,7 @@\n>\n> int cmd_annotate(int argc UNUSED, const char **argv, const char *prefix)\n> {\n> -\tstruct strvec args = STRVEC_INIT;\n> +\tstruct strvec args = STRVEC_INIT_NODUP;\n> \tint ret;\n>\n> \tstrvec_pushl(&args, argv[0], \"-c\", NULL);\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index e504e22e0b9..b15dd2ebb3c 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -1823,7 +1823,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)\n> {\n> \tpid_t pid = getpid();\n> \tconst char *index_file;\n> -\tstruct strvec args = STRVEC_INIT;\n> +\tstruct strvec args = STRVEC_INIT_NODUP;\n> \tparse_opt_subcommand_fn *fn = NULL;\n> \tstruct option options[] = {\n> \t\tOPT_SUBCOMMAND(\"apply\", &fn, apply_stash),\n> diff --git a/t/t0023-crlf-am.sh b/t/t0023-crlf-am.sh\n> index f9bbb91f64e..575805513a3 100755\n> --- a/t/t0023-crlf-am.sh\n> +++ b/t/t0023-crlf-am.sh\n> @@ -2,6 +2,7 @@\n>\n> test_description='Test am with auto.crlf'\n>\n> +TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n>\n> cat >patchfile <<\\EOF\n> diff --git a/t/t4152-am-subjects.sh b/t/t4152-am-subjects.sh\n> index 4c68245acad..9f2edba1f83 100755\n> --- a/t/t4152-am-subjects.sh\n> +++ b/t/t4152-am-subjects.sh\n> @@ -1,6 +1,8 @@\n> #!/bin/sh\n>\n> test_description='test subject preservation with format-patch | am'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n>\n> make_patches() {\n> diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh\n> index 54be7da1611..45f1d4f95e5 100755\n> --- a/t/t4254-am-corrupt.sh\n> +++ b/t/t4254-am-corrupt.sh\n> @@ -1,6 +1,8 @@\n> #!/bin/sh\n>\n> test_description='git am with corrupt input'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n>\n> make_mbox_with_nul () {\n> diff --git a/t/t4256-am-format-flowed.sh b/t/t4256-am-format-flowed.sh\n> index 2369c4e17ad..1015273bc82 100755\n> --- a/t/t4256-am-format-flowed.sh\n> +++ b/t/t4256-am-format-flowed.sh\n> @@ -2,6 +2,7 @@\n>\n> test_description='test format=flowed support of git am'\n>\n> +TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n>\n> test_expect_success 'setup' '\n> diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh\n> index aed8f4de3d6..f26d7fd2dbd 100755\n> --- a/t/t4257-am-interactive.sh\n> +++ b/t/t4257-am-interactive.sh\n> @@ -1,6 +1,8 @@\n> #!/bin/sh\n>\n> test_description='am --interactive tests'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n>\n> test_expect_success 'set up patches to apply' '\n> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\n> index 978f240cdac..cfaae547398 100755\n> --- a/t/t5403-post-checkout-hook.sh\n> +++ b/t/t5403-post-checkout-hook.sh\n> @@ -7,6 +7,7 @@ test_description='Test the post-checkout hook.'\n> GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>\n> +TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n>\n> test_expect_success setup '\n"},{"id":"469211","messageId":"e7f8de6e-6448-278b-a47f-cc7e5b813cf6@web.de","threadId":"58942","inReplyTo":"RFC-patch-1.5-cde038825d0-20221215T090226Z-avarab@gmail.com","subject":"Re: [RFC PATCH 1/5] builtin/annotate.c: simplify for strvec API","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-17T12:45:49Z","receivedAt":"2022-12-17T12:46:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.12.22 um 10:11 schrieb Ævar Arnfjörð Bjarmason:\n> When this code was migrated to \"struct strvec\" (or rather, its\n> predecessor API) in 8c2cfa55446 (annotate: use argv_array, 2014-07-16)\n> it didn't take full advantage of what we were given:\n>\n> * We are always passed the name \"annotate\" as argv[0] here, so we\n> don't need to re-hardcode it. We've already declared it in \"struct\n> cmd_struct commands\" in git.c.\n\nThis change has nothing to do with strvec; it could have been done by\ne68989a739 (annotate: fix for cvsserver., 2007-02-06) already. I don't\nsee much of a benefit to it because the string is already there, but if\nit's needed then I think it deserves its own patch.\n\n> * We are guaranteed to get at least one argument, so rather than\n> looping here ourselves let's have strvec_pushv() handle that. If we\n> only have one argument we'll pass the terminating NULL to it, making\n> it a NOOP.\n\n85b343245b (argv-array: implement argv_array_pushv(), 2015-06-14) added\nthe _pushv() function, so 8c2cfa55446 could not have used it.\n\n> This change helps to make the subsequent commit smaller, and as a\n> bonus removes the type discrepancy between \"int\" and \"size_t\".\n\nThis type mismatch is benign as long as we get a valid argc value,\nas positive int fit into size_t. Getting rid of the loop is nice,\nthough.\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/annotate.c | 10 +++-------\n> 1 file changed, 3 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/annotate.c b/builtin/annotate.c\n> index 58ff977a231..e37b269196f 100644\n> --- a/builtin/annotate.c\n> +++ b/builtin/annotate.c\n> @@ -7,16 +7,12 @@\n> #include \"builtin.h\"\n> #include \"strvec.h\"\n>\n> -int cmd_annotate(int argc, const char **argv, const char *prefix)\n> +int cmd_annotate(int argc UNUSED, const char **argv, const char *prefix)\n> {\n> \tstruct strvec args = STRVEC_INIT;\n> -\tint i;\n>\n> -\tstrvec_pushl(&args, \"annotate\", \"-c\", NULL);\n> -\n> -\tfor (i = 1; i < argc; i++) {\n> -\t\tstrvec_push(&args, argv[i]);\n> -\t}\n> +\tstrvec_pushl(&args, argv[0], \"-c\", NULL);\n> +\tstrvec_pushv(&args, &argv[1]);\n ^^^^^^^^\n\"argv + 1\" would be easier to read.\n\n>\n> \treturn cmd_blame(args.nr, args.v, prefix);\n> }\n"},{"id":"469213","messageId":"e7ea30dd-91a8-9cd8-8ac4-cb9af1eb1b76@web.de","threadId":"58942","inReplyTo":"221214.86ilie48cv.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] am: don't pass strvec to apply_parse_options()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-17T12:46:01Z","receivedAt":"2022-12-17T12:46:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.12.22 um 09:44 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, Dec 13 2022, René Scharfe wrote:\n>\n>> Am 13.12.22 um 09:37 schrieb Ævar Arnfjörð Bjarmason:\n>>>\n>>> On Tue, Dec 13 2022, René Scharfe wrote:\n>>>\n>>>> apply_parse_options() passes the array of argument strings to\n>>>> parse_options(), which removes recognized options. The removed strings\n>>>> are not freed, though.\n>>>>\n>>>> Make a copy of the strvec to pass to the function to retain the pointers\n>>>> of its strings, so we release them all at the end.\n>>>>\n>>>> Signed-off-by: René Scharfe \n>>>> ---\n>>>> builtin/am.c | 12 +++++++++++-\n>>>> 1 file changed, 11 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/builtin/am.c b/builtin/am.c\n>>>> index 30c9b3a9cd..dddf1b9af0 100644\n>>>> --- a/builtin/am.c\n>>>> +++ b/builtin/am.c\n>>>> @@ -1476,6 +1476,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n>>>> \tint res, opts_left;\n>>>> \tint force_apply = 0;\n>>>> \tint options = 0;\n>>>> +\tconst char **apply_argv;\n>>>>\n>>>> \tif (init_apply_state(&apply_state, the_repository, NULL))\n>>>> \t\tBUG(\"init_apply_state() failed\");\n>>>> @@ -1483,7 +1484,15 @@ static int run_apply(const struct am_state *state, const char *index_file)\n>>>> \tstrvec_push(&apply_opts, \"apply\");\n>>>> \tstrvec_pushv(&apply_opts, state->git_apply_opts.v);\n>>>>\n>>>> -\topts_left = apply_parse_options(apply_opts.nr, apply_opts.v,\n>>>> +\t/*\n>>>> +\t * Build a copy that apply_parse_options() can rearrange.\n>>>> +\t * apply_opts.v keeps referencing the allocated strings for\n>>>> +\t * strvec_clear() to release.\n>>>> +\t */\n>>>> +\tALLOC_ARRAY(apply_argv, apply_opts.nr);\n>>>> +\tCOPY_ARRAY(apply_argv, apply_opts.v, apply_opts.nr);\n>>>> +\n>>>> +\topts_left = apply_parse_options(apply_opts.nr, apply_argv,\n>>>> \t\t\t\t\t&apply_state, &force_apply, &options,\n>>>> \t\t\t\t\tNULL);\n>>>>\n>>>> @@ -1513,6 +1522,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n>>>> \tstrvec_clear(&apply_paths);\n>>>> \tstrvec_clear(&apply_opts);\n>>>> \tclear_apply_state(&apply_state);\n>>>> +\tfree(apply_argv);\n>>>>\n>>>> \tif (res)\n>>>> \t\treturn res;\n>>>\n>>> I don't mind this going in, but it really feels like a bit of a dirty\n>>> hack.\n\nWhat's so dirty about it, by the way?\n\n>>>\n>>> We have widespread leaks all over the place due to this\n>>> idiom. I.e. parse_options() and a couple of other APIs expect that they\n>>> can munge the \"argv\", which is fine if it arrives via main(), but not if\n>>> we're editing our own strvecs.\n>>\n>> Where? A quick \"git grep 'parse_options.*nr'\" turns up only this place\n>> as one that passes a strvec to parse_options.\n>\n> Sorry about the vagueness, this was from memory and I didn't have a full\n> list handy.\n>\n> First, that grep isn't going to give you what you want, since it only\n> catches cases where we're passing the \"strvec\" to the \"parse_options()\"\n> directly.\n>\n> But if you look at e.g. how cmd_stash() invokes push_stash() or\n> cmd_describe() invoking cmd_name_rev() you'll see callers where we're\n> potentially losing the strvec due to parse_options().\n>\n> \"Potentially\" because in both those cases we call that API, but in the\n> latter case we've got a missing strvec_clear(), so maybe that's all\n> that's needed (it doesn't always munge the argv).\n\nOK.\n\n>>> I think less of a hack is to teach the eventual parse_options() that\n>>> when it munges it it should free() it. I did that for the revisions API\n>>> in f92dbdbc6a8 (revisions API: don't leak memory on argv elements that\n>>> need free()-ing, 2022-08-02).\n>>>\n>>> What do you think?\n>>\n>> Generating string lists and then parsing them is weird. When calls have\n>> to cross a process boundary then we have no choice, but in-process we\n>> shouldn't have to lower our request to an intermediate text format. git\n>> am does it anyway because it writes its options to a file and reads them\n>> back when it resumes with --continue, IIUC.\n>\n> I agree, but making all those use a nicer API would be a much bigger\n> change.\n\nIndeed.\n\n>> I hope that is and will be the only place that uses parse_options() with\n>> a strvec -- and then we don't have to change that function.\n>>\n>> If this pattern is used more widely then we could package the copying\n>> done by this patch somehow, e.g. by adding a strvec_parse_options()\n>> that wraps the real thing.\n>\n> The reason I'm encouraging you to look at some of the other strvec leaks\n> is to see if you can think of a pattern that fits these various leaky\n> callers.\n>\n> So e.g. a strvec_parse_options() won't work for the ones noted above\n> where we've lost track of it being a \"struct strvec\" in the first place.\n\nRight. It would work if we used a strvec in place of *every* argc/argv\npair, but that seems a but heavy a change.\n\n> I have a local version somewhere where I did (pseudocode):\n>\n> \tstruct strvec vec = STRVEC_INIT;\n> \tstruct string_list to_free = STRING_LIST_INIT_DUP;\n>\n> // populate vec...\n> for (size_t i = 0; i < vec.nr; i++)\n> \t\tstring_list_append_nodup(&to_free, v[i]):\n> \t// call parse_options(), or otherwise munge \"vec\"...\n> \tfree(strvec_detach(&vec));\n> string_list_clear(&to_free, 0);\n>\n> I.e. you snapshot the members of the \"vec\" before it's munged, and\n> free() those via a \"struct string_list\".\n>\n> Then you don't strvec_clear(), but instead free() the container, its\n> members being free'd by the string_list.\n\nSo the same as my patch, just with a string_list instead of a string\narray and ownership juggling.\n\n> Here's a (not yet in-tree) patch that uses that:\n> https://lore.kernel.org/git/patch-10.10-81f138e460c-20221017T115544Z-avarab@gmail.com/\n>\n> I think that's more reliable & general than the pattern you're adding\n> here.\n\nHow so?\n\n> In your version the only reason we're not segfaulting is because the\n> parse_options() consumes all the arguments, but that's not something you\n> can generally rely on with parse_options().\n\nCould you please elaborate? What do you mean with \"consume\"? Or could\nyou give an example of a segfault do to partial consumption?\n\nI see one caveat: We should keep a NULL sentinel at the end when we\nhand the copy to arbitrary functions, as some of them don't even look\nat argc. Is that what you meant?\n\n> Also, and maybe I'm missing something, but wouldn't this be functionally\n> the same as your implementation:\n>\n> \tdiff --git a/builtin/am.c b/builtin/am.c\n> \tindex 30c9b3a9cd7..e65262cbb21 100644\n> \t--- a/builtin/am.c\n> \t+++ b/builtin/am.c\n> \t@@ -1476,11 +1476,13 @@ static int run_apply(const struct am_state *state, const char *index_file)\n> \t \tint res, opts_left;\n> \t \tint force_apply = 0;\n> \t \tint options = 0;\n> \t+\tchar *to_free;\n>\n> \t \tif (init_apply_state(&apply_state, the_repository, NULL))\n> \t \t\tBUG(\"init_apply_state() failed\");\n>\n> \t \tstrvec_push(&apply_opts, \"apply\");\n> \t+\tto_free = (char *)apply_opts.v[0];\n> \t \tstrvec_pushv(&apply_opts, state->git_apply_opts.v);\n>\n> \t \topts_left = apply_parse_options(apply_opts.nr, apply_opts.v,\n> \t@@ -1489,6 +1491,7 @@ static int run_apply(const struct am_state *state, const char *index_file)\n>\n> \t \tif (opts_left != 0)\n> \t \t\tdie(\"unknown option passed through to git apply\");\n> \t+\tfree(to_free);\n>\n> \t \tif (index_file) {\n> \t \t\tapply_state.index_file = index_file;\n>\n> I.e. we leak the strvec_push() of the \"apply\" literal, but for the rest\n> we append the \"state->git_apply_opts.v\", which we already free()\n> elsewhere.\n\nstate->git_apply_opts is released elsewhere, but the copies of its elements\nin apply_opts are leaked here.\n\n> So actually, aren't we really just fumbling our way towards the \"nodup\"\n> interface that the \"struct string_list\" has, and we should add the same\n> to the \"struct strvec\".\n>\n> This seems to also fix all the leaks you fixed, but without any of the\n> copying:\n>\n> \tdiff --git a/builtin/am.c b/builtin/am.c\n> \tindex 30c9b3a9cd7..691ec1d152d 100644\n> \t--- a/builtin/am.c\n> \t+++ b/builtin/am.c\n> \t@@ -1471,7 +1471,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)\n> \t static int run_apply(const struct am_state *state, const char *index_file)\n> \t {\n> \t \tstruct strvec apply_paths = STRVEC_INIT;\n> \t-\tstruct strvec apply_opts = STRVEC_INIT;\n> \t+\tstruct strvec apply_opts = STRVEC_INIT_NODUP;\n> \t \tstruct apply_state apply_state;\n> \t \tint res, opts_left;\n> \t \tint force_apply = 0;\n> \tdiff --git a/strvec.c b/strvec.c\n> \tindex 61a76ce6cb9..efdc47a5854 100644\n> \t--- a/strvec.c\n> \t+++ b/strvec.c\n> \t@@ -22,7 +22,9 @@ static void strvec_push_nodup(struct strvec *array, const char *value)\n>\n> \t const char *strvec_push(struct strvec *array, const char *value)\n> \t {\n> \t-\tstrvec_push_nodup(array, xstrdup(value));\n> \t+\tconst char *to_push = array->nodup_strings ? value : xstrdup(value);\n> \t+\n> \t+\tstrvec_push_nodup(array, to_push);\n> \t \treturn array->v[array->nr - 1];\n> \t }\n>\n> \t@@ -31,6 +33,9 @@ const char *strvec_pushf(struct strvec *array, const char *fmt, ...)\n> \t \tva_list ap;\n> \t \tstruct strbuf v = STRBUF_INIT;\n>\n> \t+\tif (array->nodup_strings)\n> \t+\t\tBUG(\"cannot strvec_pushf() on a 'nodup' strvec\");\n> \t+\n> \t \tva_start(ap, fmt);\n> \t \tstrbuf_vaddf(&v, fmt, ap);\n> \t \tva_end(ap);\n> \t@@ -67,6 +72,9 @@ void strvec_pop(struct strvec *array)\n>\n> \t void strvec_split(struct strvec *array, const char *to_split)\n> \t {\n> \t+\tif (array->nodup_strings)\n> \t+\t\tBUG(\"cannot strvec_split() on a 'nodup' strvec\");\n> \t+\n> \t \twhile (isspace(*to_split))\n> \t \t\tto_split++;\n> \t \tfor (;;) {\n> \t@@ -90,7 +98,7 @@ void strvec_clear(struct strvec *array)\n> \t \tif (array->v != empty_strvec) {\n> \t \t\tint i;\n> \t \t\tfor (i = 0; i < array->nr; i++)\n> \t-\t\t\tfree((char *)array->v[i]);\n> \t+\t\t\tfree(array->nodup_strings ? NULL : (char *)array->v[i]);\n> \t \t\tfree(array->v);\n> \t \t}\n> \t \tstrvec_init(array);\n> \tdiff --git a/strvec.h b/strvec.h\n> \tindex 9f55c8766ba..ac6e2c04cea 100644\n> \t--- a/strvec.h\n> \t+++ b/strvec.h\n> \t@@ -31,12 +31,18 @@ struct strvec {\n> \t \tconst char **v;\n> \t \tsize_t nr;\n> \t \tsize_t alloc;\n> \t+\tunsigned int nodup_strings:1;\n> \t };\n>\n> \t #define STRVEC_INIT { \\\n> \t \t.v = empty_strvec, \\\n> \t }\n>\n> \t+#define STRVEC_INIT_NODUP { \\\n> \t+\t.v = empty_strvec, \\\n> \t+\t.nodup_strings = 1, \\\n> \t+}\n> \t+\n> \t /**\n> \t * Initialize an array. This is no different than assigning from\n> \t * `STRVEC_INIT`.\n\nWe can do it, but it's quite complicated overall. strvec is with its\n\"duplicate everything\" stance is easy to understand and reason about.\nAdding a nodup variant will remove that feature.\n\n> In any case, for this change (and leak fixes in general), could you\n> please also mark the now-passing leak-free tests. This fails after your\n> patch, but passes before.\n>\n> The failure is a \"good\" one, as they're now leak-free, but should be\n> marked as such:\n>\n> \tGIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true \\\n> \tmake SANITIZE=leak test T=\"t4256-am-format-flowed.sh t0023-crlf-am.sh t4254-am-corrupt.sh t4257-am-interactive.sh t5403-post-checkout-hook.sh t4152-am-subjects.sh\"\n>\n\nWell, it might be my laziness talking, but can we avoid that? There are\nlots of leaks present in tests and plugging the last one for a particular\ntest script holds no particular significance. Wouldn't it be better to\nupdate the annotations at the end of the release cycle, similar to\ntranslations? Or automate it via some kind of CI job?\n\nIn any case: Requiring to run the test suite with LeakSanitizer for leak\nfixes would be a hurdle that I'd jump only reluctantly. (LeakSanitizer\nis not supported on my development system, an M1 Mac.)\n\n>> If we have to change parse_options() at all then I'd prefer it to not\n>> free() anything (to keep it usable with main()'s parameters),...\n>\n> I'm suggesting passing a flag to it to have it free() things it\n> NULL's.\n>\n> Maybe that's a bad idea, but I don't see the incompatibility with\n> main(), for those we just wouldn't pass the flag.\n>\n> It does have the inherent limitation that you couldn't mix strvec and\n> non-strvec parameters, i.e. if some of your elements are dup'd but\n> others aren't you'd need to arrange to have them all dup'd.\n>\n> But I don't think that's an issue in practice, either we pass the fully\n> dup'd version, or main() parameters.\n\nYes, but your examples at the top show that sometimes the parse_options()\ncall is hidden by several layers of other functions. We better have a\nmore general solution.\n\n>> but to\n>> reorder in a non-destructive way. That would mean keeping the NULL\n>> sentinel where it is, and making sure all callers use only the returned\n>> argc to determine which arguments parse_options() didn't recognize.\n>\n> I think this would work if parse_options() wasn't clobbering those\n> elements in some cases, and replacing them with new ones, but doesn't it\n> do that e.g. in parse_options_step()?\n\nYou mean the \"fake a short option thing\", right? That would have to be\naddressed somehow.\n\n> It also seems like a much more fragile change to try to ensure that\n> nothing uses the \"argv\" again, but only looks at the updated\n> \"argc\".\n>\n> Both that & adding a \"const\" to it would be a huge change across the\n> codebase, so I'd like to avoid them, but at least the \"const\" method\n> would be helped along by the compiler.\n\nAgreed.\n\nRené\n"},{"id":"469214","messageId":"Y53AXmfabIvdkfZz@coredump.intra.peff.net","threadId":"58942","inReplyTo":"RFC-cover-0.5-00000000000-20221215T090226Z-avarab@gmail.com","subject":"Re: [RFC PATCH 0/5] strvec: add a \"nodup\" mode, fix memory leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-17T13:13:02Z","receivedAt":"2022-12-17T13:13:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 15, 2022 at 10:11:06AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> This is an alternative to René's [1], his already fixes a leak in \"git\n> am\", and this could be done later, so I'm submitting it as RFC, but it\n> could also replace it.\n> \n> I think as this series shows extending the \"strvec\" API to get a\n> feature that works like the existing \"strdup_strings\" that the \"struct\n> string_list\" has can make memory management much simpler.\n\nI know this is kind of a surface level review, but...please don't do\nthis. We have chased so many bugs over the years due to string-list's\n\"maybe this is allocated and maybe not\", in both directions (accidental\nleaks and double-frees).\n\nOne of the reasons I advocated for strvec in the first place is so that\nit would have consistent memory management semantics, at the minor cost\nof sometimes duplicating them when we don't need to.\n\nAnd having a nodup form doesn't even save you from having to call\nstrvec_clear(); you still need to do so to avoid leaking the array\nitself. It only helps in the weird parse-options case, where we don't\nhandle ownership of the array very well (the strvec owns it, but\nparse-options wants to modify it).\n\n> This does make the API slightly more dangerous to use, as it's no\n> longer guaranteed that it owns all the members it points to. But as\n> the \"struct string_list\" has shown this isn't an issue in practice,\n> and e.g. SANITIZE=address et al are good about finding double-frees,\n> or frees of fixed strings.\n\nI would disagree that this hasn't been an issue in practice. A few\nrecent examples:\n\n - 5eeb9aa208 (refs: fix memory leak when parsing hideRefs config,\n 2022-11-17)\n - 7e2619d8ff (list_objects_filter_options: plug leak of filter_spec\n strings, 2022-09-08)\n - 4c81ee9669 (submodule--helper: fix \"reference\" leak, 2022-09-01)\n\nNow you could argue that those leaks might still exist if we only had a\nduplicating version of string-list (after all, the problem in a leak is\nan extra duplication). But IMHO it is the ambiguity and the games we\nplay with setting/unsetting the strdup_strings field that lead to these\nerrors.\n\nAnd yes, leak-checking and sanitizers can sometimes find these bugs. But\nthat implies triggering the bug in the test suite. And it implies extra\ntime to track and fix them. An interface which is harder to get wrong in\nthe first place is preferable.\n\n-Peff\n"},{"id":"469215","messageId":"Y53DAoJ4eK01rEji@coredump.intra.peff.net","threadId":"58942","inReplyTo":"2d0d77a4-f6ac-1fa7-bddb-9083579d8dd7@web.de","subject":"Re: [PATCH] am: don't pass strvec to apply_parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-17T13:24:18Z","receivedAt":"2022-12-17T13:24:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2022 at 07:31:13PM +0100, René Scharfe wrote:\n\n> > I think less of a hack is to teach the eventual parse_options() that\n> > when it munges it it should free() it. I did that for the revisions API\n> > in f92dbdbc6a8 (revisions API: don't leak memory on argv elements that\n> > need free()-ing, 2022-08-02).\n> >\n> > What do you think?\n> \n> Generating string lists and then parsing them is weird. When calls have\n> to cross a process boundary then we have no choice, but in-process we\n> shouldn't have to lower our request to an intermediate text format. git\n> am does it anyway because it writes its options to a file and reads them\n> back when it resumes with --continue, IIUC.\n\nThe argument has been made[1] in the past that the public API for the\nrevision machinery is not poking bits in the rev_info struct yourself,\nbut passing the appropriate options to setup_revisions().\n\nAnd I can see the point; if option \"--foo\" requires twiddling both\nrevs.foo and revs.bar, then we have no way to enforce that callers have\nremembered to do both. But if the only code which handles this is the\nparser for \"--foo\", then we're OK.\n\nIn a non-C language, we'd probably mark \"foo\" and \"bar\" as private and\nprovide a method to enable the option. We could provide a function, but\nwe'd have no compiler help to ensure that it was used over fiddling the\nbits. Possibly calling them \"foo_private\" would be sufficient to make\npeople think twice about tweaking them, though.\n\n[1] Apologies for the passive voice; I didn't want to muddle the\n paragraph by discussing who and when. But I know this is an argument\n Junio has made; I don't know if he still stands by it these days\n (there is quite a lot of field-twiddling of rev_info by now). I'm\n not sure to what degree I agree with it myself, but I thought it was\n worth mentioning here.\n\n> If we have to change parse_options() at all then I'd prefer it to not\n> free() anything (to keep it usable with main()'s parameters), but to\n> reorder in a non-destructive way. That would mean keeping the NULL\n> sentinel where it is, and making sure all callers use only the returned\n> argc to determine which arguments parse_options() didn't recognize.\n\nMy findings with -Wunused-parameter show that there are quite a lot of\nplaces that take argv/argc but assume the NULL-termination of the argv.\n\nIf we are just re-ordering argv, though, it feels like this could still\nwork with a strvec. Right now a strvec with \"nr\" items will free items 0\nthrough nr-1, assuming that v[nr] is its NULL invariant. But it could\nfree v[nr], too, in case the NULL was swapped into an earlier position.\n\nIt's a little weird already, because that swap has violated the\ninvariant, so trying to strvec_push() onto it would cause confusing\nresults. But if the general use case is to pass the strvec to\nparse_options(), get reordered, and then clear() it, it should work. If\nyou wanted to get really fancy, push() et al could double-check the\ninvariant and BUG().\n\n-Peff\n"},{"id":"469230","messageId":"91c5120d-bbd4-8aa5-a205-d1f5387a7f19@web.de","threadId":"58942","inReplyTo":"Y53DAoJ4eK01rEji@coredump.intra.peff.net","subject":"Re: [PATCH] am: don't pass strvec to apply_parse_options()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-12-17T16:07:12Z","receivedAt":"2022-12-17T16:07:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.12.22 um 14:24 schrieb Jeff King:\n> On Tue, Dec 13, 2022 at 07:31:13PM +0100, René Scharfe wrote:\n>\n>>> I think less of a hack is to teach the eventual parse_options() that\n>>> when it munges it it should free() it. I did that for the revisions API\n>>> in f92dbdbc6a8 (revisions API: don't leak memory on argv elements that\n>>> need free()-ing, 2022-08-02).\n>>>\n>>> What do you think?\n>>\n>> Generating string lists and then parsing them is weird. When calls have\n>> to cross a process boundary then we have no choice, but in-process we\n>> shouldn't have to lower our request to an intermediate text format. git\n>> am does it anyway because it writes its options to a file and reads them\n>> back when it resumes with --continue, IIUC.\n>\n> The argument has been made[1] in the past that the public API for the\n> revision machinery is not poking bits in the rev_info struct yourself,\n> but passing the appropriate options to setup_revisions().\n\nMy point was that we are stuck with the way it's done in git am\nspecifically because of its on-disk parameter store. It forces us to\nbe able to handle a parameter list that we allocate ourselves instead\nof being given one by the OS like argc/argv in main(). And this\njustifies a bespoke solution instead of changing parse_options().\n\nIn the meantime Ævar showed enough examples to convince me that a more\ngeneral solution is indeed needed, but I still think we should keep\nparse_options() unchanged and add something like strvec_clone_nodup()\ninstead.\n\nRegarding using textual interfaces internally in general: It's the\neasiest method, as we have to provide it for users anyway. It's not\nsimple, though -- generating strings and managing their ownership adds\noverhead like this patch, which we wouldn't have in an struct\ninterface.\n\n> And I can see the point; if option \"--foo\" requires twiddling both\n> revs.foo and revs.bar, then we have no way to enforce that callers have\n> remembered to do both. But if the only code which handles this is the\n> parser for \"--foo\", then we're OK.\n\nDepends on the specifics. Perhaps .bar is redundant and could be\ninferred from .foo. Or they could be replaced by an enum.\n\n> In a non-C language, we'd probably mark \"foo\" and \"bar\" as private and\n> provide a method to enable the option. We could provide a function, but\n> we'd have no compiler help to ensure that it was used over fiddling the\n> bits. Possibly calling them \"foo_private\" would be sufficient to make\n> people think twice about tweaking them, though.\n\nOr normalize the struct to avoid dependencies between fields.\n\n> [1] Apologies for the passive voice; I didn't want to muddle the\n> paragraph by discussing who and when. But I know this is an argument\n> Junio has made; I don't know if he still stands by it these days\n> (there is quite a lot of field-twiddling of rev_info by now). I'm\n> not sure to what degree I agree with it myself, but I thought it was\n> worth mentioning here.\n>\n>> If we have to change parse_options() at all then I'd prefer it to not\n>> free() anything (to keep it usable with main()'s parameters), but to\n>> reorder in a non-destructive way. That would mean keeping the NULL\n>> sentinel where it is, and making sure all callers use only the returned\n>> argc to determine which arguments parse_options() didn't recognize.\n>\n> My findings with -Wunused-parameter show that there are quite a lot of\n> places that take argv/argc but assume the NULL-termination of the argv.\n\nRight.\n\n> If we are just re-ordering argv, though, it feels like this could still\n> work with a strvec. Right now a strvec with \"nr\" items will free items 0\n> through nr-1, assuming that v[nr] is its NULL invariant. But it could\n> free v[nr], too, in case the NULL was swapped into an earlier position.\n>\n> It's a little weird already, because that swap has violated the\n> invariant, so trying to strvec_push() onto it would cause confusing\n> results. But if the general use case is to pass the strvec to\n> parse_options(), get reordered, and then clear() it, it should work. If\n> you wanted to get really fancy, push() et al could double-check the\n> invariant and BUG().\n\nYes, parse_options() and strvec are not fitting perfectly. Changing the\nformer to reorder the elements and keeping them all would require\nchecking that all callers use the return value. Feels like a lot of work.\n\nA variant that takes a strvec and removes and frees parsed items from it\nwould be clean, but would require refactoring indirect callers like\napply_parse_options() users. Busywork.\n\nMaking a shallow copy to give to parse_options() in callers that currently\npass a strvec directly or indirectly seems like the simplest solution to\nme for now.\n\nRené\n"},{"id":"469233","messageId":"Y546bgdvYIkfwTlv@coredump.intra.peff.net","threadId":"58942","inReplyTo":"91c5120d-bbd4-8aa5-a205-d1f5387a7f19@web.de","subject":"Re: [PATCH] am: don't pass strvec to apply_parse_options()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-17T21:53:50Z","receivedAt":"2022-12-17T21:53:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 17, 2022 at 05:07:12PM +0100, René Scharfe wrote:\n\n> > If we are just re-ordering argv, though, it feels like this could still\n> > work with a strvec. Right now a strvec with \"nr\" items will free items 0\n> > through nr-1, assuming that v[nr] is its NULL invariant. But it could\n> > free v[nr], too, in case the NULL was swapped into an earlier position.\n> >\n> > It's a little weird already, because that swap has violated the\n> > invariant, so trying to strvec_push() onto it would cause confusing\n> > results. But if the general use case is to pass the strvec to\n> > parse_options(), get reordered, and then clear() it, it should work. If\n> > you wanted to get really fancy, push() et al could double-check the\n> > invariant and BUG().\n> \n> Yes, parse_options() and strvec are not fitting perfectly. Changing the\n> former to reorder the elements and keeping them all would require\n> checking that all callers use the return value. Feels like a lot of work.\n\nI think we're already munging the strvec arrays in the option parser,\nthough. I'm just suggesting that parse_options() swap arguments to the\nend instead of overwriting a NULL (actually, I'm not even sure it\ndoesn't do that already), and strvec_clear() checking the final element.\nThe end state is not necessarily safe, but it's no worse than what we\nhave today.\n\nThat said...\n\n> A variant that takes a strvec and removes and frees parsed items from it\n> would be clean, but would require refactoring indirect callers like\n> apply_parse_options() users. Busywork.\n> \n> Making a shallow copy to give to parse_options() in callers that currently\n> pass a strvec directly or indirectly seems like the simplest solution to\n> me for now.\n\nYes, I thought your original patch actually got to the root of the\nproblem. strvec owns the array and its elements, and parse-options wants\nto munge the array itself (but not the elements). Making a shallow copy\nis eliminates the conflict over ownership.\n\n-Peff\n"},{"id":"469241","messageId":"xmqqili9xwj8.fsf@gitster.g","threadId":"58942","inReplyTo":"Y546bgdvYIkfwTlv@coredump.intra.peff.net","subject":"Re: [PATCH] am: don't pass strvec to apply_parse_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-18T02:42:19Z","receivedAt":"2022-12-18T02:42:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Making a shallow copy to give to parse_options() in callers that currently\n>> pass a strvec directly or indirectly seems like the simplest solution to\n>> me for now.\n>\n> Yes, I thought your original patch actually got to the root of the\n> problem. strvec owns the array and its elements, and parse-options wants\n> to munge the array itself (but not the elements). Making a shallow copy\n> is eliminates the conflict over ownership.\n\nYup, it did look very sensible to me.\n"},{"id":"469266","messageId":"221219.867cyn20zj.gmgdl@evledraar.gmail.com","threadId":"58942","inReplyTo":"Y53AXmfabIvdkfZz@coredump.intra.peff.net","subject":"Re: [RFC PATCH 0/5] strvec: add a \"nodup\" mode, fix memory leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-19T09:20:00Z","receivedAt":"2022-12-19T09:31:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Dec 17 2022, Jeff King wrote:\n\n> On Thu, Dec 15, 2022 at 10:11:06AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> This is an alternative to René's [1], his already fixes a leak in \"git\n>> am\", and this could be done later, so I'm submitting it as RFC, but it\n>> could also replace it.\n>> \n>> I think as this series shows extending the \"strvec\" API to get a\n>> feature that works like the existing \"strdup_strings\" that the \"struct\n>> string_list\" has can make memory management much simpler.\n>\n> I know this is kind of a surface level review, but...please don't do\n> this. We have chased so many bugs over the years due to string-list's\n> \"maybe this is allocated and maybe not\", in both directions (accidental\n> leaks and double-frees).\n>\n> One of the reasons I advocated for strvec in the first place is so that\n> it would have consistent memory management semantics, at the minor cost\n> of sometimes duplicating them when we don't need to.\n>\n> And having a nodup form doesn't even save you from having to call\n> strvec_clear(); you still need to do so to avoid leaking the array\n> itself. It only helps in the weird parse-options case, where we don't\n> handle ownership of the array very well (the strvec owns it, but\n> parse-options wants to modify it).\n\nYes, just like \"struct string_list\" in the \"nodup\" mode.\n\nI hear you, but I also think you're implicitly conflating two things\nhere.\n\nThere's the question of whether we should in general optimize for safety\nover more optimila memory use. I.e. if we simply have every strvec,\nstring_list etc. own its memory fully we don't need to think as much\nabout allocation or ownership.\n\nI think we should do that in general, but we also have cases where we'd\nlike to not do that, e.g. where we're adding thousands of strings to a\nstring_list, which are all borrewed from elsewhere, except for a few\nwe'd like to xstrdup().\n\nSuch API use *is* tricky, but I think formalizing it as the\n\"string_list\" does is making it better, not worse. In particular...:\n\n>> This does make the API slightly more dangerous to use, as it's no\n>> longer guaranteed that it owns all the members it points to. But as\n>> the \"struct string_list\" has shown this isn't an issue in practice,\n>> and e.g. SANITIZE=address et al are good about finding double-frees,\n>> or frees of fixed strings.\n>\n> I would disagree that this hasn't been an issue in practice. A few\n> recent examples:\n>\n> - 5eeb9aa208 (refs: fix memory leak when parsing hideRefs config,\n> 2022-11-17)\n> - 7e2619d8ff (list_objects_filter_options: plug leak of filter_spec\n> strings, 2022-09-08)\n> - 4c81ee9669 (submodule--helper: fix \"reference\" leak, 2022-09-01)\n\n...it's funny that those are the examples I would have dug up to argue\nthat this is a good idea, and to add some:\n\n\t- 4a0479086a9 (commit-graph: fix memory leak in misused\n string_list API, 2022-03-04)\n\t- b202e51b154 (grep: fix a \"path_list\" memory leak, 2021-10-22)\n\nI.e. above you note \"in both directions [...] leaks [...] and double\nfrees\", but these (and the ones I added) are all in the second category.\n\nWhich is why I don't think it's an issue in practice. The leaks have\nbeen a non-issue, and to the extent that we care the SANITIZE=leak\ntesting is closing that gap.\n\nThe dangerous issue is the double-free, but (and over the years I have\nlooked at pretty much every caller) I can't imagine a string_list\nuse-case where we:\n\n a) Actually still want to keep that memory optimization, i.e. it\n wouldn't be better by just saying \"screw it, let's dup it\".\n b) Given \"a\", we'd be better off with some bespoke custom pattern over\n the facility to do this with the \"string_list\".\n\nSo I really think we're in agreement about 99% of this, I just don't see\nhow *if* we want to do this why we're better of re-inventing this\nparticular wheel for every caller.\n"},{"id":"469339","messageId":"xmqqa63izwuv.fsf@gitster.g","threadId":"58942","inReplyTo":"91c5120d-bbd4-8aa5-a205-d1f5387a7f19@web.de","subject":"Re: [PATCH] am: don't pass strvec to apply_parse_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-20T01:29:12Z","receivedAt":"2022-12-20T01:42:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Depends on the specifics. Perhaps .bar is redundant and could be\n> inferred from .foo. Or they could be replaced by an enum.\n> ...\n> Or normalize the struct to avoid dependencies between fields.\n\nI think the example Peff brought up was mostly from the revisions\nAPI, where there are two vastly different traversal behaviour\n(i.e. limited and !limited). With .limited bit set, we first\nfully enumerate the potential commits, before showing a single\nentry in the output, and without, we can stream the output.\nIn general, we prefer !limited traversal because that would give us\nbetter interactive latency, but some options only can work with\nthe limited bit set (e.g. --ancestry-path, --cherry).\n\nWe _could_ probably have do without the .limited bit and instead\nwrote each and every check we currently make to the revs->limited\nbit as something like \n\n#define is_limited(revs) (revs->cherry_mark || \\\n\t\t\t revs->ancestry_path || \\\n\t\t\t ...)\n\nso that the \"struct members\" API users do not have to know that they\nhave to set the .limit bit when they set .cherry_mark bit. There\nare many others even in the revisions API alone.\n\nIOW, yes, this depends on the specifics, and \"normalize\" is easier\nsaid than done, unfortunately.\n"},{"id":"469870","messageId":"Y7lx1hUpZ7zOP1Lo@coredump.intra.peff.net","threadId":"58942","inReplyTo":"221219.867cyn20zj.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 0/5] strvec: add a \"nodup\" mode, fix memory leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-07T13:21:26Z","receivedAt":"2023-01-07T13:21:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 19, 2022 at 10:20:00AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> I hear you, but I also think you're implicitly conflating two things\n> here.\n> \n> There's the question of whether we should in general optimize for safety\n> over more optimila memory use. I.e. if we simply have every strvec,\n> string_list etc. own its memory fully we don't need to think as much\n> about allocation or ownership.\n> \n> I think we should do that in general, but we also have cases where we'd\n> like to not do that, e.g. where we're adding thousands of strings to a\n> string_list, which are all borrewed from elsewhere, except for a few\n> we'd like to xstrdup().\n> \n> Such API use *is* tricky, but I think formalizing it as the\n> \"string_list\" does is making it better, not worse. In particular...:\n\nI think the two things are related, though, because \"safety\" is \"people\nnot mis-using the API\". Once you give the API the option to do the\ntrickier thing, people will do it, and they will do it badly. That has\nbeen my experience with the string-list API (especially that people try\nto do clever things with transferring ownership by using a\nnon-duplicating list and then setting strdup_strings midway through the\nfunction).\n\n> > I would disagree that this hasn't been an issue in practice. A few\n> > recent examples:\n> >\n> > - 5eeb9aa208 (refs: fix memory leak when parsing hideRefs config,\n> > 2022-11-17)\n> > - 7e2619d8ff (list_objects_filter_options: plug leak of filter_spec\n> > strings, 2022-09-08)\n> > - 4c81ee9669 (submodule--helper: fix \"reference\" leak, 2022-09-01)\n> \n> ...it's funny that those are the examples I would have dug up to argue\n> that this is a good idea, and to add some:\n> \n> \t- 4a0479086a9 (commit-graph: fix memory leak in misused\n> string_list API, 2022-03-04)\n> \t- b202e51b154 (grep: fix a \"path_list\" memory leak, 2021-10-22)\n> \n> I.e. above you note \"in both directions [...] leaks [...] and double\n> frees\", but these (and the ones I added) are all in the second category.\n\nI almost didn't give a list, because it's hard to dig into all of the\ndetails. For example the second one in my list, which I worked on, did\nfix things by consistently setting strdup_strings, and it was \"just\" a\nleak fix. But it took me a full day to untangle the mess of that code,\nand there were intermediate states were we did access dangling pointers\nbefore I got to a working order of the series. And all of it was\ncomplicated by the fact that string_list is configurable, so different\nbits of the code expected different things. Even after that commit,\nthere's a horrible hack to set the strdup field and hope it's enough.\n\nIf that were the only time I'd wrestled with string_list, I'd be less\nconcerned. But (subjectively) this feels like the latest in a long line\nof bugs and irritations.\n\n-Peff\n"}]}