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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> wrote:
>
> kmcguigan@xxxxxxxxxxxxxxxx writes:
>
> > From: Keith McGuigan <kmcguigan@xxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxx>
> > ---
>
> 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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxx> wrote:
>>
>> kmcguigan@xxxxxxxxxxxxxxxx writes:
>>
>> > From: Keith McGuigan <kmcguigan@xxxxxxxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxxxxxxxxx>
>> > ---
>>
>> 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();
>
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]