Git in next is broken

Git in next is broken

[Nicolas Pitre]

Simply issuing a "git fetch" in my copy of git.git makes glibc complain 
with this:

*** glibc detected *** git: corrupted double-linked list: 0x0000000000974180 ***

The gdb backtrace is:

(gdb) bt
#0  0x0000003c76632f05 in raise () from /lib64/libc.so.6
#1  0x0000003c76634a73 in abort () from /lib64/libc.so.6
#2  0x0000003c76672438 in __libc_message () from /lib64/libc.so.6
#3  0x0000003c76677ec8 in malloc_printerr () from /lib64/libc.so.6
#4  0x0000003c7667a23e in _int_free () from /lib64/libc.so.6
#5  0x0000003c7667a486 in free () from /lib64/libc.so.6
#6  0x0000000000493f3f in ref_remove_duplicates (ref_map=0x7562b0)
    at remote.c:756
#7  0x0000000000424afc in get_ref_map () at builtin-fetch.c:165
#8  do_fetch () at builtin-fetch.c:644
#9  cmd_fetch (argc=<value optimized out>, argv=0x7fffffffe6a0,
    prefix=<value optimized out>) at builtin-fetch.c:754
#10 0x0000000000403d83 in run_builtin () at git.c:251
#11 handle_internal_command (argc=1, argv=0x7fffffffe6a0) at git.c:396
#12 0x0000000000403f2d in run_argv () at git.c:438
#13 main (argc=1, argv=0x7fffffffe6a0) at git.c:509

Bisection reveals the following culprit:

commit 73cf0822b2a4ffa7ad559d1f0772e39718fc7776
Author: Julian Phillips <julian@quantumfyre.co.uk>
Date:   Sun Oct 25 21:28:11 2009 +0000

    remote: Make ref_remove_duplicates faster for large numbers of refs

    The ref_remove_duplicates function was very slow at dealing with very
    large numbers of refs.  This is because it was using a linear search
    through all remaining refs to find any duplicates of the current ref.

    Rewriting it to use a string list to keep track of which refs have
    already been seen and removing duplicates when they are found is much
    more efficient.

    Signed-off-by: Julian Phillips <julian@quantumfyre.co.uk>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Reverting that commit from next does indeed fix the problem.
Note that this problem doesn't show up with all repositoryes.


Nicolas

Re: Git in next is broken

[René Scharfe]

Nicolas Pitre schrieb:
> Simply issuing a "git fetch" in my copy of git.git makes glibc complain 
> with this:
> 
> *** glibc detected *** git: corrupted double-linked list: 0x0000000000974180 ***
> 
> The gdb backtrace is:
> 
> (gdb) bt
> #0  0x0000003c76632f05 in raise () from /lib64/libc.so.6
> #1  0x0000003c76634a73 in abort () from /lib64/libc.so.6
> #2  0x0000003c76672438 in __libc_message () from /lib64/libc.so.6
> #3  0x0000003c76677ec8 in malloc_printerr () from /lib64/libc.so.6
> #4  0x0000003c7667a23e in _int_free () from /lib64/libc.so.6
> #5  0x0000003c7667a486 in free () from /lib64/libc.so.6
> #6  0x0000000000493f3f in ref_remove_duplicates (ref_map=0x7562b0)
>     at remote.c:756
> #7  0x0000000000424afc in get_ref_map () at builtin-fetch.c:165
> #8  do_fetch () at builtin-fetch.c:644
> #9  cmd_fetch (argc=<value optimized out>, argv=0x7fffffffe6a0,
>     prefix=<value optimized out>) at builtin-fetch.c:754
> #10 0x0000000000403d83 in run_builtin () at git.c:251
> #11 handle_internal_command (argc=1, argv=0x7fffffffe6a0) at git.c:396
> #12 0x0000000000403f2d in run_argv () at git.c:438
> #13 main (argc=1, argv=0x7fffffffe6a0) at git.c:509
> 
> Bisection reveals the following culprit:
> 
> commit 73cf0822b2a4ffa7ad559d1f0772e39718fc7776
> Author: Julian Phillips <julian@quantumfyre.co.uk>
> Date:   Sun Oct 25 21:28:11 2009 +0000
> 
>     remote: Make ref_remove_duplicates faster for large numbers of refs

Can't reproduce because I don't know how to create duplicate refs, but does
the following help?


 remote.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/remote.c b/remote.c
index 4f9f0cc..10cc985 100644
--- a/remote.c
+++ b/remote.c
@@ -754,6 +754,8 @@ void ref_remove_duplicates(struct ref *ref_map)
 			prev->next = ref_map->next;
 			free(ref_map->peer_ref);
 			free(ref_map);
+			ref_map = next;
+			continue;
 		}
 
 		item = string_list_insert(ref_map->peer_ref->name, &refs);

Re: Git in next is broken

[Julian Phillips]

On Thu, 12 Nov 2009, Ren? Scharfe wrote:

> Nicolas Pitre schrieb:
>> Simply issuing a "git fetch" in my copy of git.git makes glibc complain
>> with this:
>>
>> *** glibc detected *** git: corrupted double-linked list: 0x0000000000974180 ***
>>
>> The gdb backtrace is:
>>
>> (gdb) bt
>> #0  0x0000003c76632f05 in raise () from /lib64/libc.so.6
>> #1  0x0000003c76634a73 in abort () from /lib64/libc.so.6
>> #2  0x0000003c76672438 in __libc_message () from /lib64/libc.so.6
>> #3  0x0000003c76677ec8 in malloc_printerr () from /lib64/libc.so.6
>> #4  0x0000003c7667a23e in _int_free () from /lib64/libc.so.6
>> #5  0x0000003c7667a486 in free () from /lib64/libc.so.6
>> #6  0x0000000000493f3f in ref_remove_duplicates (ref_map=0x7562b0)
>>     at remote.c:756
>> #7  0x0000000000424afc in get_ref_map () at builtin-fetch.c:165
>> #8  do_fetch () at builtin-fetch.c:644
>> #9  cmd_fetch (argc=<value optimized out>, argv=0x7fffffffe6a0,
>>     prefix=<value optimized out>) at builtin-fetch.c:754
>> #10 0x0000000000403d83 in run_builtin () at git.c:251
>> #11 handle_internal_command (argc=1, argv=0x7fffffffe6a0) at git.c:396
>> #12 0x0000000000403f2d in run_argv () at git.c:438
>> #13 main (argc=1, argv=0x7fffffffe6a0) at git.c:509
>>
>> Bisection reveals the following culprit:
>>
>> commit 73cf0822b2a4ffa7ad559d1f0772e39718fc7776
>> Author: Julian Phillips <julian@quantumfyre.co.uk>
>> Date:   Sun Oct 25 21:28:11 2009 +0000
>>
>>     remote: Make ref_remove_duplicates faster for large numbers of refs
>
> Can't reproduce because I don't know how to create duplicate refs, but does
> the following help?
>
>
> remote.c |    2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 4f9f0cc..10cc985 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -754,6 +754,8 @@ void ref_remove_duplicates(struct ref *ref_map)
> 			prev->next = ref_map->next;
> 			free(ref_map->peer_ref);
> 			free(ref_map);
> +			ref_map = next;

You don't need this line (this is taken care of in the for(...)).

> +			continue;

Ack. This one however, you do need.  Good catch.

> 		}
>
> 		item = string_list_insert(ref_map->peer_ref->name, &refs);
>

-- 
Julian

  ---
Punishment becomes ineffective after a certain point.  Men become insensitive.
 		-- Eneg, "Patterns of Force", stardate 2534.7

Re: Git in next is broken

[Nicolas Pitre]

On Thu, 12 Nov 2009, Julian Phillips wrote:

> On Thu, 12 Nov 2009, Ren? Scharfe wrote:
> 
> > Nicolas Pitre schrieb:
> > > Simply issuing a "git fetch" in my copy of git.git makes glibc complain
> > > with this:
> > > 
> > > *** glibc detected *** git: corrupted double-linked list:
> > > 0x0000000000974180 ***
> > > 
> > > The gdb backtrace is:
> > > 
> > > (gdb) bt
> > > #0  0x0000003c76632f05 in raise () from /lib64/libc.so.6
> > > #1  0x0000003c76634a73 in abort () from /lib64/libc.so.6
> > > #2  0x0000003c76672438 in __libc_message () from /lib64/libc.so.6
> > > #3  0x0000003c76677ec8 in malloc_printerr () from /lib64/libc.so.6
> > > #4  0x0000003c7667a23e in _int_free () from /lib64/libc.so.6
> > > #5  0x0000003c7667a486 in free () from /lib64/libc.so.6
> > > #6  0x0000000000493f3f in ref_remove_duplicates (ref_map=0x7562b0)
> > >     at remote.c:756
> > > #7  0x0000000000424afc in get_ref_map () at builtin-fetch.c:165
> > > #8  do_fetch () at builtin-fetch.c:644
> > > #9  cmd_fetch (argc=<value optimized out>, argv=0x7fffffffe6a0,
> > >     prefix=<value optimized out>) at builtin-fetch.c:754
> > > #10 0x0000000000403d83 in run_builtin () at git.c:251
> > > #11 handle_internal_command (argc=1, argv=0x7fffffffe6a0) at git.c:396
> > > #12 0x0000000000403f2d in run_argv () at git.c:438
> > > #13 main (argc=1, argv=0x7fffffffe6a0) at git.c:509
> > > 
> > > Bisection reveals the following culprit:
> > > 
> > > commit 73cf0822b2a4ffa7ad559d1f0772e39718fc7776
> > > Author: Julian Phillips <julian@quantumfyre.co.uk>
> > > Date:   Sun Oct 25 21:28:11 2009 +0000
> > > 
> > >     remote: Make ref_remove_duplicates faster for large numbers of refs
> > 
> > Can't reproduce because I don't know how to create duplicate refs, but does
> > the following help?

Nope.

> > remote.c |    2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/remote.c b/remote.c
> > index 4f9f0cc..10cc985 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -754,6 +754,8 @@ void ref_remove_duplicates(struct ref *ref_map)
> > 			prev->next = ref_map->next;
> > 			free(ref_map->peer_ref);
> > 			free(ref_map);
> > +			ref_map = next;
> 
> You don't need this line (this is taken care of in the for(...)).
> 
> > +			continue;
> 
> Ack. This one however, you do need.  Good catch.

Without the "ref_map = next" there is no change: glibc still complains 
about corruption and abort the execution.  With the "ref_map = next" 
then git simply segfaults.

I simply have zero time to investigate the issue myself now 
unfortunately.


Nicolas

Re: Git in next is broken

[Julian Phillips]

On Thu, 12 Nov 2009, Nicolas Pitre wrote:

> On Thu, 12 Nov 2009, Julian Phillips wrote:
>
>> On Thu, 12 Nov 2009, Ren? Scharfe wrote:
>>
>>> Nicolas Pitre schrieb:
>>>> Simply issuing a "git fetch" in my copy of git.git makes glibc complain
>>>> with this:
>>>>
>>>> *** glibc detected *** git: corrupted double-linked list:
>>>> 0x0000000000974180 ***

>>> Can't reproduce because I don't know how to create duplicate refs, but does
>>> the following help?
>
> Nope.
>
>>> remote.c |    2 ++
>>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/remote.c b/remote.c
>>> index 4f9f0cc..10cc985 100644
>>> --- a/remote.c
>>> +++ b/remote.c
>>> @@ -754,6 +754,8 @@ void ref_remove_duplicates(struct ref *ref_map)
>>> 			prev->next = ref_map->next;
>>> 			free(ref_map->peer_ref);
>>> 			free(ref_map);
>>> +			ref_map = next;
>>
>> You don't need this line (this is taken care of in the for(...)).
>>
>>> +			continue;
>>
>> Ack. This one however, you do need.  Good catch.
>
> Without the "ref_map = next" there is no change: glibc still complains
> about corruption and abort the execution.  With the "ref_map = next"
> then git simply segfaults.
>
> I simply have zero time to investigate the issue myself now
> unfortunately.

I was half right about "ref_map = next", I had forgotten about setting 
prev in the for(...).  For me, the following fixes it on MacOS (I don't 
have time to test on Linux right now):

diff --git a/remote.c b/remote.c
index 4f9f0cc..6195a58 100644
--- a/remote.c
+++ b/remote.c
@@ -754,6 +754,8 @@ void ref_remove_duplicates(struct ref *ref_map)
                         prev->next = ref_map->next;
                         free(ref_map->peer_ref);
                         free(ref_map);
+                       ref_map = prev; // Keep the same prev.
+                       continue;
                 }

                 item = string_list_insert(ref_map->peer_ref->name, &refs);

-- 
Julian

  ---
C'est magnifique, mais ce n'est pas l'Informatique.
 		-- Bosquet [on seeing the IBM 4341]

Re: Git in next is broken

[Nicolas Pitre]

On Fri, 13 Nov 2009, Julian Phillips wrote:

> On Thu, 12 Nov 2009, Nicolas Pitre wrote:
> 
> > Without the "ref_map = next" there is no change: glibc still complains
> > about corruption and abort the execution.  With the "ref_map = next"
> > then git simply segfaults.
> 
> I was half right about "ref_map = next", I had forgotten about setting prev in
> the for(...).  For me, the following fixes it on MacOS (I don't have time to
> test on Linux right now):
> 
> diff --git a/remote.c b/remote.c
> index 4f9f0cc..6195a58 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -754,6 +754,8 @@ void ref_remove_duplicates(struct ref *ref_map)
>                         prev->next = ref_map->next;
>                         free(ref_map->peer_ref);
>                         free(ref_map);
> +                       ref_map = prev; // Keep the same prev.
> +                       continue;
>                 }
> 
>                 item = string_list_insert(ref_map->peer_ref->name, &refs);

Yep, this fixes it for me on Linux too.  Please resend to Junio with my 
ACK.


Nicolas

Re: Git in next is broken

[Eric Raible]

Nicolas Pitre <nico <at> fluxnic.net> writes:

 > diff --git a/remote.c b/remote.c
> > index 4f9f0cc..6195a58 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -754,6 +754,8 @@ void ref_remove_duplicates(struct ref *ref_map)
> >                         prev->next = ref_map->next;
> >                         free(ref_map->peer_ref);
> >                         free(ref_map);
> > +                       ref_map = prev; // Keep the same prev.
> > +                       continue;
> >                 }
> > 

This is one of those example where the comment is essentially useless.
Wouldn't something like "// Skip the freed item" be more useful?

- Eric

[PATCH] remote: Fix glibc error in ref_remove_duplicates

[Julian Phillips]

In ref_remove_duplicates, when we encounter a duplicate and remove it
from the list we need to make sure that the prev pointer stays
pointing at the last entry and also skip over adding the just freed
entry to the string_list.

Previously fetch could crash with:
*** glibc detected *** git: corrupted double-linked list: ...

Also add a test to try and catch problems with duplicate removal in
the future.

Acked-by: Nicolas Pitre <nico@fluxnic.net>
Signed-off-by: Julian Phillips <julian@quantumfyre.co.uk>
---

Thanks to Rene for pointing me at the problem before I even looked at
it.  Made it much easier to figure out what was going wrong. :)

 remote.c         |    2 ++
 t/t5510-fetch.sh |   11 +++++++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/remote.c b/remote.c
index 4f9f0cc..002f790 100644
--- a/remote.c
+++ b/remote.c
@@ -754,6 +754,8 @@ void ref_remove_duplicates(struct ref *ref_map)
 			prev->next = ref_map->next;
 			free(ref_map->peer_ref);
 			free(ref_map);
+			ref_map = prev; // don't change the prev pointer.
+			continue;
 		}
 
 		item = string_list_insert(ref_map->peer_ref->name, &refs);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index d13c806..169af1e 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -341,4 +341,15 @@ test_expect_success 'fetch into the current branch with --update-head-ok' '
 
 '
 
+test_expect_success "should be able to fetch with duplicate refspecs" '
+        mkdir dups &&
+        cd dups &&
+        git init &&
+        git config branch.master.remote three &&
+        git config remote.three.url ../three/.git &&
+        git config remote.three.fetch +refs/heads/*:refs/remotes/origin/* &&
+        git config --add remote.three.fetch +refs/heads/*:refs/remotes/origin/* &&
+        git fetch three
+'
+
 test_done
-- 
1.6.5.rc2