threads / discuss / 21596

Git in next is broken

Subject: Git in next is broken

## tl;dr

8 messages between Nov 12, 2009 and Nov 13, 2009.

replies: 7people: 4as markdown or json

Nicolas Pitre· Nov 12, 2009, 20:36 UTC · lore

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
René Scharfe· Nov 12, 2009, 22:17 UTC · re: Nicolas Pitre · lore

Re: Git in next is broken

Nicolas Pitre schrieb:
Show 32 quoted lines
> 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);
Julian Phillips· Nov 12, 2009, 22:45 UTC · re: René Scharfe · lore

Re: Git in next is broken

On Thu, 12 Nov 2009, Ren? Scharfe wrote:
Show 50 quoted lines
> 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
Nicolas Pitre· Nov 13, 2009, 04:50 UTC · re: Julian Phillips · lore

Re: Git in next is broken

On Thu, 12 Nov 2009, Julian Phillips wrote:
Show 39 quoted lines
> 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.
Show 18 quoted lines
> > 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
Julian Phillips· Nov 13, 2009, 09:14 UTC · re: Nicolas Pitre · lore

Re: Git in next is broken

On Thu, 12 Nov 2009, Nicolas Pitre wrote:
Show 10 quoted lines
> 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 ***
Show 30 quoted lines
>>> 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]
Nicolas Pitre· Nov 13, 2009, 16:54 UTC · re: Julian Phillips · lore

Re: Git in next is broken

On Fri, 13 Nov 2009, Julian Phillips wrote:
Show 23 quoted lines
> 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
Eric Raible· Nov 13, 2009, 20:20 UTC · re: Nicolas Pitre · lore

Re: Git in next is broken

Nicolas Pitre <nico <at> fluxnic.net> writes:
 > diff --git a/remote.c b/remote.c
Show 11 quoted lines
> > 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
Julian Phillips· Nov 13, 2009, 21:25 UTC · re: Nicolas Pitre · lore

[PATCH] remote: Fix glibc error in ref_remove_duplicates

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

← back to recent threads