[PATCH 1/1] Don't free remote->name after fetch

[PATCH 1/1] Don't free remote->name after fetch

[kmcguigan]

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>
---
 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();
-- 
2.8.0.rc3.95.gc8e4e3a

Re: [PATCH 1/1] Don't free remote->name after fetch

[Junio C Hamano]

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();

Re: [PATCH 1/1] Don't free remote->name after fetch

[Keith McGuigan]

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();

On Mon, Jun 13, 2016 at 8:11 PM, Keith McGuigan
<kmcguigan@twopensource.com> wrote:
> 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();
>
>

Re: [PATCH 1/1] Don't free remote->name after fetch

[Jeff King]

On Mon, Jun 13, 2016 at 08:14:43PM -0400, Keith McGuigan wrote:

> 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.

Yeah, I think nobody noticed because we don't have any actual code that
runs after this string_list_clear(), but that doesn't make it not-buggy.

> 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.

That sounds much better. Fixing any case like this is really a two-part
thing:

  1. Making the memory ownership policy of the string_list consistent
     (either all allocated, or all not). And if you have _some_ items
     which must be newly allocated (i.e., there is no other place to own
     them), then the only consistent solution is to allocate all of
     them.

  2. Matching the strdup_strings field to that policy.

     The main() function should not have to play tricks with setting
     list->strdup_strings after the fact. It should set it up front,
     and the functions it calls should use string_list_append as
     appropriate.

-Peff

Re: [PATCH 1/1] Don't free remote->name after fetch

[Junio C Hamano]

Keith McGuigan <kmcguigan@twopensource.com> writes:

> 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.

Yeah, I think that is the right way to fix it, even though I agree
with you that a small leak you introduced is probably better than
unwanted freeing we currently have.