Re: Re: Segmentation fault with latest git (070c57df)

Re: Re: Segmentation fault with latest git (070c57df)

[Jongman Heo]

On Thu, Jan 31, 2013 at 7:49 AM, Jeff King wrote:
> On Thu, Jan 31, 2013 at 01:35:21AM +0000, Jongman Heo wrote:
>
>> Looks like following commit causes a segmentation fault in my machine
>> (when running git pull or git fetch);
>>
>> commit 8dd5afc926acb9829ebf56e9b78826a5242cd638
>> Author: Junio C Hamano 
>> Date:   Mon Jan 7 12:24:55 2013 -0800
>>
>>     string-list: allow case-insensitive string list
>>
>>
>> In my case, list->cmp (at get_entry_index() function) has an invalid
>> address, obviously not an address of string comparision function,
>> instead it points to 1.
>
> Can you show us a stack trace? The string-list functions are generic and
> get called in a lot of places. It would be useful to know which list is
> causing the problem.
>
> -Peff
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi,

FYI, gdb backtrace and valgrind output attached below, Thanks.


(gdb) run fetch
Starting program: /home/hjongman/repos/git/git fetch
warning: .dynamic section for "/lib/libc.so.6" is not at the expected address
warning: difference appears to be caused by prelink, adjusting expectations
[Thread debugging using libthread_db enabled]

Program received signal SIGSEGV, Segmentation fault.
0x00000001 in ?? ()
(gdb) bt
#0  0x00000001 in ?? ()
#1  0x0812b457 in get_entry_index (list=0xbfffe7c0, string=0x821ec3c "refs/remotes/origin/HEAD", exact_match=0xbfffe568)
    at string-list.c:14
#2  0x0812bd60 in add_entry (list=0xbfffe7c0, insert_at=-1, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:33
#3  string_list_insert_at_index (list=0xbfffe7c0, insert_at=-1, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:63
#4  0x0812bda0 in string_list_insert (list=0xbfffe7c0, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:57
#5  0x08071838 in add_existing (refname=0x821ec3c "refs/remotes/origin/HEAD", 
    sha1=0x821ec14 "\a\fW\337B\352N\255\314C\320Em\021E`\022C&", <incomplete sequence \303>, flag=1, cbdata=0xbfffe7c0)
    at builtin/fetch.c:570
#6  0x0810af97 in do_one_ref (base=<value optimized out>, fn=0x8071820 <add_existing>, trim=0, flags=<value optimized out>, 
    cb_data=0xbfffe7c0, entry=0x821ec10) at refs.c:525
#7  0x0810bd9f in do_for_each_ref_in_dirs (dir1=0x8215d54, dir2=0x821ea44, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0, 
    flags=0, cb_data=0xbfffe7c0) at refs.c:627
#8  0x0810bc8e in do_for_each_ref_in_dirs (dir1=0x8215cac, dir2=0x8226954, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0, 
    flags=0, cb_data=0xbfffe7c0) at refs.c:597
#9  0x0810bc8e in do_for_each_ref_in_dirs (dir1=0x8215c0c, dir2=0x8215a54, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0, 
    flags=0, cb_data=0xbfffe7c0) at refs.c:597
#10 0x0810bc8e in do_for_each_ref_in_dirs (dir1=0x8215a1c, dir2=0x821862c, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0, 
    flags=0, cb_data=0xbfffe7c0) at refs.c:597
#11 0x0810c304 in do_for_each_ref (submodule=<value optimized out>, base=0x814f9ff "", fn=0x8071820 <add_existing>, trim=0, flags=0, 
    cb_data=0xbfffe7c0) at refs.c:1295
#12 0x0810c63b in for_each_ref (fn=0x8071820 <add_existing>, cb_data=0xbfffe7c0) at refs.c:1343
#13 0x0807390a in do_fetch (remote=<value optimized out>, argc=0, argv=0xbfffe9f8) at builtin/fetch.c:699
#14 fetch_one (remote=<value optimized out>, argc=0, argv=0xbfffe9f8) at builtin/fetch.c:949
#15 0x08074251 in cmd_fetch (argc=1, argv=0xbfffe9f8, prefix=0x0) at builtin/fetch.c:992
#16 0x0804b60b in run_builtin (argc=1, argv=0xbfffe9f8) at git.c:281
#17 handle_internal_command (argc=1, argv=0xbfffe9f8) at git.c:443
#18 0x0804ba51 in run_argv (argc=1, argv=0xbfffe9f8) at git.c:489
#19 main (argc=1, argv=0xbfffe9f8) at git.c:564


$ valgrind git fetch
==2195== Memcheck, a memory error detector
==2195== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==2195== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info
==2195== Command: git fetch
==2195== 
==2195== Conditional jump or move depends on uninitialised value(s)
==2195==    at 0x812B41F: get_entry_index (string-list.c:10)
==2195==    by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
==2195==    by 0x812BD9F: string_list_insert (string-list.c:57)
==2195==    by 0x8071837: add_existing (fetch.c:570)
==2195==    by 0x810AF96: do_one_ref (refs.c:525)
==2195==    by 0x810BB20: do_for_each_ref_in_dir (refs.c:551)
==2195==    by 0x810BD34: do_for_each_ref_in_dirs (refs.c:623)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810C303: do_for_each_ref (refs.c:1295)
==2195==    by 0x810C63A: for_each_ref (refs.c:1343)
==2195==    by 0x8073909: fetch_one (fetch.c:699)
==2195==    by 0x8074250: cmd_fetch (fetch.c:992)
==2195== 
==2195== Use of uninitialised value of size 4
==2195==    at 0x812B454: get_entry_index (string-list.c:14)
==2195==    by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
==2195==    by 0x812BD9F: string_list_insert (string-list.c:57)
==2195==    by 0x8071837: add_existing (fetch.c:570)
==2195==    by 0x810AF96: do_one_ref (refs.c:525)
==2195==    by 0x810BD9E: do_for_each_ref_in_dirs (refs.c:627)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810C303: do_for_each_ref (refs.c:1295)
==2195==    by 0x810C63A: for_each_ref (refs.c:1343)
==2195==    by 0x8073909: fetch_one (fetch.c:699)
==2195== 
==2195== 
==2195== Process terminating with default action of signal 11 (SIGSEGV)
==2195==  Access not within mapped region at address 0x0
==2195==    at 0x1: ???
==2195==    by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
==2195==    by 0x812BD9F: string_list_insert (string-list.c:57)
==2195==    by 0x8071837: add_existing (fetch.c:570)
==2195==    by 0x810AF96: do_one_ref (refs.c:525)
==2195==    by 0x810BD9E: do_for_each_ref_in_dirs (refs.c:627)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
==2195==    by 0x810C303: do_for_each_ref (refs.c:1295)
==2195==    by 0x810C63A: for_each_ref (refs.c:1343)
==2195==    by 0x8073909: fetch_one (fetch.c:699)
==2195==  If you believe this happened as a result of a stack
==2195==  overflow in your program's main thread (unlikely but
==2195==  possible), you can try to increase the size of the
==2195==  main thread stack using the --main-stacksize= flag.
==2195==  The main thread stack size used in this run was 10485760.
==2195== 
==2195== HEAP SUMMARY:
==2195==     in use at exit: 325,604 bytes in 3,579 blocks
==2195==   total heap usage: 3,687 allocs, 108 frees, 518,058 bytes allocated
==2195== 
==2195== LEAK SUMMARY:
==2195==    definitely lost: 72 bytes in 2 blocks
==2195==    indirectly lost: 0 bytes in 0 blocks
==2195==      possibly lost: 31,347 bytes in 460 blocks
==2195==    still reachable: 294,185 bytes in 3,117 blocks
==2195==         suppressed: 0 bytes in 0 blocks
==2195== Rerun with --leak-check=full to see details of leaked memory
==2195== 
==2195== For counts of detected and suppressed errors, rerun with: -v
==2195== Use --track-origins=yes to see where uninitialised values come from
==2195== ERROR SUMMARY: 3 errors from 2 contexts (suppressed: 17 from 8)
Segmentation fault

Re: Re: Segmentation fault with latest git (070c57df)

[Jeff King]

On Thu, Jan 31, 2013 at 07:27:04AM +0000, Jongman Heo wrote:

> FYI, gdb backtrace and valgrind output attached below, Thanks.

Thanks, that's helpful.

> #4  0x0812bda0 in string_list_insert (list=0xbfffe7c0, string=0x821ec3c "refs/remotes/origin/HEAD") at string-list.c:57
> #5  0x08071838 in add_existing (refname=0x821ec3c "refs/remotes/origin/HEAD", 
>     sha1=0x821ec14 "\a\fW\337B\352N\255\314C\320Em\021E`\022C&", <incomplete sequence \303>, flag=1, cbdata=0xbfffe7c0)
>     at builtin/fetch.c:570

So we are inserting the string from add_existing, which gets the list
from a callback parameter. Which comes from...

> #13 0x0807390a in do_fetch (remote=<value optimized out>, argc=0, argv=0xbfffe9f8) at builtin/fetch.c:699

...here, which does this:

  struct string_list existing_refs = STRING_LIST_INIT_NODUP;
  [...]
  for_each_ref(add_existing, &existing_refs);

And yet we get:

> ==2195== Conditional jump or move depends on uninitialised value(s)
> ==2195==    at 0x812B41F: get_entry_index (string-list.c:10)
> ==2195==    by 0x812BD5F: string_list_insert_at_index (string-list.c:33)
> ==2195==    by 0x812BD9F: string_list_insert (string-list.c:57)
> ==2195==    by 0x8071837: add_existing (fetch.c:570)
> ==2195==    by 0x810AF96: do_one_ref (refs.c:525)
> ==2195==    by 0x810BB20: do_for_each_ref_in_dir (refs.c:551)
> ==2195==    by 0x810BD34: do_for_each_ref_in_dirs (refs.c:623)
> ==2195==    by 0x810BC8D: do_for_each_ref_in_dirs (refs.c:597)
> ==2195==    by 0x810C303: do_for_each_ref (refs.c:1295)
> ==2195==    by 0x810C63A: for_each_ref (refs.c:1343)
> ==2195==    by 0x8073909: fetch_one (fetch.c:699)
> ==2195==    by 0x8074250: cmd_fetch (fetch.c:992)

which seems odd. cmp should be initialized to NULL, and then we never
touch it (and even if we did, it wouldn't be unitialized, but rather
have the value we put in it).

It's almost like the compiler is getting the initializer wrong. It's a
long shot, but I wonder if the presence of the bitfield could be
triggering a compiler bug (or there is a subtle C rule about bitfield
initializations that I do not know). Just for the sake of my sanity,
what does the following program output for you?

-- >8 --
#include <stdio.h>
#include <stdlib.h>

typedef int (*compare_fn)(const char *, const char *);

struct foo {
  char **items;
  unsigned int nr, alloc;
  unsigned int bitfield:1;
  compare_fn cmp;
};

int main(void)
{
  struct foo f = { NULL, 0, 0, 0 };
  printf("cmp is %lu\n", (unsigned long)f.cmp);
  return 0;
}

Re: Re: Segmentation fault with latest git (070c57df)

[허종만]

> 
[snip]
> Good point. Unfortunately, I can't get either yours or mine to fail,
> neither with a recent version of gcc nor with gcc-4.1.  But I can't
> convince git to fail, either. The only gcc-4.1 I have is Debian's
> 4.1.3 release, which is not quite what the OP has.
> 
> > Or perhaps something in the build process went wrong, and fetch.c didn't
> > get the memo about the new field in the struct.  Depending on stack
> > layout, the next variable might be the 'int i' right before the
> > 'string_list list' in the code, which could explain the value of 1.
> 
> Yeah, that would make sense to me with respect to the behavior we are
> seeing, but that part of the Makefile should be pretty simple and
> bug-free, I'd think (and from the original report, it seems like he was
> able to reproduce it well enough to bisect). Still, trying a "make clean
> && make" might be worth it just to rule that out.
> 
> Puzzled...
> 
> -Peff

Hi, all,

Thomas's test code also returns "cmp is 0". 
But "make clean && make" fixes my issue.

Sorry for the noise I made.
But usually when I build upstream Linux kernel, I don't do "make clean" after git pull.. 
I didn't expect that I needed "make clean" for git build. Thanks you guys.

Best regards,
Jongman Heo.

Re: Re: Segmentation fault with latest git (070c57df)

[Jongman Heo]

Junio C Hamano<gitster@pobox.com> wrote :
> 허종만 writes:
>> But usually when I build upstream Linux kernel, I don't do "make
>> clean" after git pull..  I didn't expect that I needed "make
>> clean" for git build.
>
> We don't expect anybody need "make clean", either.  There is
> something wrong in the dependency.


Hi all,

I can reproduce the issue in my machine (RedHat Enterprise 5, x86 PAE) as follows. 
But in my different machine (Fedora 16 x86) I can't reproduce.

  $ git reset --hard v1.8.1       # back to v1.8.1
  $ make clean 
  $ make all install               # this git works fine

  $ git pull                          # top commit 9a6c84e6, "Merge git://ozlabs.org/~paulus/gitk"
  $ make all install

  $ git fetch                         # this git segfaults
  Segmentation fault


So if there is any patch to test, just let me know  (but will not available during weekend).


Regards,
Jongman Heo

Re: Re: Segmentation fault with latest git (070c57df)

[Jeff King]

On Fri, Feb 01, 2013 at 09:14:41AM +0000, Jongman Heo wrote:

> I can reproduce the issue in my machine (RedHat Enterprise 5, x86 PAE) as follows. 

Great, thanks for taking the time to reproduce.

> But in my different machine (Fedora 16 x86) I can't reproduce.

That makes me wonder if it is related to the gcc or make version. I
couldn't reproduce the problem on my gcc-4.1 system, though. My make is:

  $ make --version
  GNU Make 3.81
  [...]

>   $ git reset --hard v1.8.1       # back to v1.8.1
>   $ make clean 
>   $ make all install               # this git works fine

After this step, what does builtin/.depend/fetch.o.d contain? It should
show a dependency of builtin/fetch.o on string-list (among other
things).

>   $ git pull                          # top commit 9a6c84e6, "Merge git://ozlabs.org/~paulus/gitk"
>   $ make all install

Can you try running "make -d builtin/fetch.o" here instead of "make all
install"? Can you confirm that it reads builtin/.depend/fetch.o, and
that fetch.o gets rebuilt (you should even be able to see the list of
"newer than" dependencies in the debug output)?

Another thing to double-check: does it work if you instead run

  make all install COMPUTE_HEADER_DEPENDENCIES=no

?

-Peff

Re: Re: Segmentation fault with latest git (070c57df)

[Jongman Heo]

Jonathan Nieder wrote:
> Jongman Heo wrote:
> 
>> Unfortunately, the patch didn't help to me.
>
>Thanks for testing.  Did you apply the patch to the older version of
>git that generates builtin/.depend/fetch.o.d or the newer version that
>consumes it?
>
>Curious,
>Jonathan

Hi, Jonathan,

I applied the patch to newer version. 

This time, I tried to apply the patch to older version of Makefile, and now the issue is fixed~. 
Thanks~!

Best regards,
Jongman Heo