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

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

[Bert Wesarg]

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
---
 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

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

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

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

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

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

[Bert Wesarg]

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
>

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

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> On Tue, Dec 1, 2009 at 01:21, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> 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.

That is _exactly_ why I want the description in the commit log message.  I
don't want commit messages (or lack thereof) to force people to look at
the code outside the patch.

Otherwise I'll have to ask _you_ to personally give the 7-line explanation
you just gave us to anybody who runs "git log -p" with the default context
size after this patch is applied.  I do not think you have the bandwidth
to handle that ;-).

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

I too thought something like that may make sense, but it doesn't look like
so, for two reasons:

 - string-list API is a bit cumbersome to use if you allocate the string
   yourself.  You will be made responsible for freeing them, and often you
   do so by setting strdup_strings to true immediately before calling
   string_list_clear(), which is kind of ugly;

 - The ref abbrev_branch() is called and the address of whose substring is
   taken to be used as "name" in handle_one_branch() is refspec.src, but
   what goes to .util is refname that is refspec.dst---they are different
   strings and one is not a substring of the other.

IOW, your patch text is good; it just would have been better with a bit
more explanation.

Thanks, will queue in 'maint'.

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

[Johannes Schindelin]

Hi,

On Tue, 1 Dec 2009, Bert Wesarg wrote:

> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>

Thanks.  I trust you ran the test suite with valgrind just to make sure?

Ciao,
Dscho

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

[Bert Wesarg]

On Tue, Dec 1, 2009 at 09:35, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Tue, 1 Dec 2009, Bert Wesarg wrote:
>
>> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>
> Thanks.  I trust you ran the test suite with valgrind just to make sure?
Not the test suite. But my use case where I found the problem (Ie.
cut-off branch names) which was 'git remote show <remote>'.
There are still invalid reads of size 4. I think the problem is the
flex array member of 'struct ref' and strlen(). If its worth I can
look into this.

Bert
>
> Ciao,
> Dscho
>

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

[Bert Wesarg]

On Tue, Dec 1, 2009 at 08:34, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>> On Tue, Dec 1, 2009 at 01:21, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>> 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.
>
> That is _exactly_ why I want the description in the commit log message.  I
> don't want commit messages (or lack thereof) to force people to look at
> the code outside the patch.
>
> Otherwise I'll have to ask _you_ to personally give the 7-line explanation
> you just gave us to anybody who runs "git log -p" with the default context
> size after this patch is applied.  I do not think you have the bandwidth
> to handle that ;-).
Yes. That makes perfectly sense. Sorry for the hassle.

>
>> 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.
>
> I too thought something like that may make sense, but it doesn't look like
> so, for two reasons:
>
>  - string-list API is a bit cumbersome to use if you allocate the string
>   yourself.  You will be made responsible for freeing them, and often you
>   do so by setting strdup_strings to true immediately before calling
>   string_list_clear(), which is kind of ugly;
>
>  - The ref abbrev_branch() is called and the address of whose substring is
>   taken to be used as "name" in handle_one_branch() is refspec.src, but
>   what goes to .util is refname that is refspec.dst---they are different
>   strings and one is not a substring of the other.
I don't see you point here. The current code is:

	for (ref = stale_refs; ref; ref = ref->next) {
		struct string_list_item *item =
			string_list_append(abbrev_branch(ref->name), &states->stale);
		item->util = xstrdup(ref->name);
	}
So 0 == strcmp(item->string, abbrev_ref(item->util, "refs/heads/"))
should be true, shouldn't it?

Bert

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

[Bert Wesarg]

On Tue, Dec 1, 2009 at 10:05, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> On Tue, Dec 1, 2009 at 09:35, Johannes Schindelin
>> Thanks.  I trust you ran the test suite with valgrind just to make sure?
> Not the test suite. But my use case where I found the problem (Ie.
> cut-off branch names) which was 'git remote show <remote>'.
> There are still invalid reads of size 4. I think the problem is the
> flex array member of 'struct ref' and strlen(). If its worth I can
> look into this.
I need this new suppression to run the test suite:

diff --git i/t/valgrind/default.supp w/t/valgrind/default.supp
index 9e013fa..39b080a 100644
--- i/t/valgrind/default.supp
+++ w/t/valgrind/default.supp
@@ -43,3 +43,10 @@
        fun:write_buffer
        fun:write_loose_object
 }
+
+{
+       writing-data-from-zlib-triggers-even-more-errors-2
+       Memcheck:Param
+       write(buf)
+       obj:*libpthread-*.so
+}

Bert

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

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

>>  - The ref abbrev_branch() is called and the address of whose substring is
>>   taken to be used as "name" in handle_one_branch() is refspec.src, but
>>   what goes to .util is refname that is refspec.dst---they are different
>>   strings and one is not a substring of the other.
> I don't see you point here.

Of course you don't ;-) because we were looking at different versions.

I wanted to apply the same fix to both maint and master.  For the code in
'master' your observation is 100% correct.

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

[Bert Wesarg]

On Tue, Dec 1, 2009 at 18:20, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>>>  - The ref abbrev_branch() is called and the address of whose substring is
>>>   taken to be used as "name" in handle_one_branch() is refspec.src, but
>>>   what goes to .util is refname that is refspec.dst---they are different
>>>   strings and one is not a substring of the other.
>> I don't see you point here.
>
> Of course you don't ;-) because we were looking at different versions.
>
> I wanted to apply the same fix to both maint and master.  For the code in
> 'master' your observation is 100% correct.
A quick test with my use case does not show errors in the maint
branch. So it should not be needed (except the memory leak fix of the
.util member). And valgrind confirms this.

Bert

>

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

[Bert Wesarg]

On Tue, Dec 1, 2009 at 10:05, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> There are still invalid reads of size 4. I think the problem is the
> flex array member of 'struct ref' and strlen(). If its worth I can
> look into this.
A short heads-up, here is the valgrind error I get for this invalid read:

==27305== Invalid read of size 4
==27305==    at 0x4936AF: copy_ref (remote.c:870)
==27305==    by 0x4942E4: get_fetch_map (remote.c:1271)
==27305==    by 0x44473E: get_remote_ref_states (builtin-remote.c:271)
==27305==    by 0x446DCE: cmd_remote (builtin-remote.c:1022)
==27305==    by 0x4045F0: handle_internal_command (git.c:257)
==27305==    by 0x404B8F: main (git.c:482)
==27305==  Address 0x5b5ba38 is 104 bytes inside a block of size 107 alloc'd
==27305==    at 0x4C24477: calloc (vg_replace_malloc.c:418)
==27305==    by 0x4B09AD: xcalloc (wrapper.c:75)
==27305==    by 0x493924: alloc_ref_with_prefix (remote.c:853)
==27305==    by 0x46653B: get_remote_heads (connect.c:96)
==27305==    by 0x4A9347: get_refs_via_connect (transport.c:453)
==27305==    by 0x4A7F14: transport_get_remote_refs (transport.c:895)
==27305==    by 0x4445B6: get_remote_ref_states (builtin-remote.c:810)
==27305==    by 0x446DCE: cmd_remote (builtin-remote.c:1022)
==27305==    by 0x4045F0: handle_internal_command (git.c:257)
==27305==    by 0x404B8F: main (git.c:482)

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

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> A quick test with my use case does not show errors in the maint
> branch. So it should not be needed (except the memory leak fix of the
> .util member). And valgrind confirms this.

Thanks.