{"thread":{"id":"63925","subject":"[PATCH] diff-no-index: fix stdin path in subdirectory","startedAt":"2025-08-07T15:06:59Z","lastAt":"2025-08-10T01:24:05Z","messageCount":5,"participants":["Gregoire Geis","Junio C Hamano","opensource@gregoirege.is"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"523757","messageId":"20250807150613.32177-1-opensource@gregoirege.is","threadId":"63925","inReplyTo":null,"subject":"[PATCH] diff-no-index: fix stdin path in subdirectory","fromName":"Gregoire Geis","fromEmail":"opensource@gregoirege.is","sentAt":"2025-08-07T15:06:13Z","receivedAt":"2025-08-07T15:06:59Z","isPatch":true,"sender":{"key":"opensource@gregoirege.is","avatar":null},"body":"`git diff --no-index` tried to normalize paths before printing them,\nskipping a prefix before reading a string. However, `-` doesn't start\nwith this prefix, leading to a buffer overflow:\n\n$ make SANITIZE=address\n$ mkdir a && cd a\n$ echo a | ../git diff --no-index /dev/null -\n=================================================================\n==14536==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60b000001f92 at pc 0x000102612a30 bp 0x00016db386d0 sp 0x00016db386c8\nREAD of size 1 at 0x60b000001f92 thread T0\n    #0 0x102612a2c in diff_flush_patch diff.c:6154\n    #1 0x10260dbfc in diff_flush diff.c:6829\n    #2 0x1025f218c in diff_no_index diff-no-index.c:422\n    #3 0x102356338 in cmd_diff diff.c:501\n    #4 0x1022cc780 in run_builtin git.c:480\n    #5 0x1022c9bec in handle_builtin git.c:746\n    #6 0x1022c8670 in cmd_main git.c:953\n    #7 0x1024fe230 in main common-main.c:9\n\nSigned-off-by: Gregoire Geis <opensource@gregoirege.is>\n---\nHello!\n\nI noticed that running `git diff --no-index /dev/null -` in a\nsubdirectory leads to garbage file names (instead of `a/-` and `b/-`).\n\nValgrind and ASAN confirmed that this is a heap buffer overflow. I\nfixed it by checking against \"-\" when writing the file names in the\npatch, which leads to results similar to using an absolute path (e.g.\n`a/dev/stdin`, `a/-`, no matter the working directory). \"-\" is a\nspecial input for `git diff --no-index`, so I don't think we need any\nkind of normalization before this check.\n\nI added a test which currently fails on `master`. I chose a\nsubdirectory `aa/bb/cc` to make sure that the path is long enough for\ngarbage to appear even if git is compiled without ASAN.\n\n diff.c                        |  4 ++--\n t/meson.build                 |  1 +\n t/t4072-diff-no-index-dash.sh | 17 +++++++++++++++++\n 3 files changed, 20 insertions(+), 2 deletions(-)\n create mode 100755 t/t4072-diff-no-index-dash.sh\n\ndiff --git a/diff.c b/diff.c\nindex dca87e1..88e93e5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4626,12 +4626,12 @@ static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *is\n static void strip_prefix(int prefix_length, const char **namep, const char **otherp)\n {\n \t/* Strip the prefix but do not molest /dev/null and absolute paths */\n-\tif (*namep && !is_absolute_path(*namep)) {\n+\tif (*namep && !is_absolute_path(*namep) && strcmp(*namep, \"-\")) {\n \t\t*namep += prefix_length;\n \t\tif (**namep == '/')\n \t\t\t++*namep;\n \t}\n-\tif (*otherp && !is_absolute_path(*otherp)) {\n+\tif (*otherp && !is_absolute_path(*otherp) && strcmp(*otherp, \"-\")) {\n \t\t*otherp += prefix_length;\n \t\tif (**otherp == '/')\n \t\t\t++*otherp;\ndiff --git a/t/meson.build b/t/meson.build\nindex bbeba1a..9042f3f 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -486,6 +486,7 @@ integration_tests = [\n   't4069-remerge-diff.sh',\n   't4070-diff-pairs.sh',\n   't4071-diff-minimal.sh',\n+  't4072-diff-no-index-dash.sh',\n   't4100-apply-stat.sh',\n   't4101-apply-nonl.sh',\n   't4102-apply-rename.sh',\ndiff --git a/t/t4072-diff-no-index-dash.sh b/t/t4072-diff-no-index-dash.sh\nnew file mode 100755\nindex 0000000..35170a4\n--- /dev/null\n+++ b/t/t4072-diff-no-index-dash.sh\n@@ -0,0 +1,17 @@\n+#!/bin/sh\n+\n+test_description='`git diff file -` does not crash'\n+\n+. ./test-lib.sh\n+\n+test_expect_success '`git diff --no-index /dev/null -` works in root' '\n+  echo foo | git diff --no-index /dev/null - | grep \"a/- b/-\"\n+'\n+\n+test_expect_success '`git diff --no-index /dev/null -` works in subdirectory' '\n+  mkdir -p aa/bb/cc &&\n+  cd aa/bb/cc &&\n+  echo foo | git diff --no-index /dev/null - | grep \"a/- b/-\"\n+'\n+\n+test_done\n\nbase-commit: 64cbe5e2e8a7b0f92c780b210e602496bd5cad0f\n-- \n2.48.1\n\n"},{"id":"523767","messageId":"xmqqtt2jm118.fsf@gitster.g","threadId":"63925","inReplyTo":"20250807150613.32177-1-opensource@gregoirege.is","subject":"Re: [PATCH] diff-no-index: fix stdin path in subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-07T18:36:51Z","receivedAt":"2025-08-07T18:36:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gregoire Geis <opensource@gregoirege.is> writes:\n\n> `git diff --no-index` tried to normalize paths before printing them,\n> skipping a prefix before reading a string. However, `-` doesn't start\n> with this prefix, leading to a buffer overflow:\n\nThanks for a report, but ...\n\n> I noticed that running `git diff --no-index /dev/null -` in a\n> subdirectory leads to garbage file names (instead of `a/-` and `b/-`).\n\n... there fundamentally is something wrong here.  Why does the code\neven need to \"strip\" the \"prefix\" in the first place?  Once the user\nsaid \"git diff --no-index\", there is no concept of \"subdirectory\"\nanymore.  The user just told Git to pretend as if it is operating\noutside any Git-controlled working tree, so whereever you are is\nyour current directory, and you would never be \"in a subdirectory\"\nof anything to begin with.  But apparently diff_no_index() code is\npretending as if it is part of Git proper and doing the prepend and\nstrip the prefix dance.\n\nWould something more direct like the attached patch work?\n\n--- >8 ---\nSubject: diff: --no-index should ignore the worktree\n\nThe act of giving \"--no-index\" tells Git to pretend that the current\ndirectory is not under control of any Git index or repository, so\neven when you happen to be in a Git controlled working tree, where\nin that working tree should not matter.\n\nBut the start-up sequence tries to discover the top of the working\ntree and chdir(2)'s there, even before Git passes control to the\nsubcommand being run.  When diff_no_index() starts running, it\nstarts at a wrong (from the end-user's point of view who thinks \"git\ndiff --no-index\" is merely a better version of GNU diff) directory,\nand the original directory the user started the command is at\n\"prefix\".\n\nBecause the paths given from argv[] have already been adjusted to\naccount for this path shuffling by prepending the prefix, and\nshowing the resulting path by stripping the prefix, the effect of\nthese nonsense operations (nonsense in the context of \"--no-index\",\nthat is) is usually not observable.\n\nExcept for special cases like \"-\", where it is not preprocessed by\nprepending the prefix.\n\nInstead of papering over by adding more special cases only to cater\nto the no-index codepath in the generic code, drive the diff\nmachinery more faithfully to what is going on.  If the user started\n\"git diff --no-index\" in directory X/Y/Z in a working tree\ncontrolled by Git, and the start up sequence of Git chdir(2)'ed up\nto directory X and left Y/Z in the prefix, revert the effect of the\nstart up sequence by chdir'ing back to Y/Z and emptying the prefix.\n\nReported-by: Gregoire Geis <opensource@gregoirege.is>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * After we worked on our own \"diff\" enhancements like rename\n   detecion, colored output, etc., we thought it may make the world\n   a better place if we gave non-Git using people the benefit of our\n   \"diff\" implementation.  The correct thing to back then do may\n   have been to give patches to integrate our extended diff code to\n   diff implementations of other people (like GNU), but we took the\n   lazy route to bolt the code to slurp two sets of paths to compare\n   out of filesystem files on to our own diff implementation.  After\n   fixing many fallouts from this impedance-mismatched code, it is\n   not surprising that a bug like this still remained.\n\n builtin/diff.c           | 14 ++++++++++++++\n t/t4053-diff-no-index.sh | 17 +++++++++++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git c/builtin/diff.c w/builtin/diff.c\nindex 9a89e25a98..3eb4cbb057 100644\n--- c/builtin/diff.c\n+++ w/builtin/diff.c\n@@ -487,6 +487,20 @@ int cmd_diff(int argc,\n \n \tinit_diff_ui_defaults();\n \trepo_config(the_repository, git_diff_ui_config, NULL);\n+\n+\t/*\n+\t * If we are ignoring the fact that our current directory may\n+\t * be part of a working tree controlled by a Git repository to\n+\t * pretend to be a \"better GNU diff\", we should undo the\n+\t * effect of the setup code that did a chdir() to the top of\n+\t * the working tree.  Where we came from is recorded in the\n+\t * prefix.\n+\t */\n+\tif (no_index && prefix) {\n+\t\tchdir(prefix);\n+\t\tprefix = NULL;\n+\t}\n+\n \tprefix = precompose_argv_prefix(argc, argv, prefix);\n \n \trepo_init_revisions(the_repository, &rev, prefix);\ndiff --git c/t/t4053-diff-no-index.sh w/t/t4053-diff-no-index.sh\nindex 01db9243ab..44b4b13f5d 100755\n--- c/t/t4053-diff-no-index.sh\n+++ w/t/t4053-diff-no-index.sh\n@@ -26,6 +26,23 @@ test_expect_success 'git diff --no-index directories' '\n \ttest_line_count = 14 cnt\n '\n \n+test_expect_success 'git diff --no-index with -' '\n+\tcat >expect <<-\\EOF &&\n+\tdiff --git a/- b/-\n+\tnew file mode 100644\n+\t--- /dev/null\n+\t+++ b/-\n+\t@@ -0,0 +1 @@\n+\t+frotz\n+\tEOF\n+\t(\n+\t\tcd a &&\n+\t\techo frotz |\n+\t\ttest_expect_code 1 git diff --no-index /dev/null - >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git diff --no-index relative path outside repo' '\n \t(\n \t\tcd repo &&\n"},{"id":"523817","messageId":"CCF4B0E0-980D-472A-8689-7615FFFE0198@gregoirege.is","threadId":"63925","inReplyTo":"xmqqtt2jm118.fsf@gitster.g","subject":"Re: [PATCH] diff-no-index: fix stdin path in subdirectory","fromName":"","fromEmail":"opensource@gregoirege.is","sentAt":"2025-08-08T11:25:36Z","receivedAt":"2025-08-08T11:25:54Z","isPatch":true,"sender":{"key":"opensource@gregoirege.is","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n\n> ... there fundamentally is something wrong here.\n\nThank you for the thorough response and for explaining the\nreasoning / implementation of the more correct patch.\n\n> Would something more direct like the attached patch work?\n\nI am not familiar with the inner workings of Git at all, but I\ncannot think of any problem with this approach. This works for me!\n"},{"id":"523830","messageId":"xmqqjz3diymi.fsf@gitster.g","threadId":"63925","inReplyTo":"CCF4B0E0-980D-472A-8689-7615FFFE0198@gregoirege.is","subject":"Re: [PATCH] diff-no-index: fix stdin path in subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-08T16:09:25Z","receivedAt":"2025-08-08T16:09:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"opensource@gregoirege.is writes:\n\n> Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> ... there fundamentally is something wrong here.\n>\n> Thank you for the thorough response and for explaining the\n> reasoning / implementation of the more correct patch.\n>\n>> Would something more direct like the attached patch work?\n>\n> I am not familiar with the inner workings of Git at all, but I\n> cannot think of any problem with this approach. This works for me!\n\nAh, what I meant was that I did not test the patch, so I did not\nknow if it solves the problem you observed in your environment and\nwith your development tools, hence I was asking you to apply it and\ntest, like you did when you originally noticed the problem ;-)\n\nThanks.\n"},{"id":"523881","messageId":"7E0AF030-2C14-4121-A2AE-A419B46EA701@gregoirege.is","threadId":"63925","inReplyTo":"xmqqjz3diymi.fsf@gitster.g","subject":"Re: [PATCH] diff-no-index: fix stdin path in subdirectory","fromName":"","fromEmail":"opensource@gregoirege.is","sentAt":"2025-08-09T02:12:13Z","receivedAt":"2025-08-10T01:24:05Z","isPatch":true,"sender":{"key":"opensource@gregoirege.is","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n\n> Ah, what I meant was that I did not test the patch, so I did not\n> know if it solves the problem you observed in your environment and\n> with your development tools, hence I was asking you to apply it and\n> test, like you did when you originally noticed the problem ;-)\n\nAh, I definitely should have been more explicit, sorry. I did test\nyour patch, and it did work where git previously encountered a buffer\noverflow.\n\nI didn't try a lot of inputs, but different combinations of `-`,\n`/dev/null` and files in parent / subdirectories yield the same\noutputs as before, and additionally don't trigger any buffer overflow\nwith ASAN.\n\n`prove --shuffle -j8 t*-diff*.sh` also succeeded with your patch,\nwhereas `t4053-diff-no-index.sh` fails on current `master`. I did\nskip `t4056-diff-order.sh` as it never terminates on my computer\nboth with and without your patch.\n\n"}]}