{"thread":{"id":"56317","subject":"Bug: Segmentation fault in git diff","startedAt":"2021-08-18T08:42:51Z","lastAt":"2021-08-19T16:58:22Z","messageCount":8,"participants":["Thomas De Zeeuw","Đoàn Trần Công Danh","Junio C Hamano","Carlo Arenas"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"433047","messageId":"40BE2EF2-0AF3-45BA-9880-8A6011B38D03@slight.dev","threadId":"56317","inReplyTo":null,"subject":"Bug: Segmentation fault in git diff","fromName":"Thomas De Zeeuw","fromEmail":"thomas@slight.dev","sentAt":"2021-08-18T08:42:45Z","receivedAt":"2021-08-18T08:42:51Z","isPatch":false,"sender":{"key":"thomas@slight.dev","avatar":null},"body":"Hello,\n\nThis is my first bug report to Git mailing list so let me know if more information is needed.\n\nRunning the following command results in a segmentation fault on macOS arm64\n$ git diff --name-only --diff-filter=U —relative\nSegmentation fault: 11\n\nI was rebasing while it happened, trying to resolve merge conflicts. This is roughly how the status of the repo looked (I changed the file names, but keep them relative the to output, i.e. ../actual-file.txt I change to ../some-file.txt).\n\n$ git status\ninteractive rebase in progress; onto a4aabaa\nLast command done (1 command done):\n   pick ad0e02e WIP\nNo commands remaining.\nYou are currently rebasing branch 'my_branch' on 'a4aabaa'.\n  (fix conflicts and then run \"git rebase --continue\")\n  (use \"git rebase --skip\" to skip this patch)\n  (use \"git rebase --abort\" to check out the original branch)\n\nChanges to be committed:\n  (use \"git restore --staged <file>...\" to unstage)\n        modified:   ../dir1/Makefile\n        modified:   ../dir1/http_spec.yaml\n        modified:   ../dir1/tests/api/file1.rs\n        modified:   file2.proto\n\nUnmerged paths:\n  (use \"git restore --staged <file>...\" to unstage)\n  (use \"git add <file>...\" to mark resolution)\n        both modified:   ../dir1/build.rs\n        both modified:   ../dir1/tests/data/file3.sql\n        both modified:   file4.go\n\nRunning it through lldb doesn’t give too much information, but just in case it helps:\n\n$ lldb -- git diff --name-only --diff-filter=U --relative\n(lldb) target create \"git\"\nCurrent executable set to 'git' (arm64).\n(lldb) settings set -- target.run-args  \"diff\" \"--name-only\" \"--diff-filter=U\" \"--relative\"\n(lldb) r\nProcess 10619 launched: '/opt/homebrew/bin/git' (arm64)\nProcess 10619 stopped\n* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)\n    frame #0: 0x00000001000ea2ac git`run_diff_files + 864\ngit`run_diff_files:\n->  0x1000ea2ac <+864>: ldr    x8, [x0, #0x8]\n    0x1000ea2b0 <+868>: strh   w19, [x8, #0x50]\n    0x1000ea2b4 <+872>: ldr    w8, [x28, #0x38]\n    0x1000ea2b8 <+876>: ubfx   w8, w8, #12, #2\nTarget 0: (git) stopped.\n(lldb) bt\n* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)\n  * frame #0: 0x00000001000ea2ac git`run_diff_files + 864\n    frame #1: 0x000000010002a65c git`cmd_diff + 1820\n    frame #2: 0x00000001000043e4 git`run_builtin + 420\n    frame #3: 0x0000000100003948 git`handle_builtin + 272\n    frame #4: 0x000000010000322c git`cmd_main + 812\n    frame #5: 0x00000001000a6ec4 git`main + 140\n    frame #6: 0x00000001a19e9450 libdyld.dylib`start + 4\n\nI got my Git binary from Homebrew, tried version 2.32.0 and 2.33.0 (currently the latest on Homebrew).\n\nOS information:\nmacOS Big Sur Version 11.4\n$ uname -a\nDarwin MacBook-Pro.local 20.5.0 Darwin Kernel Version 20.5.0: Sat May  8 05:10:31 PDT 2021; root:xnu-7195.121.3~9/RELEASE_ARM64_T8101 arm64\n\n—\n\nRegards,\n\nThomas de Zeeuw\nthomas@slight.dev\n\n\n\n"},{"id":"433063","messageId":"YRzkq47gOGfn8zsw@danh.dev","threadId":"56317","inReplyTo":"40BE2EF2-0AF3-45BA-9880-8A6011B38D03@slight.dev","subject":"Re: Bug: Segmentation fault in git diff","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-08-18T10:44:59Z","receivedAt":"2021-08-18T10:45:04Z","isPatch":false,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2021-08-18 10:42:45+0200, Thomas De Zeeuw <thomas@slight.dev> wrote:\n> Hello,\n> \n> This is my first bug report to Git mailing list so let me know if more information is needed.\n> \n> Running the following command results in a segmentation fault on macOS arm64\n> $ git diff --name-only --diff-filter=U —relative\n> Segmentation fault: 11\n\nMVCE:\n\n---- 8< ---\n#!/bin/sh\n\nrm -rf /tmp/diff-bug\ngit init /tmp/diff-bug\ncd /tmp/diff-bug\nmkdir -p dir\n\nprintf '%s\\n' one two three >file\nprintf '%s\\n' inner >dir/file\ngit add file dir/file\ngit commit -m first\n\ngit branch side\n\nprintf '%s\\n' one two >file\ngit add file\ngit commit -m checkpoint\ngit tag checkpoint\n\ngit switch side\nprintf '%s\\n' two two four >file\ngit add file\ngit commit -m side\n\ncd dir\ngit rebase checkpoint\n\ngit diff --name-only --relative\n---- >8 -----\n\nIt's NULL pointer dereference bug because pair is NULL.\nI haven't check further:\n\n---- 8< -----\n#0  run_diff_files (revs=revs@entry=0x7ffcc85ae270, option=option@entry=0)\n    at diff-lib.c:196\n196                                     pair->two->mode = wt_mode;\n----- >8 -----\n\n-- \nDanh\n"},{"id":"433067","messageId":"YR0CkdVNVKa65vbo@danh.dev","threadId":"56317","inReplyTo":"YRzkq47gOGfn8zsw@danh.dev","subject":"Re: Bug: Segmentation fault in git diff","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-08-18T12:52:33Z","receivedAt":"2021-08-18T12:52:41Z","isPatch":false,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2021-08-18 17:44:59+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:\n> On 2021-08-18 10:42:45+0200, Thomas De Zeeuw <thomas@slight.dev> wrote:\n> > Hello,\n> > \n> > This is my first bug report to Git mailing list so let me know if more information is needed.\n> > \n> > Running the following command results in a segmentation fault on macOS arm64\n> > $ git diff --name-only --diff-filter=U —relative\n> > Segmentation fault: 11\n> \n> MVCE:\n> \n> ---- 8< ---\n> #!/bin/sh\n> \n> rm -rf /tmp/diff-bug\n> git init /tmp/diff-bug\n> cd /tmp/diff-bug\n> mkdir -p dir\n> \n> printf '%s\\n' one two three >file\n> printf '%s\\n' inner >dir/file\n> git add file dir/file\n> git commit -m first\n> \n> git branch side\n> \n> printf '%s\\n' one two >file\n> git add file\n> git commit -m checkpoint\n> git tag checkpoint\n> \n> git switch side\n> printf '%s\\n' two two four >file\n> git add file\n> git commit -m side\n> \n> cd dir\n> git rebase checkpoint\n> \n> git diff --name-only --relative\n> ---- >8 -----\n> \n> It's NULL pointer dereference bug because pair is NULL.\n> I haven't check further:\n> \n> ---- 8< -----\n> #0  run_diff_files (revs=revs@entry=0x7ffcc85ae270, option=option@entry=0)\n>     at diff-lib.c:196\n> 196                                     pair->two->mode = wt_mode;\n> ----- >8 -----\n\nThis diff could fix the issue, and the test suite still passes:\n\n---- 8< ----\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f9eadc4fc1..8f303958dd 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -192,7 +192,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t * from the desired stage.\n \t\t\t */\n \t\t\tpair = diff_unmerge(&revs->diffopt, ce->name);\n-\t\t\tif (wt_mode)\n+\t\t\tif (pair && wt_mode)\n \t\t\t\tpair->two->mode = wt_mode;\n \t\t\tif (ce_stage(ce) != diff_unmerged_stage)\n \t\t\t\tcontinue;\n---- >8 -----\n\n-- \nDanh\n"},{"id":"433082","messageId":"xmqq5yw25wdu.fsf@gitster.g","threadId":"56317","inReplyTo":"YR0CkdVNVKa65vbo@danh.dev","subject":"Re: Bug: Segmentation fault in git diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-18T20:30:05Z","receivedAt":"2021-08-18T20:30:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:\n\n> This diff could fix the issue, and the test suite still passes:\n>\n> ---- 8< ----\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index f9eadc4fc1..8f303958dd 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -192,7 +192,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n>  \t\t\t * from the desired stage.\n>  \t\t\t */\n>  \t\t\tpair = diff_unmerge(&revs->diffopt, ce->name);\n> -\t\t\tif (wt_mode)\n> +\t\t\tif (pair && wt_mode)\n>  \t\t\t\tpair->two->mode = wt_mode;\n>  \t\t\tif (ce_stage(ce) != diff_unmerged_stage)\n>  \t\t\t\tcontinue;\n> ---- >8 -----\n\nLooks sensible.  \n\nThe bug is the result of a bad interaction between cd676a51 (diff\n--relative: output paths as relative to the current subdirectory,\n2008-02-12) that decided to return NULL from diff_unmerge for an\nirrelevant path outside the --relative area, and 095ce953\n(diff-files: show unmerged entries correctly, 2011-04-22) that\nassumed diff_unmerge() would always yield a usable unmerged pair.\n\nAs the NULL pair would never be added to the diff_queue(), when the\nbig loop around this area of code finishes and hands the processing\noff to diffcore_std(), the mode futzing the original code was\nattempting to do would have affected absolutely nothing, so I do not\nthink there would be any subtle breakage coming from this fix.\n\nWhen you submit an \"git am\"-able patch, please have my Acked-by.\n\nThanks.\n"},{"id":"433117","messageId":"bc7eda4ed8d52072b929a4af6e4e4ed7478ef9d6.1629361733.git.congdanhqx@gmail.com","threadId":"56317","inReplyTo":"40BE2EF2-0AF3-45BA-9880-8A6011B38D03@slight.dev","subject":"[PATCH] diff-lib: ignore all outsider if --relative asked","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-08-19T08:29:09Z","receivedAt":"2021-08-19T08:29:28Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"For diff family commands, we can tell them to exclude changes outside\nof some directories if --relative is requested.\n\nIn diff_unmerge(), NULL will be returned if the requested path is\noutside of the interesting directories, thus we'll run into NULL\npointer dereference in run_diff_files when trying to dereference\nits return value.\n\nWe can simply check for NULL there before dereferencing said\nreturn value.  However, we can do better by not running diff\non those unintesting entries.  Let's do that instead.\n\nReported-by: Thomas De Zeeuw <thomas@slight.dev>\nSigned-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n---\n\nCc: Junio C Hamano <gitster@pobox.com>\n\nNotes:\n    Check for return value of diff_unmerge is not enough.\n    \n    Yes, it works with --name-only, however, with only --relative,\n    git-diff shows unmerged entries outside of subdir, too.\n    \n    Furthermore, the filename in \"diff --cc\" ignores the relative prefix.\n    Fixing this requires touching all over places, at least from my study.\n    Let's fix the crash, first.\n    \n    We have two choices here:\n    \n    * Check pair, aka return value of diff_unmerge, like my original\n      suggestion, and the unmerged entries from outside will be shown, too.\n      Some inconsistent will be observed, --name-only won't list files\n      outside of subdir, while the patch shows them.  At least, it doesn't\n      create false impression of no change outside of subdir.\n    \n    * Skip all outsiders, like this patch.\n    \n    While I prefer this approach, I don't know all ramifications of this change,\n    let's say an entry moved to outside of subdir in one side, and modified in\n    other side.\n    \n    Because, I pick the different approach, Junio's ack isn't included here.\n    \n\n diff-lib.c               |  4 +++\n t/t4045-diff-relative.sh | 53 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f9eadc4fc1..ca085a03ef 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -117,6 +117,10 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n \t\t\tcontinue;\n \n+\t\tif (revs->diffopt.prefix &&\n+\t\t    strncmp(ce->name, revs->diffopt.prefix, revs->diffopt.prefix_length))\n+\t\t\tcontinue;\n+\n \t\tif (ce_stage(ce)) {\n \t\t\tstruct combine_diff_path *dpath;\n \t\t\tstruct diff_filepair *pair;\ndiff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh\nindex 61ba5f707f..8cbbe53262 100755\n--- a/t/t4045-diff-relative.sh\n+++ b/t/t4045-diff-relative.sh\n@@ -162,4 +162,57 @@ check_diff_relative_option subdir file2 true --no-relative --relative\n check_diff_relative_option . file2 false --no-relative --relative=subdir\n check_diff_relative_option . file2 true --no-relative --relative=subdir\n \n+test_expect_success 'setup diff --relative unmerged' '\n+\ttest_commit zero file0 &&\n+\ttest_commit base subdir/file0 &&\n+\tgit switch -c br1 &&\n+\ttest_commit one file0 &&\n+\ttest_commit sub1 subdir/file0 &&\n+\tgit switch -c br2 base &&\n+\ttest_commit two file0 &&\n+\tgit switch -c br3 &&\n+\ttest_commit sub3 subdir/file0\n+'\n+\n+test_expect_success 'diff --relative without change in subdir' '\n+\tgit switch br2 &&\n+\ttest_when_finished \"git merge --abort\" &&\n+\ttest_must_fail git merge one &&\n+\tgit -C subdir diff --relative >out &&\n+\ttest_must_be_empty out &&\n+\tgit -C subdir diff --relative --name-only >out &&\n+\ttest_must_be_empty out\n+'\n+\n+test_expect_success 'diff --relative --name-only with change in subdir' '\n+\tgit switch br3 &&\n+\ttest_when_finished \"git merge --abort\" &&\n+\ttest_must_fail git merge sub1 &&\n+\ttest_write_lines file0 file0 >expected &&\n+\tgit -C subdir diff --relative --name-only >out &&\n+\ttest_cmp expected out\n+'\n+\n+test_expect_failure 'diff --relative with change in subdir' '\n+\tgit switch br3 &&\n+\tbr1_blob=$(git rev-parse --short --verify br1:subdir/file0) &&\n+\tbr3_blob=$(git rev-parse --short --verify br3:subdir/file0) &&\n+\ttest_when_finished \"git merge --abort\" &&\n+\ttest_must_fail git merge br1 &&\n+\tcat >expected <<-EOF &&\n+\tdiff --cc file0\n+\tindex $br3_blob,$br1_blob..0000000\n+\t--- a/file0\n+\t+++ b/file0\n+\t@@@ -1,1 -1,1 +1,5 @@@\n+\t++<<<<<<< HEAD\n+\t +sub3\n+\t++=======\n+\t+ sub1\n+\t++>>>>>>> sub1\n+\tEOF\n+\tgit -C subdir diff --relative >out &&\n+\ttest_cmp expected out\n+'\n+\n test_done\n-- \n2.33.0.254.g68ee769121\n\n"},{"id":"433120","messageId":"CAPUEspg+LUP59DJ+Z-AqKW0hfz=AbMfvOTFLe1rT5g5auV7gdA@mail.gmail.com","threadId":"56317","inReplyTo":"bc7eda4ed8d52072b929a4af6e4e4ed7478ef9d6.1629361733.git.congdanhqx@gmail.com","subject":"Re: [PATCH] diff-lib: ignore all outsider if --relative asked","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-08-19T09:02:43Z","receivedAt":"2021-08-19T09:02:58Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Awesome work, and this not only fixes the crash but a bug in\n--relative that has been going for a while, after all the\ndocumentation clearly says that --relative should filter out entries\noutside the tree and it does unless in this crashing scenario, which\nas pointed out was also showing the wrong paths for the diff chunks\noutside of the directory.\n\nMy only concern is that it seems this has been broken for a while\n(couldn't bisect, but AFAIK the first implementation did filter like\nthis one does), so some people might be expecting the broken\nbehaviour.\n\ngot my Tested-by or Reviewed-by and gratitude.\n\nCarlo\n"},{"id":"433131","messageId":"xmqqwnoh2x2i.fsf@gitster.g","threadId":"56317","inReplyTo":"bc7eda4ed8d52072b929a4af6e4e4ed7478ef9d6.1629361733.git.congdanhqx@gmail.com","subject":"Re: [PATCH] diff-lib: ignore all outsider if --relative asked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-19T16:55:49Z","receivedAt":"2021-08-19T16:55:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:\n\n> For diff family commands, we can tell them to exclude changes outside\n> of some directories if --relative is requested.\n>\n> In diff_unmerge(), NULL will be returned if the requested path is\n> outside of the interesting directories, thus we'll run into NULL\n> pointer dereference in run_diff_files when trying to dereference\n> its return value.\n>\n> We can simply check for NULL there before dereferencing said\n> return value.  However, we can do better by not running diff\n> on those unintesting entries.  Let's do that instead.\n>\n> Reported-by: Thomas De Zeeuw <thomas@slight.dev>\n> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>\n> ---\n\nNicely done.\n\nIf we look at cd676a51 (diff --relative: output paths as relative to\nthe current subdirectory, 2008-02-12) where the \"--relative\" feature\nwas introduced a bit more carefully, we notice that it wanted to\nimplement \"anything outside the .prefix gets discarded\" at\ndiff_addremove(), diff_change(), and diff_unmerge() level, instead\nof the side that enumerates the paths and calls these helpers, and\nthat way, the \"--relative\" feature would consistently work across\ndiff-files, diff-tree, and diff-index, as they all share these three\nhelpers.\n\nBut filtering upfront before the codepath even has to decide if it\nneeds to call diff_addremove() or diff_change(), like this patch\ndoes, makes sense, especially in the context of diff-files where the\nenumeration of paths is just to walk a single flat array that is the\nin-core index.\n\nThe proposed log message needs a bit more work, though.  It would be\nan 80% OK explanation if the \"check diff_unmerge()'s return value\"\napproach was sufficient to correct bugs and we took the approach,\nbut that is not the case.  As you found out, it is not sufficient,\nand it is not the approach you took.  The only part in the proposed\nlog that explains the approach that was actually taken was \"we can\ndo better by ...\".\n\nUntil/unless we do similar \"filter with diffopt.prefix upfront\" in\ndiff-index and diff-tree codepaths, we unfortunately cannot lose the\nfilter added to diff_addremove() and diff_change(), but I think this\nis a good first step towards such a longer-term clean-up.\n\nThanks.\n"},{"id":"433132","messageId":"xmqqsfz52wyf.fsf@gitster.g","threadId":"56317","inReplyTo":"CAPUEspg+LUP59DJ+Z-AqKW0hfz=AbMfvOTFLe1rT5g5auV7gdA@mail.gmail.com","subject":"Re: [PATCH] diff-lib: ignore all outsider if --relative asked","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-19T16:58:16Z","receivedAt":"2021-08-19T16:58:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas <carenas@gmail.com> writes:\n\n> My only concern is that it seems this has been broken for a while\n> (couldn't bisect, but AFAIK the first implementation did filter like\n> this one does), so some people might be expecting the broken\n> behaviour.\n\nI do not think it is likely.\n\nIt only shows changes to unmerged paths outside the current\ndirectory, which does not sound all that useful.\n"}]}