{"thread":{"id":"43945","subject":"Crash when using git blame on untracked file","startedAt":"2016-08-27T17:58:45Z","lastAt":"2016-08-29T21:17:57Z","messageCount":5,"participants":["Simon Ruderich","Thomas Gummerer","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"300337","messageId":"20160827175336.bphgkb6zclvwfntk@ruderich.org","threadId":"43945","inReplyTo":null,"subject":"Crash when using git blame on untracked file","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2016-08-27T17:53:36Z","receivedAt":"2016-08-27T17:58:45Z","isPatch":false,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"Hello,\n\nI'm seeing the following crash with Git 2.9.3 on Debian sid\n(amd64):\n\n    $ git init foo\n    $ cd foo\n    $ touch x\n    $ git add x\n    $ git commit -m test\n    $ touch x.conf\n    $ git blame x.conf\n    segmentation fault\n\nI've tested it on Debian stable's 2.1.4 which works fine:\n\n    $ git blame x.conf\n    fatal: no such path 'x.conf' in HEAD\n\nIt requires the blamed file to be present, but in some cases it\nworks only if the file e.g. \"x\" is already tracked in Git and the\nother file is called \"x.conf\" (\".conf\"-suffix). But in an empty\nrepo it seems to happen always.\n\nSadly Debian's git has no dbg-package, so the stacktrace is not\nvery useful:\n\n    #0  __strcmp_sse2_unaligned () at ../sysdeps/x86_64/multiarch/strcmp-sse2-unaligned.S:31\n    #1  0x000000000041ad7a in ?? ()\n    #2  0x0000000000406171 in ?? ()\n    #3  0x0000000000405321 in ?? ()\n    #4  0x00007ffff6f9f700 in __libc_start_main (main=0x4051c0, argc=0x3, argv=0x7fffffffe1a8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe198)\n        at ../csu/libc-start.c:291\n    #5  0x00000000004057d9 in ?? ()\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"300354","messageId":"20160827200150.GA18723@hank","threadId":"43945","inReplyTo":"20160827175336.bphgkb6zclvwfntk@ruderich.org","subject":"Re: Crash when using git blame on untracked file","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-08-27T20:01:50Z","receivedAt":"2016-08-27T20:01:53Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Hi,\n\nOn 08/27, Simon Ruderich wrote:\n> Hello,\n> \n> I'm seeing the following crash with Git 2.9.3 on Debian sid\n> (amd64):\n> \n>     $ git init foo\n>     $ cd foo\n>     $ touch x\n>     $ git add x\n>     $ git commit -m test\n>     $ touch x.conf\n>     $ git blame x.conf\n>     segmentation fault\n> \n> I've tested it on Debian stable's 2.1.4 which works fine:\n> \n>     $ git blame x.conf\n>     fatal: no such path 'x.conf' in HEAD\n> \n> It requires the blamed file to be present, but in some cases it\n> works only if the file e.g. \"x\" is already tracked in Git and the\n> other file is called \"x.conf\" (\".conf\"-suffix). But in an empty\n> repo it seems to happen always.\n> \n> Sadly Debian's git has no dbg-package, so the stacktrace is not\n> very useful:\n> \n>     #0  __strcmp_sse2_unaligned () at ../sysdeps/x86_64/multiarch/strcmp-sse2-unaligned.S:31\n>     #1  0x000000000041ad7a in ?? ()\n>     #2  0x0000000000406171 in ?? ()\n>     #3  0x0000000000405321 in ?? ()\n>     #4  0x00007ffff6f9f700 in __libc_start_main (main=0x4051c0, argc=0x3, argv=0x7fffffffe1a8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe198)\n>         at ../csu/libc-start.c:291\n>     #5  0x00000000004057d9 in ?? ()\n> \n\nThank you very much for the bug report.  A proposed fix for it below:\n\n--- >8 ---\nSubject: [PATCH] blame: fix segfault on untracked files\n\nSince 3b75ee9 (\"blame: allow to blame paths freshly added to the index\",\n2016-07-16) git blame also looks at the index to determine if there is a\nfile that was freshly added to the index.\n\ncache_name_pos returns -pos - 1 in case there is no match is found, or\nif the name matches, but the entry has a stage other than 0.  As git\nblame should work for unmerged files, it uses strcmp to determine\nwhether the name of the returned position matches, in which case the\nfile exists, but is merely unmerged, or if the file actually doesn't\nexist in the index.\n\nIf the repository is empty, or if the file would lexicographically be\nsorted as the last file in the repository, -cache_name_pos - 1 is\noutside of the length of the active_cache array, causing git blame to\nsegfault.  Guard against that, and die() normally to restore the old\nbehaviour.\n\nReported-by: Simon Ruderich <simon@ruderich.org>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/blame.c  | 3 ++-\n t/t8002-blame.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 7ec7823..a5bbf91 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2244,7 +2244,8 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path)\n \tpos = cache_name_pos(path, strlen(path));\n \tif (pos >= 0)\n \t\t; /* path is in the index */\n-\telse if (!strcmp(active_cache[-1 - pos]->name, path))\n+\telse if (-1 - pos < active_nr &&\n+\t\t !strcmp(active_cache[-1 - pos]->name, path))\n \t\t; /* path is in the index, unmerged */\n \telse\n \t\tdie(\"no such path '%s' in HEAD\", path);\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex ff09ace..7983bb7 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -6,6 +6,13 @@ test_description='git blame'\n PROG='git blame -c'\n . \"$TEST_DIRECTORY\"/annotate-tests.sh\n \n+test_expect_success 'blame untracked file in empty repo' '\n+\ttouch untracked &&\n+\ttest_must_fail git blame untracked 2>actual.err &&\n+\techo \"fatal: no such path '\\''untracked'\\'' in HEAD\" >expected.err &&\n+\ttest_cmp expected.err actual.err\n+'\n+\n PROG='git blame -c -e'\n test_expect_success 'blame --show-email' '\n \tcheck_count \\\n-- \n2.8.4.1.g3b75ee9\n\n"},{"id":"300480","messageId":"xmqqeg57bfcu.fsf@gitster.mtv.corp.google.com","threadId":"43945","inReplyTo":"20160827200150.GA18723@hank","subject":"Re: Crash when using git blame on untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T18:27:13Z","receivedAt":"2016-08-29T18:27:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> Subject: [PATCH] blame: fix segfault on untracked files\n>\n> Since 3b75ee9 (\"blame: allow to blame paths freshly added to the index\",\n> 2016-07-16) git blame also looks at the index to determine if there is a\n> file that was freshly added to the index.\n>\n> cache_name_pos returns -pos - 1 in case there is no match is found, or\n> if the name matches, but the entry has a stage other than 0.  As git\n> blame should work for unmerged files, it uses strcmp to determine\n> whether the name of the returned position matches, in which case the\n> file exists, but is merely unmerged, or if the file actually doesn't\n> exist in the index.\n>\n> If the repository is empty, or if the file would lexicographically be\n> sorted as the last file in the repository, -cache_name_pos - 1 is\n> outside of the length of the active_cache array, causing git blame to\n> segfault.  Guard against that, and die() normally to restore the old\n> behaviour.\n>\n> Reported-by: Simon Ruderich <simon@ruderich.org>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n\nThis is a recent regression and unfortunately is also in 2.9.3; the\npatch looks obviously correct.\n\n>  builtin/blame.c  | 3 ++-\n>  t/t8002-blame.sh | 7 +++++++\n>  2 files changed, 9 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 7ec7823..a5bbf91 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2244,7 +2244,8 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path)\n>  \tpos = cache_name_pos(path, strlen(path));\n>  \tif (pos >= 0)\n>  \t\t; /* path is in the index */\n> -\telse if (!strcmp(active_cache[-1 - pos]->name, path))\n> +\telse if (-1 - pos < active_nr &&\n> +\t\t !strcmp(active_cache[-1 - pos]->name, path))\n>  \t\t; /* path is in the index, unmerged */\n>  \telse\n>  \t\tdie(\"no such path '%s' in HEAD\", path);\n> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> index ff09ace..7983bb7 100755\n> --- a/t/t8002-blame.sh\n> +++ b/t/t8002-blame.sh\n> @@ -6,6 +6,13 @@ test_description='git blame'\n>  PROG='git blame -c'\n>  . \"$TEST_DIRECTORY\"/annotate-tests.sh\n>  \n> +test_expect_success 'blame untracked file in empty repo' '\n> +\ttouch untracked &&\n> +\ttest_must_fail git blame untracked 2>actual.err &&\n> +\techo \"fatal: no such path '\\''untracked'\\'' in HEAD\" >expected.err &&\n> +\ttest_cmp expected.err actual.err\n> +'\n\nThe point of this fix is not that we show the exact error message,\nbut we fail in a controlled manner.  I think\n\n        test_expect_success 'blame untracked file in empty repo' '\n                >untracked &&\n                test_must_fail git blame untracked\n        '\n\nis sufficient.\n\nThanks.\n\n>  PROG='git blame -c -e'\n>  test_expect_success 'blame --show-email' '\n>  \tcheck_count \\\n"},{"id":"300488","messageId":"20160829195019.30876-1-t.gummerer@gmail.com","threadId":"43945","inReplyTo":"xmqqeg57bfcu.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] blame: fix segfault on untracked files","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-08-29T19:50:19Z","receivedAt":"2016-08-29T19:50:33Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Since 3b75ee9 (\"blame: allow to blame paths freshly added to the index\",\n2016-07-16) git blame also looks at the index to determine if there is a\nfile that was freshly added to the index.\n\ncache_name_pos returns -pos - 1 in case there is no match is found, or\nif the name matches, but the entry has a stage other than 0.  As git\nblame should work for unmerged files, it uses strcmp to determine\nwhether the name of the returned position matches, in which case the\nfile exists, but is merely unmerged, or if the file actually doesn't\nexist in the index.\n\nIf the repository is empty, or if the file would lexicographically be\nsorted as the last file in the repository, -cache_name_pos - 1 is\noutside of the length of the active_cache array, causing git blame to\nsegfault.  Guard against that, and die() normally to restore the old\nbehaviour.\n\nReported-by: Simon Ruderich <simon@ruderich.org>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n> The point of this fix is not that we show the exact error message,\n> but we fail in a controlled manner.  I think\n>\n> test_expect_success 'blame untracked file in empty repo' '\n>                >untracked &&\n>                test_must_fail git blame untracked\n>        '\n>\n> is sufficient.\n\nYeah, I agree with that.\n\nThanks for the review!\n\n builtin/blame.c  | 3 ++-\n t/t8002-blame.sh | 5 +++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 7ec7823..a5bbf91 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2244,7 +2244,8 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path)\n \tpos = cache_name_pos(path, strlen(path));\n \tif (pos >= 0)\n \t\t; /* path is in the index */\n-\telse if (!strcmp(active_cache[-1 - pos]->name, path))\n+\telse if (-1 - pos < active_nr &&\n+\t\t !strcmp(active_cache[-1 - pos]->name, path))\n \t\t; /* path is in the index, unmerged */\n \telse\n \t\tdie(\"no such path '%s' in HEAD\", path);\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex ff09ace..ab79de9 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -6,6 +6,11 @@ test_description='git blame'\n PROG='git blame -c'\n . \"$TEST_DIRECTORY\"/annotate-tests.sh\n \n+test_expect_success 'blame untracked file in empty repo' '\n+\t>untracked &&\n+\ttest_must_fail git blame untracked\n+'\n+\n PROG='git blame -c -e'\n test_expect_success 'blame --show-email' '\n \tcheck_count \\\n-- \n2.10.0.rc2.246.g4fd2c60\n\n"},{"id":"300510","messageId":"xmqqd1kr8ebs.fsf@gitster.mtv.corp.google.com","threadId":"43945","inReplyTo":"20160829195019.30876-1-t.gummerer@gmail.com","subject":"Re: [PATCH v2] blame: fix segfault on untracked files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-29T21:17:43Z","receivedAt":"2016-08-29T21:17:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n>> The point of this fix is not that we show the exact error message,\n>> but we fail in a controlled manner.  I think\n>>\n>> test_expect_success 'blame untracked file in empty repo' '\n>>                >untracked &&\n>>                test_must_fail git blame untracked\n>>        '\n>>\n>> is sufficient.\n>\n> Yeah, I agree with that.\n>\n> Thanks for the review!\n\nThanks for a quick follow-up fix during -rc period.\n\n>>  builtin/blame.c  | 3 ++-\n>  t/t8002-blame.sh | 5 +++++\n>  2 files changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 7ec7823..a5bbf91 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2244,7 +2244,8 @@ static void verify_working_tree_path(struct commit *work_tree, const char *path)\n>  \tpos = cache_name_pos(path, strlen(path));\n>  \tif (pos >= 0)\n>  \t\t; /* path is in the index */\n> -\telse if (!strcmp(active_cache[-1 - pos]->name, path))\n> +\telse if (-1 - pos < active_nr &&\n> +\t\t !strcmp(active_cache[-1 - pos]->name, path))\n>  \t\t; /* path is in the index, unmerged */\n>  \telse\n>  \t\tdie(\"no such path '%s' in HEAD\", path);\n> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\n> index ff09ace..ab79de9 100755\n> --- a/t/t8002-blame.sh\n> +++ b/t/t8002-blame.sh\n> @@ -6,6 +6,11 @@ test_description='git blame'\n>  PROG='git blame -c'\n>  . \"$TEST_DIRECTORY\"/annotate-tests.sh\n>  \n> +test_expect_success 'blame untracked file in empty repo' '\n> +\t>untracked &&\n> +\ttest_must_fail git blame untracked\n> +'\n> +\n>  PROG='git blame -c -e'\n>  test_expect_success 'blame --show-email' '\n>  \tcheck_count \\\n"}]}