{"thread":{"id":"60804","subject":"Git segfaults with diff.external and comparing files with different permissions","startedAt":"2024-01-28T20:24:52Z","lastAt":"2024-01-30T16:29:40Z","messageCount":5,"participants":["Wilfred Hughes","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"487478","messageId":"CAFXAjY7XcL1APhLRXU8TO96z=f7957f2ieK56dHVsXUay55vpg@mail.gmail.com","threadId":"60804","inReplyTo":null,"subject":"Git segfaults with diff.external and comparing files with different permissions","fromName":"Wilfred Hughes","fromEmail":"me@wilfred.me.uk","sentAt":"2024-01-28T20:24:39Z","receivedAt":"2024-01-28T20:24:52Z","isPatch":false,"sender":{"key":"me@wilfred.me.uk","avatar":null},"body":"Hi folks\n\nIt looks like git crashes if diff.external is set and the user\ncompares files that have different permissions. Here's a repro:\n\n$ mkdir demo\n$ cd demo\n$ git init .\nInitialized empty Git repository in /tmp/demo/.git/\n\n$ git config diff.external /bin/echo\n$ touch foo bar\n$ chmod 755 foo\n$ git diff --no-ext-diff --no-index foo bar\ndiff --git 1/foo 2/bar\nold mode 100755\nnew mode 100644\n\n$ git diff --no-index foo bar\nzsh: segmentation fault (core dumped)  git diff --no-index foo bar\n\nThis was originally reported as a difftastic bug[1] but it seems to\noccur regardless of the value of diff.external, hence my repro with\n/bin/echo.\n\n[1] https://github.com/Wilfred/difftastic/issues/615\n\n[System Info]\ngit version:\ngit version 2.43.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 6.7.0-zen3-1-zen #1 ZEN SMP PREEMPT_DYNAMIC Sat, 13 Jan\n2024 14:36:54 +0000 x86_64\ncompiler info: gnuc: 13.2\nlibc info: glibc: 2.38\n$SHELL (typically, interactive shell): /bin/zsh\n\nThanks\nWilfred\n"},{"id":"487490","messageId":"20240129015708.GA1762343@coredump.intra.peff.net","threadId":"60804","inReplyTo":"CAFXAjY7XcL1APhLRXU8TO96z=f7957f2ieK56dHVsXUay55vpg@mail.gmail.com","subject":"[PATCH] diff: handle NULL meta-info when spawning external diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-29T01:57:08Z","receivedAt":"2024-01-29T01:57:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 28, 2024 at 12:24:39PM -0800, Wilfred Hughes wrote:\n\n> It looks like git crashes if diff.external is set and the user\n> compares files that have different permissions. Here's a repro:\n> \n> $ mkdir demo\n> $ cd demo\n> $ git init .\n> Initialized empty Git repository in /tmp/demo/.git/\n> \n> $ git config diff.external /bin/echo\n> $ touch foo bar\n> $ chmod 755 foo\n> $ git diff --no-ext-diff --no-index foo bar\n> diff --git 1/foo 2/bar\n> old mode 100755\n> new mode 100644\n> \n> $ git diff --no-index foo bar\n> zsh: segmentation fault (core dumped)  git diff --no-index foo bar\n\nThanks for providing a simple reproduction recipe. There's a pretty\nstraight-forward fix below, though it leaves open some question of\nwhether there's another bug lurking with --no-index (but either way, I\nthink we'd want this simple fix as a first step).\n\n-- >8 --\nSubject: diff: handle NULL meta-info when spawning external diff\n\nRunning this:\n\n  $ touch foo bar\n  $ chmod +x foo\n  $ git -c diff.external=echo diff --ext-diff --no-index foo bar\n\nresults in a segfault. The issue is that run_diff_cmd() passes a NULL\n\"xfrm_msg\" variable to run_external_diff(), which feeds it to\nstrvec_push(), causing the segfault. The bug dates back to 82fbf269b9\n(run_external_diff: use an argv_array for the command line, 2014-04-19),\nthough it mostly only ever worked accidentally.  Before then, we just\nstuck the NULL pointer into a \"const char **\" array, so our NULL ended\nup acting as an extra end-of-argv sentinel (which was OK, because it was\nthe last thing in the array).\n\nCuriously, though, this is only a problem with --no-index. We set up\nxfrm_msg by calling fill_metainfo(). This result may be empty, or may\nhave text like \"index 1234..5678\\n\", \"rename from foo\\nrename from\nbar\\n\", etc. In run_external_diff(), we only look at xfrm_msg if the\n\"other\" variable is not NULL. That variable is set when the paths of the\ntwo sides of the diff pair aren't the same (in which case the\ndestination path becomes \"other\"). So normally it would kick in only for\na rename, in which case xfrm_msg should not be NULL (it would have the\nrename information in it).\n\nBut with a \"--no-index\" of two blobs, we of course have two different\npathnames, and thus end up with a non-NULL \"other\" filename (which is\nalways just a repeat of the file2-name), but possibly a NULL xfrm_msg.\n\nSo how to fix it? I have a feeling that --no-index always passing\n\"other\" to the external diff command is probably a bug. There was no\nrename, and the name is always redundant with existing information we\npass (and this may even cause us to pass a useless \"xfrm_msg\" that\ncontains an \"index 1234..5678\" line). So one option would be to change\nthat behavior. We don't seem to have ever documented the \"other\" or\n\"xfrm_msg\" parameters for external diffs.\n\nBut I'm not sure what fallout we might have from changing that behavior\nnow. So this patch takes the less-risky option, and simply teaches\nrun_external_diff() to avoid passing xfrm_msg when it's NULL. That makes\nit agnostic to whether \"other\" and \"xfrm_msg\" always come as a pair. It\nfixes the segfault now, and if we want to change the --no-index \"other\"\nbehavior on top, it will handle that, too.\n\nReported-by: Wilfred Hughes <me@wilfred.me.uk>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c                   |  3 ++-\n t/t4053-diff-no-index.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex a89a6a6128..ccfa1fca0d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4384,7 +4384,8 @@ static void run_external_diff(const char *pgm,\n \t\tadd_external_diff_name(o->repo, &cmd.args, two);\n \t\tif (other) {\n \t\t\tstrvec_push(&cmd.args, other);\n-\t\t\tstrvec_push(&cmd.args, xfrm_msg);\n+\t\t\tif (xfrm_msg)\n+\t\t\t\tstrvec_push(&cmd.args, xfrm_msg);\n \t\t}\n \t}\n \ndiff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh\nindex 5ce345d309..651ec77660 100755\n--- a/t/t4053-diff-no-index.sh\n+++ b/t/t4053-diff-no-index.sh\n@@ -205,6 +205,18 @@ test_expect_success POSIXPERM,SYMLINKS 'diff --no-index normalizes: mode not lik\n \ttest_cmp expected actual\n '\n \n+test_expect_success POSIXPERM 'external diff with mode-only change' '\n+\techo content >not-executable &&\n+\techo content >executable &&\n+\tchmod +x executable &&\n+\techo executable executable $(test_oid zero) 100755 \\\n+\t\tnot-executable $(test_oid zero) 100644 not-executable \\\n+\t\t>expect &&\n+\ttest_expect_code 1 git -c diff.external=echo diff \\\n+\t\t--no-index executable not-executable >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success \"diff --no-index treats '-' as stdin\" '\n \tcat >expect <<-EOF &&\n \tdiff --git a/- b/a/1\n-- \n2.43.0.794.g6ebf4d0024\n\n"},{"id":"487539","messageId":"xmqqede0htp2.fsf@gitster.g","threadId":"60804","inReplyTo":"20240129015708.GA1762343@coredump.intra.peff.net","subject":"Re: [PATCH] diff: handle NULL meta-info when spawning external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T18:37:29Z","receivedAt":"2024-01-29T18:37:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> $ git diff --no-index foo bar\n>> zsh: segmentation fault (core dumped)  git diff --no-index foo bar\n>\n> Thanks for providing a simple reproduction recipe. There's a pretty\n> straight-forward fix below, though it leaves open some question of\n> whether there's another bug lurking with --no-index (but either way, I\n> think we'd want this simple fix as a first step).\n\nYup, I agree with you that the \"--no-index\" mode violates the basic\ndesign that \"the other path\" and \"xfrm_msg\" go hand-in-hand.  In its\ntwo tree comparison mode \"git diff --no-index A/ B/\", it should be\nable to behave sensibly, but in its two files comparison mode to\ncompare plain regular files 'foo' and 'bar', there is nothing it can\ndo reasonably, I am afraid.  You could say that the change is\nrenaming 'foo' to create 'bar', and feed consistent data that is\naligned with that rename to external diff, which might be slightly\nmore logical than showing a change to 'foo' that has no rename\ninvolved (i.e. omitting \"other name\"), but neither is satisfying.\n\n> But I'm not sure what fallout we might have from changing that behavior\n> now. So this patch takes the less-risky option, and simply teaches\n> run_external_diff() to avoid passing xfrm_msg when it's NULL. That makes\n> it agnostic to whether \"other\" and \"xfrm_msg\" always come as a pair. It\n> fixes the segfault now, and if we want to change the --no-index \"other\"\n> behavior on top, it will handle that, too.\n\nSounds sensible.\n\nThanks.  Will queue.\n"},{"id":"487588","messageId":"20240130060658.GD166761@coredump.intra.peff.net","threadId":"60804","inReplyTo":"xmqqede0htp2.fsf@gitster.g","subject":"Re: [PATCH] diff: handle NULL meta-info when spawning external diff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-30T06:06:58Z","receivedAt":"2024-01-30T06:07:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 29, 2024 at 10:37:29AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> $ git diff --no-index foo bar\n> >> zsh: segmentation fault (core dumped)  git diff --no-index foo bar\n> >\n> > Thanks for providing a simple reproduction recipe. There's a pretty\n> > straight-forward fix below, though it leaves open some question of\n> > whether there's another bug lurking with --no-index (but either way, I\n> > think we'd want this simple fix as a first step).\n> \n> Yup, I agree with you that the \"--no-index\" mode violates the basic\n> design that \"the other path\" and \"xfrm_msg\" go hand-in-hand.  In its\n> two tree comparison mode \"git diff --no-index A/ B/\", it should be\n> able to behave sensibly, but in its two files comparison mode to\n> compare plain regular files 'foo' and 'bar', there is nothing it can\n> do reasonably, I am afraid.  You could say that the change is\n> renaming 'foo' to create 'bar', and feed consistent data that is\n> aligned with that rename to external diff, which might be slightly\n> more logical than showing a change to 'foo' that has no rename\n> involved (i.e. omitting \"other name\"), but neither is satisfying.\n\nYeah, I think the two-tree mode does behave correctly, and this is\nreally just about the two-blob mode. I agree that one could think of it\nas a rename or not, depending on how much you want to read into the\nimportance of the names (after all, you could compare a/foo and b/foo,\nwhich is sort of a moral equivalent of the usual two-tree case).\n\nThe current behavior is somewhere in between, though. You get an \"other\"\nname passed to the external diff, but the metainfo argument makes no\nmention of a rename (it's either blank for an exact rename, or may\ncontain an \"index\" line if there was a content change).\n\nI'm not sure anybody really cares that much either way, though. It's\nexternal diff, which I suspect hardly anybody uses, and those extra\nfields aren't even documented in the first place.\n\n-Peff\n"},{"id":"487600","messageId":"xmqqa5omdbte.fsf@gitster.g","threadId":"60804","inReplyTo":"20240130060658.GD166761@coredump.intra.peff.net","subject":"Re: [PATCH] diff: handle NULL meta-info when spawning external diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-30T16:29:33Z","receivedAt":"2024-01-30T16:29:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The current behavior is somewhere in between, though. You get an \"other\"\n> name passed to the external diff, but the metainfo argument makes no\n> mention of a rename (it's either blank for an exact rename, or may\n> contain an \"index\" line if there was a content change).\n>\n> I'm not sure anybody really cares that much either way, though. It's\n> external diff, which I suspect hardly anybody uses, and those extra\n> fields aren't even documented in the first place.\n\nOh, we probably should fix the documentation eventually, then.\n\nBut I agree that in this case, whatever stops the segfault would be\ngood enough.\n\nI am surprised to learn that this 8th hidden parameter dates back to\n427dcb4b ([PATCH] Diff overhaul, adding half of copy detection.,\n2005-05-21), and it is more surprising that even before it happened,\nthe external diff interface with 7 parameters was already\ndocumented, which happened with 03ea2802 ([PATCH 2/2] core-git\ndocumentation update, 2005-05-08).  Before the addition of the copy\ndetection, the presence of the \"other\" was how you learned if we saw\na rename (because there was no copy, the only reason \"other\" is\nthere was due to a rename).  With copy detection added, extra bits\nof information needed to be passed and we started passing the\nxfrm_msg as well through the interface.  At least, by dumping it to\nthe end-user, an external diff driver could help the end-user tell\nif that \"other\" came from a rename or from a copy, even if it did\nnot understand it itself.\n\nAnd of course, after merely 6 weeks since the inception, Git did not\nhave the \"--no-index\" mode (we did not even have a unified \"git\ndiff\" frontend), so this was never a problem back then.\n"}]}