Re: [PATCH] get_ref_states: strdup entries and free util in stale list

[Bert Wesarg <bert.wesarg@googlemail.com>]

On Tue, Dec 1, 2009 at 01:21, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
The entries of the stale branches string list are made of the stale
refs, which are immediately free'ed after the string list creation.
Therefore the list entries use memory after free. This resulted for me
in a corrupted branch list for 'git remote show' (duplicate entries
and cut-off entries). The .util member of the string list entries are
also strdup() of the branch (full)name itself, but at clear time we
request not to free the .util member. Which results in a memory leak.

>> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>
> Hmm, the Subject: matches what the code does, but nobody mentions why it
> is necessary (iow, what breaks in the current code and what becomes better
> if the patch is applied).  The blank space before your "S-o-b:" line is
> for you to describe these things.
Sure. unfortunately the code where the string list is filled is not in
the patch. But if you look at the code it should be self-explanatory.

>
>> ---

There is actually also an other solution: we could first strdup the
ref name to the .util member and take this as the input for the
abbrev_ref()/string list entry and safe there the strdup.

Bert
>>  builtin-remote.c |    6 ++++--
>>  1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin-remote.c b/builtin-remote.c
>> index 7916626..bb72e27 100644
>> --- a/builtin-remote.c
>> +++ b/builtin-remote.c
>> @@ -272,7 +272,9 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
>>                       die("Could not get fetch map for refspec %s",
>>                               states->remote->fetch_refspec[i]);
>>
>> -     states->new.strdup_strings = states->tracked.strdup_strings = 1;
>> +     states->new.strdup_strings =
>> +     states->tracked.strdup_strings =
>> +     states->stale.strdup_strings = 1;
>>       for (ref = fetch_map; ref; ref = ref->next) {
>>               unsigned char sha1[20];
>>               if (!ref->peer_ref || read_ref(ref->peer_ref->name, sha1))
>> @@ -768,7 +770,7 @@ static void clear_push_info(void *util, const char *string)
>>  static void free_remote_ref_states(struct ref_states *states)
>>  {
>>       string_list_clear(&states->new, 0);
>> -     string_list_clear(&states->stale, 0);
>> +     string_list_clear(&states->stale, 1);
>>       string_list_clear(&states->tracked, 0);
>>       string_list_clear(&states->heads, 0);
>>       string_list_clear_func(&states->push, clear_push_info);
>> --
>> 1.6.6.rc0.253.g1ec3
>