{"thread":{"id":"42592","subject":"[PATCH 1/1] Don't free remote->name after fetch","startedAt":"2016-06-16T06:55:51Z","lastAt":"2016-06-16T06:55:52Z","messageCount":5,"participants":["kmcguigan@twopensource.com","Junio C Hamano","Keith McGuigan","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"289159","messageId":"1465841837-31604-1-git-send-email-kmcguigan@twopensource.com","threadId":"42592","inReplyTo":null,"subject":"[PATCH 1/1] Don't free remote->name after fetch","fromName":"","fromEmail":"kmcguigan@twopensource.com","sentAt":"2016-06-13T18:17:17Z","receivedAt":"2016-06-16T06:55:51Z","isPatch":true,"sender":{"key":"kmcguigan@twopensource.com","avatar":null},"body":"From: Keith McGuigan \n\nThe string_list gets populated with the names from the remotes[] array,\nwhich are not dup'd and the list does not own.\n\nSigned-of-by: Keith McGuigan \n---\n builtin/fetch.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 630ae6a1bb78..181da5a2e7a3 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\targv_array_clear(&options);\n \t}\n \n-\t/* All names were strdup()ed or strndup()ed */\n-\tlist.strdup_strings = 1;\n \tstring_list_clear(&list, 0);\n \n \tclose_all_packs();\n-- \n2.8.0.rc3.95.gc8e4e3a\n"},{"id":"289176","messageId":"xmqqbn34buak.fsf@gitster.mtv.corp.google.com","threadId":"42592","inReplyTo":"1465841837-31604-1-git-send-email-kmcguigan@twopensource.com","subject":"Re: [PATCH 1/1] Don't free remote->name after fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-13T22:25:39Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"kmcguigan@twopensource.com writes:\n\n> From: Keith McGuigan \n>\n> The string_list gets populated with the names from the remotes[] array,\n> which are not dup'd and the list does not own.\n>\n> Signed-of-by: Keith McGuigan \n> ---\n\nFor names that come from remote_get()->name, e.g.\n\n static int get_one_remote_for_fetch(struct remote *remote, void *priv)\n {\n struct string_list *list = priv;\n if (!remote->skip_default_update)\n string_list_append(list, remote->name);\n return 0;\n }\n\nyou are correct that this is borrowed memory we are not allowed to\nfree. But is borrowing from remote->name the only way this list is\npopulated? For example, what happens in add_remote_or_group(),\nwhich does this?\n\n struct remote_group_data {\n const char *name;\n struct string_list *list;\n };\n\n static int get_remote_group(const char *key, const char *value, void *priv)\n {\n struct remote_group_data *g = priv;\n\n if (skip_prefix(key, \"remotes.\", &key) && !strcmp(key, g->name)) {\n /* split list by white space */\n while (*value) {\n size_t wordlen = strcspn(value, \" \\t\\n\");\n\n if (wordlen >= 1)\n string_list_append(g->list,\n xstrndup(value, wordlen));\n\nThis newly allocated piece of memory is held by g->list, which was...\n\n value += wordlen + (value[wordlen] != '\\0');\n }\n }\n\n return 0;\n }\n\n static int add_remote_or_group(const char *name, struct string_list *list)\n {\n int prev_nr = list->nr;\n struct remote_group_data g;\n g.name = name; g.list = list;\n\n... passed as a callback parameter from here.\n\n git_config(get_remote_group, &g);\n if (list->nr == prev_nr) {\n struct remote *remote = remote_get(name);\n if (!remote_is_configured(remote))\n return 0;\n string_list_append(list, remote->name);\n\nThis makes remote->name borrowed, which we cannot free() as you\npoint out.\n\n }\n return 1;\n }\n\nSo, while I agree that many should not be freed, this change makes\nthe code leak some at the same time.\n\n\n\n> builtin/fetch.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 630ae6a1bb78..181da5a2e7a3 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\targv_array_clear(&options);\n> \t}\n> \n> -\t/* All names were strdup()ed or strndup()ed */\n> -\tlist.strdup_strings = 1;\n> \tstring_list_clear(&list, 0);\n> \n> \tclose_all_packs();\n"},{"id":"289181","messageId":"CALnYDJNS9QAtu37a76Q6N3C=GRbfgU8Xq3g7F1q7vX+b=rwOOA@mail.gmail.com","threadId":"42592","inReplyTo":"CALnYDJO=_hfcQf+=+XuHQwmH4XewqHo4qggzB0rM79WVt+e6nQ@mail.gmail.com","subject":"Re: [PATCH 1/1] Don't free remote->name after fetch","fromName":"Keith McGuigan","fromEmail":"kmcguigan@twopensource.com","sentAt":"2016-06-14T00:14:43Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":true,"sender":{"key":"kmcguigan@twopensource.com","avatar":null},"body":"Right. The string_list ends up getting (potentially) populated with a\nmix of dup'd\nand borrowed values. I figured it was safer to leak here (especially\nas we're on\nthe way out anyway), than free memory that shouldn't be freed.\n\nActually, what motivates this (and I apologize that I didn't say this\nearlier) is that\nwe added (in our repo) a bit of stats collection code that executes after the\nstring_list_clear(), and calls remote_get() which goes all sideways when some of\nits memory has been freed.\n\nAs an alternative, I could xstrdup each instance where remote->name is appended,\nwhich would make the string_list a homogenous dup'd list, which we\ncould then free.\nIf you prefer that I'll do a re-roll in that style (it just seemed to\nme at the time like\nit would be doing some useless allocations). I don't much mind either way.\n\n--\n- Keith\n\nOn Mon, Jun 13, 2016 at 6:25 PM, Junio C Hamano wrote:\n>\n> kmcguigan@twopensource.com writes:\n>\n> > From: Keith McGuigan \n> >\n> > The string_list gets populated with the names from the remotes[] array,\n> > which are not dup'd and the list does not own.\n> >\n> > Signed-of-by: Keith McGuigan \n> > ---\n>\n> For names that come from remote_get()->name, e.g.\n>\n> static int get_one_remote_for_fetch(struct remote *remote, void *priv)\n> {\n> struct string_list *list = priv;\n> if (!remote->skip_default_update)\n> string_list_append(list, remote->name);\n> return 0;\n> }\n>\n> you are correct that this is borrowed memory we are not allowed to\n> free. But is borrowing from remote->name the only way this list is\n> populated? For example, what happens in add_remote_or_group(),\n> which does this?\n>\n> struct remote_group_data {\n> const char *name;\n> struct string_list *list;\n> };\n>\n> static int get_remote_group(const char *key, const char *value, void *priv)\n> {\n> struct remote_group_data *g = priv;\n>\n> if (skip_prefix(key, \"remotes.\", &key) && !strcmp(key, g->name)) {\n> /* split list by white space */\n> while (*value) {\n> size_t wordlen = strcspn(value, \" \\t\\n\");\n>\n> if (wordlen >= 1)\n> string_list_append(g->list,\n> xstrndup(value, wordlen));\n>\n> This newly allocated piece of memory is held by g->list, which was...\n>\n> value += wordlen + (value[wordlen] != '\\0');\n> }\n> }\n>\n> return 0;\n> }\n>\n> static int add_remote_or_group(const char *name, struct string_list *list)\n> {\n> int prev_nr = list->nr;\n> struct remote_group_data g;\n> g.name = name; g.list = list;\n>\n> ... passed as a callback parameter from here.\n>\n> git_config(get_remote_group, &g);\n> if (list->nr == prev_nr) {\n> struct remote *remote = remote_get(name);\n> if (!remote_is_configured(remote))\n> return 0;\n> string_list_append(list, remote->name);\n>\n> This makes remote->name borrowed, which we cannot free() as you\n> point out.\n>\n> }\n> return 1;\n> }\n>\n> So, while I agree that many should not be freed, this change makes\n> the code leak some at the same time.\n>\n>\n>\n> > builtin/fetch.c | 2 --\n> > 1 file changed, 2 deletions(-)\n> >\n> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n> > index 630ae6a1bb78..181da5a2e7a3 100644\n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> > argv_array_clear(&options);\n> > }\n> >\n> > - /* All names were strdup()ed or strndup()ed */\n> > - list.strdup_strings = 1;\n> > string_list_clear(&list, 0);\n> >\n> > close_all_packs();\n\nOn Mon, Jun 13, 2016 at 8:11 PM, Keith McGuigan\n wrote:\n> Right. The string_list ends up getting (potentially) populated with a mix\n> of dup'd\n> and borrowed values. I figured it was safer to leak here (especially as\n> we're on\n> the way out anyway), than free memory that shouldn't be freed.\n>\n> Actually, what motivates this, and I apologize that I didn't say this\n> earlier, is that\n> we added (in our repo) a bit of stats collection code that executes after\n> the\n> string_list_clear(), and calls remote_get() which goes all sideways when\n> some of\n> its memory has been freed.\n>\n> As an alternative, I could xstrdup each instance where remote->name is\n> appended,\n> which would make the string_list a homogenous dup'd list, which we could\n> then free.\n> If you prefer that I'll do a re-roll in that style (it just seemed to me at\n> the time like\n> it would be doing some useless allocations). I don't much mind either way.\n>\n> --\n> - Keith\n>\n> On Mon, Jun 13, 2016 at 6:25 PM, Junio C Hamano wrote:\n>>\n>> kmcguigan@twopensource.com writes:\n>>\n>> > From: Keith McGuigan \n>> >\n>> > The string_list gets populated with the names from the remotes[] array,\n>> > which are not dup'd and the list does not own.\n>> >\n>> > Signed-of-by: Keith McGuigan \n>> > ---\n>>\n>> For names that come from remote_get()->name, e.g.\n>>\n>> static int get_one_remote_for_fetch(struct remote *remote, void *priv)\n>> {\n>> struct string_list *list = priv;\n>> if (!remote->skip_default_update)\n>> string_list_append(list, remote->name);\n>> return 0;\n>> }\n>>\n>> you are correct that this is borrowed memory we are not allowed to\n>> free. But is borrowing from remote->name the only way this list is\n>> populated? For example, what happens in add_remote_or_group(),\n>> which does this?\n>>\n>> struct remote_group_data {\n>> const char *name;\n>> struct string_list *list;\n>> };\n>>\n>> static int get_remote_group(const char *key, const char *value, void\n>> *priv)\n>> {\n>> struct remote_group_data *g = priv;\n>>\n>> if (skip_prefix(key, \"remotes.\", &key) && !strcmp(key,\n>> g->name)) {\n>> /* split list by white space */\n>> while (*value) {\n>> size_t wordlen = strcspn(value, \" \\t\\n\");\n>>\n>> if (wordlen >= 1)\n>> string_list_append(g->list,\n>> xstrndup(value,\n>> wordlen));\n>>\n>> This newly allocated piece of memory is held by g->list, which was...\n>>\n>> value += wordlen + (value[wordlen] != '\\0');\n>> }\n>> }\n>>\n>> return 0;\n>> }\n>>\n>> static int add_remote_or_group(const char *name, struct string_list\n>> *list)\n>> {\n>> int prev_nr = list->nr;\n>> struct remote_group_data g;\n>> g.name = name; g.list = list;\n>>\n>> ... passed as a callback parameter from here.\n>>\n>> git_config(get_remote_group, &g);\n>> if (list->nr == prev_nr) {\n>> struct remote *remote = remote_get(name);\n>> if (!remote_is_configured(remote))\n>> return 0;\n>> string_list_append(list, remote->name);\n>>\n>> This makes remote->name borrowed, which we cannot free() as you\n>> point out.\n>>\n>> }\n>> return 1;\n>> }\n>>\n>> So, while I agree that many should not be freed, this change makes\n>> the code leak some at the same time.\n>>\n>>\n>>\n>> > builtin/fetch.c | 2 --\n>> > 1 file changed, 2 deletions(-)\n>> >\n>> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> > index 630ae6a1bb78..181da5a2e7a3 100644\n>> > --- a/builtin/fetch.c\n>> > +++ b/builtin/fetch.c\n>> > @@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const\n>> > char *prefix)\n>> > argv_array_clear(&options);\n>> > }\n>> >\n>> > - /* All names were strdup()ed or strndup()ed */\n>> > - list.strdup_strings = 1;\n>> > string_list_clear(&list, 0);\n>> >\n>> > close_all_packs();\n>\n>\n"},{"id":"289191","messageId":"20160614075252.GA12563@sigill.intra.peff.net","threadId":"42592","inReplyTo":"CALnYDJNS9QAtu37a76Q6N3C=GRbfgU8Xq3g7F1q7vX+b=rwOOA@mail.gmail.com","subject":"Re: [PATCH 1/1] Don't free remote->name after fetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-14T07:52:53Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 13, 2016 at 08:14:43PM -0400, Keith McGuigan wrote:\n\n> Right. The string_list ends up getting (potentially) populated with a\n> mix of dup'd and borrowed values. I figured it was safer to leak here\n> (especially as we're on the way out anyway), than free memory that\n> shouldn't be freed.\n> \n> Actually, what motivates this (and I apologize that I didn't say this\n> earlier) is that we added (in our repo) a bit of stats collection code\n> that executes after the string_list_clear(), and calls remote_get()\n> which goes all sideways when some of its memory has been freed.\n\nYeah, I think nobody noticed because we don't have any actual code that\nruns after this string_list_clear(), but that doesn't make it not-buggy.\n\n> As an alternative, I could xstrdup each instance where remote->name is\n> appended, which would make the string_list a homogenous dup'd list,\n> which we could then free. If you prefer that I'll do a re-roll in\n> that style (it just seemed to me at the time like it would be doing\n> some useless allocations). I don't much mind either way.\n\nThat sounds much better. Fixing any case like this is really a two-part\nthing:\n\n 1. Making the memory ownership policy of the string_list consistent\n (either all allocated, or all not). And if you have _some_ items\n which must be newly allocated (i.e., there is no other place to own\n them), then the only consistent solution is to allocate all of\n them.\n\n 2. Matching the strdup_strings field to that policy.\n\n The main() function should not have to play tricks with setting\n list->strdup_strings after the fact. It should set it up front,\n and the functions it calls should use string_list_append as\n appropriate.\n\n-Peff\n"},{"id":"289234","messageId":"xmqqh9cvaca3.fsf@gitster.mtv.corp.google.com","threadId":"42592","inReplyTo":"CALnYDJNS9QAtu37a76Q6N3C=GRbfgU8Xq3g7F1q7vX+b=rwOOA@mail.gmail.com","subject":"Re: [PATCH 1/1] Don't free remote->name after fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-14T17:52:20Z","receivedAt":"2016-06-16T06:55:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Keith McGuigan writes:\n\n> As an alternative, I could xstrdup each instance where remote->name is appended,\n> which would make the string_list a homogenous dup'd list, which we\n> could then free.\n\nYeah, I think that is the right way to fix it, even though I agree\nwith you that a small leak you introduced is probably better than\nunwanted freeing we currently have.\n"}]}