{"thread":{"id":"48999","subject":"[PATCH] remote: clear string_list after use in mv()","startedAt":"2018-08-01T10:19:13Z","lastAt":"2018-08-02T09:58:42Z","messageCount":3,"participants":["René Scharfe","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"354152","messageId":"553f84f2-7d9d-9a92-d5ca-39bd862ff748@web.de","threadId":"48999","inReplyTo":null,"subject":"[PATCH] remote: clear string_list after use in mv()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-08-01T10:19:07Z","receivedAt":"2018-08-01T10:19:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Switch to the _DUP variant of string_list for remote_branches to allow\nstring_list_clear() to release the allocated memory at the end, and\nactually call that function.  Free the util pointer as well; it is\nallocated in read_remote_branches().\n\nNB: This string_list is empty until read_remote_branches() is called\nvia for_each_ref(), so there is no need to clean it up when returning\nbefore that point.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nPatch generated with ---function-context for easier review -- that\nmakes it look much bigger than it actually is, though.\n\n builtin/remote.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex c74ee88690..07bd51f8eb 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -558,23 +558,23 @@ struct rename_info {\n static int read_remote_branches(const char *refname,\n \tconst struct object_id *oid, int flags, void *cb_data)\n {\n \tstruct rename_info *rename = cb_data;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct string_list_item *item;\n \tint flag;\n \tconst char *symref;\n \n \tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename->old_name);\n \tif (starts_with(refname, buf.buf)) {\n-\t\titem = string_list_append(rename->remote_branches, xstrdup(refname));\n+\t\titem = string_list_append(rename->remote_branches, refname);\n \t\tsymref = resolve_ref_unsafe(refname, RESOLVE_REF_READING,\n \t\t\t\t\t    NULL, &flag);\n \t\tif (symref && (flag & REF_ISSYMREF))\n \t\t\titem->util = xstrdup(symref);\n \t\telse\n \t\t\titem->util = NULL;\n \t}\n \tstrbuf_release(&buf);\n \n \treturn 0;\n }\n@@ -607,133 +607,134 @@ static int migrate_file(struct remote *remote)\n static int mv(int argc, const char **argv)\n {\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n \tstruct remote *oldremote, *newremote;\n \tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT,\n \t\told_remote_context = STRBUF_INIT;\n-\tstruct string_list remote_branches = STRING_LIST_INIT_NODUP;\n+\tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n \tstruct rename_info rename;\n \tint i, refspec_updated = 0;\n \n \tif (argc != 3)\n \t\tusage_with_options(builtin_remote_rename_usage, options);\n \n \trename.old_name = argv[1];\n \trename.new_name = argv[2];\n \trename.remote_branches = &remote_branches;\n \n \toldremote = remote_get(rename.old_name);\n \tif (!remote_is_configured(oldremote, 1))\n \t\tdie(_(\"No such remote: %s\"), rename.old_name);\n \n \tif (!strcmp(rename.old_name, rename.new_name) && oldremote->origin != REMOTE_CONFIG)\n \t\treturn migrate_file(oldremote);\n \n \tnewremote = remote_get(rename.new_name);\n \tif (remote_is_configured(newremote, 1))\n \t\tdie(_(\"remote %s already exists.\"), rename.new_name);\n \n \tstrbuf_addf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new_name);\n \tif (!valid_fetch_refspec(buf.buf))\n \t\tdie(_(\"'%s' is not a valid remote name\"), rename.new_name);\n \n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s\", rename.old_name);\n \tstrbuf_addf(&buf2, \"remote.%s\", rename.new_name);\n \tif (git_config_rename_section(buf.buf, buf2.buf) < 1)\n \t\treturn error(_(\"Could not rename config section '%s' to '%s'\"),\n \t\t\t\tbuf.buf, buf2.buf);\n \n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.fetch\", rename.new_name);\n \tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n \tstrbuf_addf(&old_remote_context, \":refs/remotes/%s/\", rename.old_name);\n \tfor (i = 0; i < oldremote->fetch.raw_nr; i++) {\n \t\tchar *ptr;\n \n \t\tstrbuf_reset(&buf2);\n \t\tstrbuf_addstr(&buf2, oldremote->fetch.raw[i]);\n \t\tptr = strstr(buf2.buf, old_remote_context.buf);\n \t\tif (ptr) {\n \t\t\trefspec_updated = 1;\n \t\t\tstrbuf_splice(&buf2,\n \t\t\t\t      ptr-buf2.buf + strlen(\":refs/remotes/\"),\n \t\t\t\t      strlen(rename.old_name), rename.new_name,\n \t\t\t\t      strlen(rename.new_name));\n \t\t} else\n \t\t\twarning(_(\"Not updating non-default fetch refspec\\n\"\n \t\t\t\t  \"\\t%s\\n\"\n \t\t\t\t  \"\\tPlease update the configuration manually if necessary.\"),\n \t\t\t\tbuf2.buf);\n \n \t\tgit_config_set_multivar(buf.buf, buf2.buf, \"^$\", 0);\n \t}\n \n \tread_branches();\n \tfor (i = 0; i < branch_list.nr; i++) {\n \t\tstruct string_list_item *item = branch_list.items + i;\n \t\tstruct branch_info *info = item->util;\n \t\tif (info->remote_name && !strcmp(info->remote_name, rename.old_name)) {\n \t\t\tstrbuf_reset(&buf);\n \t\t\tstrbuf_addf(&buf, \"branch.%s.remote\", item->string);\n \t\t\tgit_config_set(buf.buf, rename.new_name);\n \t\t}\n \t}\n \n \tif (!refspec_updated)\n \t\treturn 0;\n \n \t/*\n \t * First remove symrefs, then rename the rest, finally create\n \t * the new symrefs.\n \t */\n \tfor_each_ref(read_remote_branches, &rename);\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n \t\tint flag = 0;\n \t\tstruct object_id oid;\n \n \t\tread_ref_full(item->string, RESOLVE_REF_READING, &oid, &flag);\n \t\tif (!(flag & REF_ISSYMREF))\n \t\t\tcontinue;\n \t\tif (delete_ref(NULL, item->string, NULL, REF_NO_DEREF))\n \t\t\tdie(_(\"deleting '%s' failed\"), item->string);\n \t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n \n \t\tif (item->util)\n \t\t\tcontinue;\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addstr(&buf, item->string);\n \t\tstrbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n \t\t\t\trename.new_name, strlen(rename.new_name));\n \t\tstrbuf_reset(&buf2);\n \t\tstrbuf_addf(&buf2, \"remote: renamed %s to %s\",\n \t\t\t\titem->string, buf.buf);\n \t\tif (rename_ref(item->string, buf.buf, buf2.buf))\n \t\t\tdie(_(\"renaming '%s' failed\"), item->string);\n \t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n \n \t\tif (!item->util)\n \t\t\tcontinue;\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addstr(&buf, item->string);\n \t\tstrbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n \t\t\t\trename.new_name, strlen(rename.new_name));\n \t\tstrbuf_reset(&buf2);\n \t\tstrbuf_addstr(&buf2, item->util);\n \t\tstrbuf_splice(&buf2, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n \t\t\t\trename.new_name, strlen(rename.new_name));\n \t\tstrbuf_reset(&buf3);\n \t\tstrbuf_addf(&buf3, \"remote: renamed %s to %s\",\n \t\t\t\titem->string, buf.buf);\n \t\tif (create_symref(buf.buf, buf2.buf, buf3.buf))\n \t\t\tdie(_(\"creating '%s' failed\"), buf.buf);\n \t}\n+\tstring_list_clear(&remote_branches, 1);\n \treturn 0;\n }\n \n-- \n2.18.0\n"},{"id":"354211","messageId":"20180802025641.GF189024@aiede.svl.corp.google.com","threadId":"48999","inReplyTo":"553f84f2-7d9d-9a92-d5ca-39bd862ff748@web.de","subject":"Re: [PATCH] remote: clear string_list after use in mv()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-02T02:56:41Z","receivedAt":"2018-08-02T02:56:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRené Scharfe wrote:\n\n> Subject: remote: clear string_list after use in mv()\n\nThis subject line doesn't fully reflect the goodness of this change.\nHow about something like:\n\n\tremote mv: avoid leaking ref names in string_list\n\n?\n\n> Switch to the _DUP variant of string_list for remote_branches to allow\n> string_list_clear() to release the allocated memory at the end, and\n> actually call that function.  Free the util pointer as well; it is\n> allocated in read_remote_branches().\n>\n> NB: This string_list is empty until read_remote_branches() is called\n> via for_each_ref(), so there is no need to clean it up when returning\n> before that point.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n> Patch generated with ---function-context for easier review -- that\n> makes it look much bigger than it actually is, though.\n\nThanks, that indeed helps.\n\n[...]\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -558,23 +558,23 @@ struct rename_info {\n\noptional: Would it be worth a comment in the struct definition to say\nthis string_list owns its items (or in other words that it's a _DUP\nstring_list)?\n\nI think we're fine without, since it's local to the file, but may make\nsense to do if rerolling.\n\n[...]\n> @@ -607,133 +607,134 @@ static int migrate_file(struct remote *remote)\n>  static int mv(int argc, const char **argv)\n>  {\n>  \tstruct option options[] = {\n>  \t\tOPT_END()\n>  \t};\n>  \tstruct remote *oldremote, *newremote;\n>  \tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT,\n>  \t\told_remote_context = STRBUF_INIT;\n> -\tstruct string_list remote_branches = STRING_LIST_INIT_NODUP;\n> +\tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n\nVerified that these are the only two functions that touch the\nremote_branches field.  This patch didn't miss any callers.\n\n[...]\n>  \tif (!refspec_updated)\n>  \t\treturn 0;\n>  \n>  \t/*\n>  \t * First remove symrefs, then rename the rest, finally create\n>  \t * the new symrefs.\n>  \t */\n>  \tfor_each_ref(read_remote_branches, &rename);\n\nAs you noted, this is the first caller that writes to the string_list,\nso we don't have to worry about the 'return 0' above.  That said, I\nwonder if it might be simpler and more futureproof to use\ndestructor-style cleanup handling anyway:\n\n\tif (!refspec_updated)\n\t\tgoto done;\n [...]\n  done:\n\tstring_list_clear(&remote_branches, 1);\n\treturn 0;\n\n[...]\n> +\tstring_list_clear(&remote_branches, 1);\n\nnot about this patch: I wonder if we should make the free_util\nparameter a flag word so the behavior is more obvious in the caller:\n\n\tstring_list_clear(&remote_branches, STRING_LIST_FREE_UTIL);\n\nOr maybe even having a separate function:\n\n\tstring_list_clear_free_util(&remote_branches);\n\nThat's a topic for another day, though.\n\nWith whatever subset of the changes suggested above makes sense,\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for a pleasant read.\n"},{"id":"354226","messageId":"6b88257e-d698-a160-952a-738104021025@web.de","threadId":"48999","inReplyTo":"20180802025641.GF189024@aiede.svl.corp.google.com","subject":"Re: [PATCH] remote: clear string_list after use in mv()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-08-02T09:58:33Z","receivedAt":"2018-08-02T09:58:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Thank you for the review!\n\nAm 02.08.2018 um 04:56 schrieb Jonathan Nieder:\n> René Scharfe wrote:\n> \n>> Subject: remote: clear string_list after use in mv()\n> \n> This subject line doesn't fully reflect the goodness of this change.\n> How about something like:\n> \n> \tremote mv: avoid leaking ref names in string_list\n> \n> ?\n\nHmm, \"clearing\" a string_list is how we release its memory, and since we\ndidn't do it before (otherwise we wouldn't need the patch) it leaked\nbefore.  So I think the title implies plugging a leak already.\n\nThe ref names don't take up much space -- they are probably in the range\nof hundreds to thousands and probably take up less than 100 bytes each.\nThe command exits after calling mv(), so this is a minor leak which\nwon't be noticed by users.\n\nThe benefit of this patch is to bring this function in line with its\nsiblings, which all release their string_lists when done.\n\n>> --- a/builtin/remote.c\n>> +++ b/builtin/remote.c\n>> @@ -558,23 +558,23 @@ struct rename_info {\n> \n> optional: Would it be worth a comment in the struct definition to say\n> this string_list owns its items (or in other words that it's a _DUP\n> string_list)?\n\nSuch a comment could easily get out of sync with the assignment later in\nthe code.  And the struct could easily be used with both types of\nstring_lists, even if that's not the case here, so I don't think that's\nthe right place.\n\nWe could add an assert(rename->remote_branches->strdup_strings) before\nthe string_list_append() call instead, which would document that\nrequirement in the right place in a way that shouldn't get out of sync\nsilently, but I'm not sure it's worth it here.\n\n>>   \tif (!refspec_updated)\n>>   \t\treturn 0;\n>>   \n>>   \t/*\n>>   \t * First remove symrefs, then rename the rest, finally create\n>>   \t * the new symrefs.\n>>   \t */\n>>   \tfor_each_ref(read_remote_branches, &rename);\n> \n> As you noted, this is the first caller that writes to the string_list,\n> so we don't have to worry about the 'return 0' above.  That said, I\n> wonder if it might be simpler and more futureproof to use\n> destructor-style cleanup handling anyway:\n> \n> \tif (!refspec_updated)\n> \t\tgoto done;\n>   [...]\n>    done:\n> \tstring_list_clear(&remote_branches, 1);\n> \treturn 0;\n\nThere are some more early returns higher up, which would have to be\nadjusted as well.  Such a restructuring would be helpful if we decide\nto release the various strbufs in that function as well..\n\nBut perhaps the main problem is that the function is too long. Could\nit be split up into sensible parts with a lower number of allocations\neach that can be kept track of more easily?\n\n(Sounds like a bigger bite than I can chew at the moment, though.)\n\n>> +\tstring_list_clear(&remote_branches, 1);\n> \n> not about this patch: I wonder if we should make the free_util\n> parameter a flag word so the behavior is more obvious in the caller:\n> \n> \tstring_list_clear(&remote_branches, STRING_LIST_FREE_UTIL);\n> \n> Or maybe even having a separate function:\n> \n> \tstring_list_clear_free_util(&remote_branches);\n\nI agree that naming variants instead of using binary options is a good\nidea in general, as it makes the code self-documenting.\n\nI'd like to suggest another option: remove the second parameter of\nstring_list_clear() and add string_list_free_util(), which only free(3)s\n->util pointers.  Users that attached heap objects to string_list items\nwould have to call both functions; no need to glue them together.\n\nRené\n"}]}