git-status segmentation fault in master / OS X

git-status segmentation fault in master / OS X

[Jonathan del Strother]

Heya,
I've been running into a segmentation fault on running git status in
my repository while there are staged changes.  Bisecting suggests it
was introduced in:

commit 73d66323ac78c750ba42fef23b1cb8fd2110e023
Merge: 054d2fa 8740773
Author: Junio C Hamano <gitster@pobox.com>
Date:   Wed Jan 13 11:58:34 2010 -0800

    Merge branch 'nd/sparse'


The last commit from nd/sparse (8740773) is fine, so I guess it's a
bad merge...?


Here's the crash I get on 73d6632 :

Process:         git [93736]
Path:            /Users/jon/bin/git
Identifier:      git
Version:         ??? (???)
Code Type:       X86-64 (Native)
Parent Process:  zsh [26194]

Date/Time:       2010-01-19 17:58:01.306 +0000
OS Version:      Mac OS X 10.6.2 (10C540)
Report Version:  6

Interval Since Last Report:          349902 sec
Crashes Since Last Report:           15
Per-App Crashes Since Last Report:   11
Anonymous UUID:                      2563166D-332E-42BE-9D2D-0E741A6DB38A

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000000
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   libSystem.B.dylib             	0x00007fff81260a56 fnmatch1 + 442
1   libSystem.B.dylib             	0x00007fff8126088a fnmatch + 124
2   git                           	0x0000000100076cde excluded_from_list + 526
3   git                           	0x0000000100077877 excluded + 519
4   git                           	0x0000000100077ab5
read_directory_recursive + 453
5   git                           	0x0000000100077ff9 read_directory + 249
6   git                           	0x000000010007812b fill_directory + 219
7   git                           	0x00000001000b91a7 wt_status_collect + 599
8   git                           	0x0000000100017380 cmd_status + 288
(builtin-commit.c:1032)
9   git                           	0x0000000100001d1c
handle_internal_command + 188 (git.c:257)
10  git                           	0x0000000100001f5c main + 236 (git.c:446)
11  git                           	0x0000000100001814 start + 52

Thread 0 crashed with X86 Thread State (64-bit):
  rax: 0x0000000000000003  rbx: 0x0000000000000000  rcx:
0x00007fff5fbfe260  rdx: 0x0000000000000006
  rdi: 0x000000000000002a  rsi: 0x00007fff5fbfe480  rbp:
0x00007fff5fbfe1d0  rsp: 0x00007fff5fbfe010
   r8: 0x00007fff70276980   r9: 0x0000000100528720  r10:
0x0000000100528ae0  r11: 0x00007fff8120b860
  r12: 0x00007fff5fbfe480  r13: 0x0000000000000001  r14:
0x00007fff70276980  r15: 0x00007fff5fbfe260
  rip: 0x00007fff81260a56  rfl: 0x0000000000010246  cr2: 0x0000000000000000

Binary Images:
       0x100000000 -        0x1000f9fef +git ??? (???)
<FFFFCD11-3352-4216-B27D-2700D1A69326> /Users/jon/bin/git
       0x100179000 -        0x10018bfe7 +libz.1.dylib ??? (???)
<F450102F-273C-872E-0729-BD338777CA02> /opt/local/lib/libz.1.dylib
       0x10018f000 -        0x10028bff7 +libiconv.2.dylib ??? (???)
<A27D1D71-44A7-76A7-41EB-CC4BAC91E740> /opt/local/lib/libiconv.2.dylib
       0x100298000 -        0x1003acfe7 +libcrypto.0.9.8.dylib ???
(???) <D4E1B9E7-BE64-5054-F80A-B63296AFEAD4>
/opt/local/lib/libcrypto.0.9.8.dylib
       0x100410000 -        0x10044ffff +libssl.0.9.8.dylib ??? (???)
<4E8F5D81-1DFF-5CBD-361A-C37609B799A8>
/opt/local/lib/libssl.0.9.8.dylib
    0x7fff5fc00000 -     0x7fff5fc3bdef  dyld 132.1 (???)
<B633F790-4DDB-53CD-7ACF-2A3682BCEA9F> /usr/lib/dyld
    0x7fff811d2000 -     0x7fff81390ff7  libSystem.B.dylib ??? (???)
<526DD3E5-2A8B-4512-ED97-01B832369959> /usr/lib/libSystem.B.dylib
    0x7fff871c3000 -     0x7fff871c7ff7  libmathCommon.A.dylib ???
(???) <95718673-FEEE-B6ED-B127-BCDBDB60D4E5>
/usr/lib/system/libmathCommon.A.dylib
    0x7fffffe00000 -     0x7fffffe01fff  libSystem.B.dylib ??? (???)
<526DD3E5-2A8B-4512-ED97-01B832369959> /usr/lib/libSystem.B.dylib

Model: MacPro1,1, BootROM MP11.005C.B08, 4 processors, Dual-Core Intel
Xeon, 2 GHz, 3 GB, SMC 1.7f10
Graphics: ATI Radeon X1900 XT, ATY,RadeonX1900, PCIe, 512 MB
Memory Module: global_name
AirPort: spairport_wireless_card_type_airport_extreme (0x14E4, 0x87),
Broadcom BCM43xx 1.0 (5.10.91.26)
Bluetooth: Version 2.2.4f3, 2 service, 1 devices, 1 incoming serial ports
Network Service: Ethernet 1, Ethernet, en0
PCI Card: ATY,RadeonX1900, Display, Slot-1
Serial ATA Device: WDC WD2500KS-00MJB0, 232.89 GB
Serial ATA Device: WDC WD2500JS-41SGB0, 232.89 GB
Parallel ATA Device: SONY    DVD RW DW-D150A
USB Device: Hub, 0x05ac  (Apple Inc.), 0x9130, 0xfd400000
USB Device: Keyboard Hub, 0x05ac  (Apple Inc.), 0x1006, 0xfd410000
USB Device: USB-PS/2 Optical Mouse, 0x046d  (Logitech Inc.), 0xc01e, 0xfd411000
USB Device: Apple Keyboard, 0x05ac  (Apple Inc.), 0x0221, 0xfd412000
USB Device: C-Media USB Headphone Set, 0x0d8c  (C-MEDIA ELECTRONICS
INC.), 0x000c, 0xfd430000
USB Device: Apple Cinema Display, 0x05ac  (Apple Inc.), 0x9222, 0xfd420000
USB Device: Hub, 0x05ac  (Apple Inc.), 0x9131, 0xfd300000
USB Device: Apple Cinema HD Display, 0x05ac  (Apple Inc.), 0x9223, 0xfd320000
USB Device: Bluetooth USB Host Controller, 0x05ac  (Apple Inc.),
0x8206, 0x5d200000
FireWire Device: built-in_hub, Up to 800 Mb/sec
FireWire Device: unknown_device, Unknown



Want me to do any more digging?

-Jonathan

Re: git-status segmentation fault in master / OS X

[Jeff King]

On Tue, Jan 19, 2010 at 05:59:51PM +0000, Jonathan del Strother wrote:

> I've been running into a segmentation fault on running git status in
> my repository while there are staged changes.

I can't reproduce it here. Is there anything else interesting about the
repo you can tell us besides that it has staged changes?

> Bisecting suggests it was introduced in:
> 
> commit 73d66323ac78c750ba42fef23b1cb8fd2110e023
> Merge: 054d2fa 8740773
> Author: Junio C Hamano <gitster@pobox.com>
> Date:   Wed Jan 13 11:58:34 2010 -0800
> 
>     Merge branch 'nd/sparse'
> 
> 
> The last commit from nd/sparse (8740773) is fine, so I guess it's a
> bad merge...?

Could be a bad interaction between commits on nd/sparse and whatever was
done since it had branched. You can try rebasing nd/sparse and bisecting
a linearised version, like this:

  bad_merge=73d66323
  # pretend we are on nd/sparse
  git checkout -b test $bad_merge^2
  # rebase onto what we merged onto
  git rebase $bad_merge^1
  # now bisect. what we have now is presumably
  # bad (though you should probably double check)
  # and from the previous bisect we know that
  # everything pre-merge is good
  git bisect start
  git bisect good $bad_merge^1
  git bisect bad

-Peff

Re: git-status segmentation fault in master / OS X

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Could be a bad interaction between commits on nd/sparse and whatever was
> done since it had branched. You can try rebasing nd/sparse and bisecting
> a linearised version, like this:
>
>   bad_merge=73d66323
>   # pretend we are on nd/sparse
>   git checkout -b test $bad_merge^2
>   # rebase onto what we merged onto
>   git rebase $bad_merge^1

That is a very good suggestion.

You will get hit by a few conflicts during the rebase, but I managed to
arrive at the same tree as $bad_merge after running the rebase procedure
above.  Just for Jonathan's convenience, the result is at:

  git://repo.or.cz/alt-git.git junk-linearized-nd-sparse-for-bisection

I'll remove this after a few days.

>   # now bisect. what we have now is presumably
>   # bad (though you should probably double check)
>   # and from the previous bisect we know that
>   # everything pre-merge is good
>   git bisect start
>   git bisect good $bad_merge^1
>   git bisect bad

It would be interesting to hear the result of the test in the particular
repository Jonathan is seeing the problem with.  The issue didn't
reproduce for me, either but I only tried "having a staged change" case
without any more detailed set-up.

Re: git-status segmentation fault in master / OS X

[Jonathan del Strother]

2010/1/20 Junio C Hamano <gitster@pobox.com>:
> Jeff King <peff@peff.net> writes:
>
>> Could be a bad interaction between commits on nd/sparse and whatever was
>> done since it had branched. You can try rebasing nd/sparse and bisecting
>> a linearised version, like this:
>>
>>   bad_merge=73d66323
>>   # pretend we are on nd/sparse
>>   git checkout -b test $bad_merge^2
>>   # rebase onto what we merged onto
>>   git rebase $bad_merge^1
>
> That is a very good suggestion.
>
> You will get hit by a few conflicts during the rebase, but I managed to
> arrive at the same tree as $bad_merge after running the rebase procedure
> above.  Just for Jonathan's convenience, the result is at:
>
>  git://repo.or.cz/alt-git.git junk-linearized-nd-sparse-for-bisection
>
> I'll remove this after a few days.
>
>>   # now bisect. what we have now is presumably
>>   # bad (though you should probably double check)
>>   # and from the previous bisect we know that
>>   # everything pre-merge is good
>>   git bisect start
>>   git bisect good $bad_merge^1
>>   git bisect bad
>
> It would be interesting to hear the result of the test in the particular
> repository Jonathan is seeing the problem with.  The issue didn't
> reproduce for me, either but I only tried "having a staged change" case
> without any more detailed set-up.
>

OK, found some more interesting results from that.  The new bad commit is :

commit 66dce7bdb6742cb06433f8ef25441690b71c7995
Author: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date:   Thu Aug 20 20:47:01 2009 +0700

    Read .gitignore from index if it is skip-worktree


I still haven't been able to come up with a minimal test case, but gdb
gave me this:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x00000000000001c8
0x0000000100093e69 in excluded_1 (pathname=0x7fff5fbfd8b0
"shared/config/environments/.gitignore", pathlen=37,
basename=0x7fff5fbfd8cb ".gitignore", dtype=0x7fff5fbfd8a4,
el=0x7fff5fbfeb00) at dir.c:378
378					if (*exclude == '/')
(gdb) bt
#0  0x0000000100093e69 in excluded_1 (pathname=0x7fff5fbfd8b0
"shared/config/environments/.gitignore", pathlen=37,
basename=0x7fff5fbfd8cb ".gitignore", dtype=0x7fff5fbfd8a4,
el=0x7fff5fbfeb00) at dir.c:378
#1  0x0000000100093fd0 in excluded (dir=0x7fff5fbfeac0,
pathname=0x7fff5fbfd8b0 "shared/config/environments/.gitignore",
dtype_p=0x7fff5fbfd8a4) at dir.c:410
#2  0x0000000100094a10 in read_directory_recursive
(dir=0x7fff5fbfeac0, base=0x7fff5fbfdd20
"shared/config/environments/", baselen=27, check_only=0, simplify=0x0)
at dir.c:688
#3  0x0000000100094bfa in read_directory_recursive
(dir=0x7fff5fbfeac0, base=0x7fff5fbfe190 "shared/config/", baselen=14,
check_only=0, simplify=0x0) at dir.c:727
#4  0x0000000100094bfa in read_directory_recursive
(dir=0x7fff5fbfeac0, base=0x7fff5fbfe600 "shared/", baselen=7,
check_only=0, simplify=0x0) at dir.c:727
#5  0x0000000100094bfa in read_directory_recursive
(dir=0x7fff5fbfeac0, base=0x1000f48d0 "", baselen=0, check_only=0,
simplify=0x0) at dir.c:727
#6  0x0000000100094ecc in read_directory (dir=0x7fff5fbfeac0,
path=0x1000f48d0 "", len=0, pathspec=0x0) at dir.c:813
#7  0x0000000100093118 in fill_directory (dir=0x7fff5fbfeac0,
pathspec=0x0) at dir.c:70
#8  0x00000001000e85a7 in wt_status_collect_untracked
(s=0x7fff5fbfef80) at wt-status.c:346
#9  0x00000001000e86cc in wt_status_collect (s=0x7fff5fbfef80) at
wt-status.c:366
#10 0x000000010001e467 in cmd_status (argc=0, argv=0x7fff5fbff248,
prefix=0x0) at builtin-commit.c:1026
#11 0x0000000100001b52 in run_builtin (p=0x10013e3b0, argc=1,
argv=0x7fff5fbff248) at git.c:257
#12 0x0000000100001d04 in handle_internal_command (argc=1,
argv=0x7fff5fbff248) at git.c:401
#13 0x0000000100001e83 in main (argc=1, argv=0x7fff5fbff248) at git.c:482


"shared/config/environments/.gitignore" contained just
"_local_config.rb", no newline.  It seemed like adding or removing
characters from that file would 'fix' the problem, but changing them
wouldn't (ie the problem was always there when .gitignore was 16
bytes), but I'm not entirely sure because it continues to be a bit of
a Schrodinger bug.


One thing I wondered about from that commit - shouldn't the "buf =
xmalloc(size);" on dir.c:252 be "buf = xmalloc(size+1);" ?  I haven't
really looked at the program flow there, so may be wrong...

Re: git-status segmentation fault in master / OS X

[Nguyen Thai Ngoc Duy]

On 1/20/10, Jonathan del Strother <maillist@steelskies.com> wrote:
>  One thing I wondered about from that commit - shouldn't the "buf =
>  xmalloc(size);" on dir.c:252 be "buf = xmalloc(size+1);" ?  I haven't
>  really looked at the program flow there, so may be wrong...

You would also need to revert 66c3fa0 (Avoid writing to buffer in
add_excludes_from_file_1() - 2009-08-20) and see if it fixes the
problem. I think there is a potential memory corruption at "buf[i - (i
&& buf[i-1] == '\r')] = 0;". Don't know if it is the cause though.
-- 
Duy

[PATCH] Fix memory corruption when .gitignore does not end by \n

[Nguyễn Thái Ngọc Duy]

Commit b5041c5 (Avoid writing to buffer in add_excludes_from_file_1())
tried not to append '\n' at the end because the next commit
may return a buffer that does not have extra space for that.

Unfortunately it left this assignment in the loop:

  buf[i - (i && buf[i-1] == '\r')] = 0;

that can corrupt memory if "buf" is not '\n' terminated. But even if
it does not corrupt memory, the last line would not be
NULL-terminated, leading to errors later inside add_exclude().

This patch fixes it by reverting the faulty commit and make
sure "buf" is always \n terminated.

While at it, free unused memory properly.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 This patch causes a crash for me. Not sure if it does for anybody else.

  diff --git a/t/t3001-ls-files-others-exclude.sh b/t/t3001-ls-files-others-exclude.sh
  index 6d2f2b6..e7efdb5 100755
  --- a/t/t3001-ls-files-others-exclude.sh
  +++ b/t/t3001-ls-files-others-exclude.sh
  @@ -57,7 +57,7 @@ expect
   echo '*.1
   /*.3
   !*.6' >.gitignore
  -echo '*.2
  +echo -n '*.2
   two/*.4
   !*.7
   *.8' >one/.gitignore

 dir.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/dir.c b/dir.c
index 1538ad5..67c3af6 100644
--- a/dir.c
+++ b/dir.c
@@ -242,6 +242,14 @@ int add_excludes_from_file_to_list(const char *fname,
 		if (!check_index ||
 		    (buf = read_skip_worktree_file_from_index(fname, &size)) == NULL)
 			return -1;
+		if (size == 0) {
+			free(buf);
+			return 0;
+		}
+		if (buf[size-1] != '\n') {
+			buf = xrealloc(buf, size+1);
+			buf[size++] = '\n';
+		}
 	}
 	else {
 		size = xsize_t(st.st_size);
@@ -249,19 +257,21 @@ int add_excludes_from_file_to_list(const char *fname,
 			close(fd);
 			return 0;
 		}
-		buf = xmalloc(size);
+		buf = xmalloc(size+1);
 		if (read_in_full(fd, buf, size) != size) {
+			free(buf);
 			close(fd);
 			return -1;
 		}
+		buf[size++] = '\n';
 		close(fd);
 	}
 
 	if (buf_p)
 		*buf_p = buf;
 	entry = buf;
-	for (i = 0; i <= size; i++) {
-		if (i == size || buf[i] == '\n') {
+	for (i = 0; i < size; i++) {
+		if (buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
 				add_exclude(entry, base, baselen, which);
-- 
1.6.6.181.g5ee6

Re: [PATCH] Fix memory corruption when .gitignore does not end by \n

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

>  This patch causes a crash for me. Not sure if it does for anybody else.

I am puzzled.  What do you mean by this?  If this patch makes the code
crash, then it is not a fix.  Is this meant as "Jonathan, can you try this
patch and tell me what happens, so that I can diagnose the issue better?"
patch?

Is it better/safer to revert the entire nd/sparse topic from the master in
the meantime before we know what is going on?

Re: [PATCH] Fix memory corruption when .gitignore does not end by \n

[Nguyen Thai Ngoc Duy]

On 1/21/10, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>  >  This patch causes a crash for me. Not sure if it does for anybody else.
>
>
> I am puzzled.  What do you mean by this?  If this patch makes the code
>  crash, then it is not a fix.  Is this meant as "Jonathan, can you try this
>  patch and tell me what happens, so that I can diagnose the issue better?"
>  patch?

I mean the t3001 patch in comment part, which removes \n at the end of
.gitignore and crashes the unmodified git.

IOW I found a problem and this patch (not the t3001 one) should fix
it. Not sure if this causes Jonathan problem though.

>  Is it better/safer to revert the entire nd/sparse topic from the master in
>  the meantime before we know what is going on?

I would wait for Jonathan response. If this is not the cause, probably
safer to revert it.
-- 
Duy

Re: [PATCH] Fix memory corruption when .gitignore does not end by \n

[Jonathan del Strother]

2010/1/21 Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
> On 1/21/10, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>  >  This patch causes a crash for me. Not sure if it does for anybody else.
>>
>>
>> I am puzzled.  What do you mean by this?  If this patch makes the code
>>  crash, then it is not a fix.  Is this meant as "Jonathan, can you try this
>>  patch and tell me what happens, so that I can diagnose the issue better?"
>>  patch?
>
> I mean the t3001 patch in comment part, which removes \n at the end of
> .gitignore and crashes the unmodified git.
>
> IOW I found a problem and this patch (not the t3001 one) should fix
> it. Not sure if this causes Jonathan problem though.
>
>>  Is it better/safer to revert the entire nd/sparse topic from the master in
>>  the meantime before we know what is going on?
>
> I would wait for Jonathan response. If this is not the cause, probably
> safer to revert it.
> --
> Duy
>

Think it's all fixed after applying this patch - at least, I can no
longer reproduce the crash, whereas I was able to make it fail ~90% of
the time before.

Re: [PATCH] Fix memory corruption when .gitignore does not end by \n

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> I mean the t3001 patch in comment part, which removes \n at the end of
> .gitignore and crashes the unmodified git.
>
> IOW I found a problem and this patch (not the t3001 one) should fix
> it. Not sure if this causes Jonathan problem though.

Ah, I see.  And a bug that leaves a string unterminated will exhibit
different symptoms depending on what garbage happens to follow it, so it
may not be universally reproducible.

Thanks; applied (and I saw Jonathan's Ack, as well).

Thanks, both.