Re: [PATCH 1/1] Don't free remote->name after fetch
Right. The string_list ends up getting (potentially) populated with a mix of dup'd and borrowed values. I figured it was safer to leak here (especially as we're on the way out anyway), than free memory that shouldn't be freed.
Actually, what motivates this (and I apologize that I didn't say this earlier) is that we added (in our repo) a bit of stats collection code that executes after the string_list_clear(), and calls remote_get() which goes all sideways when some of its memory has been freed.
As an alternative, I could xstrdup each instance where remote->name is appended, which would make the string_list a homogenous dup'd list, which we could then free. If you prefer that I'll do a re-roll in that style (it just seemed to me at the time like it would be doing some useless allocations). I don't much mind either way.
--
- Keith
On Mon, Jun 13, 2016 at 6:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 96 quoted lines
>
> kmcguigan@twopensource.com writes:
>
> > From: Keith McGuigan <kmcguigan@twopensource.com>
> >
> > The string_list gets populated with the names from the remotes[] array,
> > which are not dup'd and the list does not own.
> >
> > Signed-of-by: Keith McGuigan <kmcguigan@twopensource.com>
> > ---
>
> For names that come from remote_get()->name, e.g.
>
> static int get_one_remote_for_fetch(struct remote *remote, void *priv)
> {
> struct string_list *list = priv;
> if (!remote->skip_default_update)
> string_list_append(list, remote->name);
> return 0;
> }
>
> you are correct that this is borrowed memory we are not allowed to
> free. But is borrowing from remote->name the only way this list is
> populated? For example, what happens in add_remote_or_group(),
> which does this?
>
> struct remote_group_data {
> const char *name;
> struct string_list *list;
> };
>
> static int get_remote_group(const char *key, const char *value, void *priv)
> {
> struct remote_group_data *g = priv;
>
> if (skip_prefix(key, "remotes.", &key) && !strcmp(key, g->name)) {
> /* split list by white space */
> while (*value) {
> size_t wordlen = strcspn(value, " \t\n");
>
> if (wordlen >= 1)
> string_list_append(g->list,
> xstrndup(value, wordlen));
>
> This newly allocated piece of memory is held by g->list, which was...
>
> value += wordlen + (value[wordlen] != '\0');
> }
> }
>
> return 0;
> }
>
> static int add_remote_or_group(const char *name, struct string_list *list)
> {
> int prev_nr = list->nr;
> struct remote_group_data g;
> g.name = name; g.list = list;
>
> ... passed as a callback parameter from here.
>
> git_config(get_remote_group, &g);
> if (list->nr == prev_nr) {
> struct remote *remote = remote_get(name);
> if (!remote_is_configured(remote))
> return 0;
> string_list_append(list, remote->name);
>
> This makes remote->name borrowed, which we cannot free() as you
> point out.
>
> }
> return 1;
> }
>
> So, while I agree that many should not be freed, this change makes
> the code leak some at the same time.
>
>
>
> > builtin/fetch.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index 630ae6a1bb78..181da5a2e7a3 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> > argv_array_clear(&options);
> > }
> >
> > - /* All names were strdup()ed or strndup()ed */
> > - list.strdup_strings = 1;
> > string_list_clear(&list, 0);
> >
> > close_all_packs();On Mon, Jun 13, 2016 at 8:11 PM, Keith McGuigan <kmcguigan@twopensource.com> wrote:
Show 129 quoted lines
> Right. The string_list ends up getting (potentially) populated with a mix
> of dup'd
> and borrowed values. I figured it was safer to leak here (especially as
> we're on
> the way out anyway), than free memory that shouldn't be freed.
>
> Actually, what motivates this, and I apologize that I didn't say this
> earlier, is that
> we added (in our repo) a bit of stats collection code that executes after
> the
> string_list_clear(), and calls remote_get() which goes all sideways when
> some of
> its memory has been freed.
>
> As an alternative, I could xstrdup each instance where remote->name is
> appended,
> which would make the string_list a homogenous dup'd list, which we could
> then free.
> If you prefer that I'll do a re-roll in that style (it just seemed to me at
> the time like
> it would be doing some useless allocations). I don't much mind either way.
>
> --
> - Keith
>
> On Mon, Jun 13, 2016 at 6:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> kmcguigan@twopensource.com writes:
>>
>> > From: Keith McGuigan <kmcguigan@twopensource.com>
>> >
>> > The string_list gets populated with the names from the remotes[] array,
>> > which are not dup'd and the list does not own.
>> >
>> > Signed-of-by: Keith McGuigan <kmcguigan@twopensource.com>
>> > ---
>>
>> For names that come from remote_get()->name, e.g.
>>
>> static int get_one_remote_for_fetch(struct remote *remote, void *priv)
>> {
>> struct string_list *list = priv;
>> if (!remote->skip_default_update)
>> string_list_append(list, remote->name);
>> return 0;
>> }
>>
>> you are correct that this is borrowed memory we are not allowed to
>> free. But is borrowing from remote->name the only way this list is
>> populated? For example, what happens in add_remote_or_group(),
>> which does this?
>>
>> struct remote_group_data {
>> const char *name;
>> struct string_list *list;
>> };
>>
>> static int get_remote_group(const char *key, const char *value, void
>> *priv)
>> {
>> struct remote_group_data *g = priv;
>>
>> if (skip_prefix(key, "remotes.", &key) && !strcmp(key,
>> g->name)) {
>> /* split list by white space */
>> while (*value) {
>> size_t wordlen = strcspn(value, " \t\n");
>>
>> if (wordlen >= 1)
>> string_list_append(g->list,
>> xstrndup(value,
>> wordlen));
>>
>> This newly allocated piece of memory is held by g->list, which was...
>>
>> value += wordlen + (value[wordlen] != '\0');
>> }
>> }
>>
>> return 0;
>> }
>>
>> static int add_remote_or_group(const char *name, struct string_list
>> *list)
>> {
>> int prev_nr = list->nr;
>> struct remote_group_data g;
>> g.name = name; g.list = list;
>>
>> ... passed as a callback parameter from here.
>>
>> git_config(get_remote_group, &g);
>> if (list->nr == prev_nr) {
>> struct remote *remote = remote_get(name);
>> if (!remote_is_configured(remote))
>> return 0;
>> string_list_append(list, remote->name);
>>
>> This makes remote->name borrowed, which we cannot free() as you
>> point out.
>>
>> }
>> return 1;
>> }
>>
>> So, while I agree that many should not be freed, this change makes
>> the code leak some at the same time.
>>
>>
>>
>> > builtin/fetch.c | 2 --
>> > 1 file changed, 2 deletions(-)
>> >
>> > diff --git a/builtin/fetch.c b/builtin/fetch.c
>> > index 630ae6a1bb78..181da5a2e7a3 100644
>> > --- a/builtin/fetch.c
>> > +++ b/builtin/fetch.c
>> > @@ -1347,8 +1347,6 @@ int cmd_fetch(int argc, const char **argv, const
>> > char *prefix)
>> > argv_array_clear(&options);
>> > }
>> >
>> > - /* All names were strdup()ed or strndup()ed */
>> > - list.strdup_strings = 1;
>> > string_list_clear(&list, 0);
>> >
>> > close_all_packs();
>
>
