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

From: Bert Wesarg <bert.wesarg@googlemail.com>
Date: Dec 1, 2009, 06:49 UTC
Message-ID: <36ca99e90911302249r5f77f031o73cc7bb13dc375cf@mail.gmail.com>
In-Reply-To: <7viqcrbl22.fsf@alter.siamese.dyndns.org>

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.

Show 6 quoted lines

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

Show 30 quoted lines

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