threads / discuss / 22292

git-status segmentation fault in master / OS X

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

## tl;dr

10 messages between Jan 19, 2010 and Jan 21, 2010.

replies: 9people: 4as markdown or json

Jonathan del Strother· Jan 19, 2010, 17:59 UTC · lore

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
Jeff King· Jan 20, 2010, 00:41 UTC · re: Jonathan del Strother · lore

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

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?

Show 12 quoted lines
> 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
Junio C Hamano· Jan 20, 2010, 00:56 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> 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.
Show 7 quoted lines
>   # 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.

Jonathan del Strother· Jan 20, 2010, 10:43 UTC · re: Junio C Hamano · lore

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

2010/1/20 Junio C Hamano <gitster@pobox.com>:
Show 35 quoted lines
> 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...

Nguyen Thai Ngoc Duy· Jan 20, 2010, 12:58 UTC · re: Jonathan del Strother · lore

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

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
Nguyễn Thái Ngọc Duy· Jan 20, 2010, 14:09 UTC · re: Nguyen Thai Ngoc Duy · lore

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

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
Junio C Hamano· Jan 20, 2010, 19:51 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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?

Nguyen Thai Ngoc Duy· Jan 21, 2010, 01:38 UTC · re: Junio C Hamano · lore

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

On 1/21/10, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> 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
Jonathan del Strother· Jan 21, 2010, 06:08 UTC · re: Nguyen Thai Ngoc Duy · lore

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

2010/1/21 Nguyen Thai Ngoc Duy <pclouds@gmail.com>:
Show 25 quoted lines
> 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.

Junio C Hamano· Jan 21, 2010, 06:48 UTC · re: Nguyen Thai Ngoc Duy · lore

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

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
Show 5 quoted lines
> 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.

← back to recent threads