threads / patch / 63925

patchdiff-no-index: fix stdin path in subdirectory

Subject: [PATCH] diff-no-index: fix stdin path in subdirectory

## tl;dr

5 messages between Aug 7, 2025 and Aug 10, 2025. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Gregoire Geis· Aug 7, 2025, 15:06 UTC · lore

`git diff --no-index` tried to normalize paths before printing them, skipping a prefix before reading a string. However, `-` doesn't start with this prefix, leading to a buffer overflow:

$ make SANITIZE=address
$ mkdir a && cd a
$ echo a | ../git diff --no-index /dev/null -
=================================================================
==14536==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60b000001f92 at pc 0x000102612a30 bp 0x00016db386d0 sp 0x00016db386c8
READ of size 1 at 0x60b000001f92 thread T0
    #0 0x102612a2c in diff_flush_patch diff.c:6154
    #1 0x10260dbfc in diff_flush diff.c:6829
    #2 0x1025f218c in diff_no_index diff-no-index.c:422
    #3 0x102356338 in cmd_diff diff.c:501
    #4 0x1022cc780 in run_builtin git.c:480
    #5 0x1022c9bec in handle_builtin git.c:746
    #6 0x1022c8670 in cmd_main git.c:953
    #7 0x1024fe230 in main common-main.c:9
Signed-off-by: Gregoire Geis <opensource@gregoirege.is>
---
Hello!

I noticed that running `git diff --no-index /dev/null -` in a subdirectory leads to garbage file names (instead of `a/-` and `b/-`).

Valgrind and ASAN confirmed that this is a heap buffer overflow. I fixed it by checking against "-" when writing the file names in the patch, which leads to results similar to using an absolute path (e.g. `a/dev/stdin`, `a/-`, no matter the working directory). "-" is a special input for `git diff --no-index`, so I don't think we need any kind of normalization before this check.

I added a test which currently fails on `master`. I chose a subdirectory `aa/bb/cc` to make sure that the path is long enough for garbage to appear even if git is compiled without ASAN.

 diff.c                        |  4 ++--
 t/meson.build                 |  1 +
 t/t4072-diff-no-index-dash.sh | 17 +++++++++++++++++
 3 files changed, 20 insertions(+), 2 deletions(-)
 create mode 100755 t/t4072-diff-no-index-dash.sh
Show changes to 3 files +20 −2

diff.c, t/meson.build, t/t4072-diff-no-index-dash.sh

diff --git a/diff.c b/diff.c
index dca87e1..88e93e5 100644
--- a/diff.c
+++ b/diff.c
@@ -4626,12 +4626,12 @@ static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *is
 static void strip_prefix(int prefix_length, const char **namep, const char **otherp)
 {
 	/* Strip the prefix but do not molest /dev/null and absolute paths */
-	if (*namep && !is_absolute_path(*namep)) {
+	if (*namep && !is_absolute_path(*namep) && strcmp(*namep, "-")) {
 		*namep += prefix_length;
 		if (**namep == '/')
 			++*namep;
 	}
-	if (*otherp && !is_absolute_path(*otherp)) {
+	if (*otherp && !is_absolute_path(*otherp) && strcmp(*otherp, "-")) {
 		*otherp += prefix_length;
 		if (**otherp == '/')
 			++*otherp;
diff --git a/t/meson.build b/t/meson.build
index bbeba1a..9042f3f 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -486,6 +486,7 @@ integration_tests = [
   't4069-remerge-diff.sh',
   't4070-diff-pairs.sh',
   't4071-diff-minimal.sh',
+  't4072-diff-no-index-dash.sh',
   't4100-apply-stat.sh',
   't4101-apply-nonl.sh',
   't4102-apply-rename.sh',
diff --git a/t/t4072-diff-no-index-dash.sh b/t/t4072-diff-no-index-dash.sh
new file mode 100755
index 0000000..35170a4
--- /dev/null
+++ b/t/t4072-diff-no-index-dash.sh
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description='`git diff file -` does not crash'
+
+. ./test-lib.sh
+
+test_expect_success '`git diff --no-index /dev/null -` works in root' '
+  echo foo | git diff --no-index /dev/null - | grep "a/- b/-"
+'
+
+test_expect_success '`git diff --no-index /dev/null -` works in subdirectory' '
+  mkdir -p aa/bb/cc &&
+  cd aa/bb/cc &&
+  echo foo | git diff --no-index /dev/null - | grep "a/- b/-"
+'
+
+test_done

base-commit: 64cbe5e2e8a7b0f92c780b210e602496bd5cad0f

-- 
2.48.1
Junio C Hamano· Aug 7, 2025, 18:36 UTC · re: Gregoire Geis · lore

Re: [PATCH] diff-no-index: fix stdin path in subdirectory

Gregoire Geis <opensource@gregoirege.is> writes:
> `git diff --no-index` tried to normalize paths before printing them,
> skipping a prefix before reading a string. However, `-` doesn't start
> with this prefix, leading to a buffer overflow:
Thanks for a report, but ...
> I noticed that running `git diff --no-index /dev/null -` in a
> subdirectory leads to garbage file names (instead of `a/-` and `b/-`).

... there fundamentally is something wrong here. Why does the code even need to "strip" the "prefix" in the first place? Once the user said "git diff --no-index", there is no concept of "subdirectory" anymore. The user just told Git to pretend as if it is operating outside any Git-controlled working tree, so whereever you are is your current directory, and you would never be "in a subdirectory" of anything to begin with. But apparently diff_no_index() code is pretending as if it is part of Git proper and doing the prepend and strip the prefix dance.

Would something more direct like the attached patch work?
--- >8 ---
Subject: diff: --no-index should ignore the worktree

The act of giving "--no-index" tells Git to pretend that the current directory is not under control of any Git index or repository, so even when you happen to be in a Git controlled working tree, where in that working tree should not matter.

But the start-up sequence tries to discover the top of the working tree and chdir(2)'s there, even before Git passes control to the subcommand being run. When diff_no_index() starts running, it starts at a wrong (from the end-user's point of view who thinks "git diff --no-index" is merely a better version of GNU diff) directory, and the original directory the user started the command is at "prefix".

Because the paths given from argv[] have already been adjusted to account for this path shuffling by prepending the prefix, and showing the resulting path by stripping the prefix, the effect of these nonsense operations (nonsense in the context of "--no-index", that is) is usually not observable.

Except for special cases like "-", where it is not preprocessed by prepending the prefix.

Instead of papering over by adding more special cases only to cater to the no-index codepath in the generic code, drive the diff machinery more faithfully to what is going on. If the user started "git diff --no-index" in directory X/Y/Z in a working tree controlled by Git, and the start up sequence of Git chdir(2)'ed up to directory X and left Y/Z in the prefix, revert the effect of the start up sequence by chdir'ing back to Y/Z and emptying the prefix.

Reported-by: Gregoire Geis <opensource@gregoirege.is>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * After we worked on our own "diff" enhancements like rename
   detecion, colored output, etc., we thought it may make the world
   a better place if we gave non-Git using people the benefit of our
   "diff" implementation.  The correct thing to back then do may
   have been to give patches to integrate our extended diff code to
   diff implementations of other people (like GNU), but we took the
   lazy route to bolt the code to slurp two sets of paths to compare
   out of filesystem files on to our own diff implementation.  After
   fixing many fallouts from this impedance-mismatched code, it is
   not surprising that a bug like this still remained.
 builtin/diff.c           | 14 ++++++++++++++
 t/t4053-diff-no-index.sh | 17 +++++++++++++++++
 2 files changed, 31 insertions(+)
Show changes to diff +31 −0
diff --git c/builtin/diff.c w/builtin/diff.c
index 9a89e25a98..3eb4cbb057 100644
--- c/builtin/diff.c
+++ w/builtin/diff.c
@@ -487,6 +487,20 @@ int cmd_diff(int argc,
 
 	init_diff_ui_defaults();
 	repo_config(the_repository, git_diff_ui_config, NULL);
+
+	/*
+	 * If we are ignoring the fact that our current directory may
+	 * be part of a working tree controlled by a Git repository to
+	 * pretend to be a "better GNU diff", we should undo the
+	 * effect of the setup code that did a chdir() to the top of
+	 * the working tree.  Where we came from is recorded in the
+	 * prefix.
+	 */
+	if (no_index && prefix) {
+		chdir(prefix);
+		prefix = NULL;
+	}
+
 	prefix = precompose_argv_prefix(argc, argv, prefix);
 
 	repo_init_revisions(the_repository, &rev, prefix);
diff --git c/t/t4053-diff-no-index.sh w/t/t4053-diff-no-index.sh
index 01db9243ab..44b4b13f5d 100755
--- c/t/t4053-diff-no-index.sh
+++ w/t/t4053-diff-no-index.sh
@@ -26,6 +26,23 @@ test_expect_success 'git diff --no-index directories' '
 	test_line_count = 14 cnt
 '
 
+test_expect_success 'git diff --no-index with -' '
+	cat >expect <<-\EOF &&
+	diff --git a/- b/-
+	new file mode 100644
+	--- /dev/null
+	+++ b/-
+	@@ -0,0 +1 @@
+	+frotz
+	EOF
+	(
+		cd a &&
+		echo frotz |
+		test_expect_code 1 git diff --no-index /dev/null - >../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git diff --no-index relative path outside repo' '
 	(
 		cd repo &&
opensource@gregoirege.is· Aug 8, 2025, 11:25 UTC · re: Junio C Hamano · lore

Re: [PATCH] diff-no-index: fix stdin path in subdirectory

Junio C Hamano <gitster@pobox.com> wrote:
> ... there fundamentally is something wrong here.

Thank you for the thorough response and for explaining the reasoning / implementation of the more correct patch.

> Would something more direct like the attached patch work?

I am not familiar with the inner workings of Git at all, but I cannot think of any problem with this approach. This works for me!

Junio C Hamano· Aug 8, 2025, 16:09 UTC · re: opensource@gregoirege.is · lore

Re: [PATCH] diff-no-index: fix stdin path in subdirectory

opensource@gregoirege.is writes:
Show 11 quoted lines
> Junio C Hamano <gitster@pobox.com> wrote:
>
>> ... there fundamentally is something wrong here.
>
> Thank you for the thorough response and for explaining the
> reasoning / implementation of the more correct patch.
>
>> Would something more direct like the attached patch work?
>
> I am not familiar with the inner workings of Git at all, but I
> cannot think of any problem with this approach. This works for me!

Ah, what I meant was that I did not test the patch, so I did not know if it solves the problem you observed in your environment and with your development tools, hence I was asking you to apply it and test, like you did when you originally noticed the problem ;-)

Thanks.
opensource@gregoirege.is· Aug 9, 2025, 02:12 UTC · re: Junio C Hamano · lore

Re: [PATCH] diff-no-index: fix stdin path in subdirectory

Junio C Hamano <gitster@pobox.com> wrote:
> Ah, what I meant was that I did not test the patch, so I did not
> know if it solves the problem you observed in your environment and
> with your development tools, hence I was asking you to apply it and
> test, like you did when you originally noticed the problem ;-)

Ah, I definitely should have been more explicit, sorry. I did test your patch, and it did work where git previously encountered a buffer overflow.

I didn't try a lot of inputs, but different combinations of `-`, `/dev/null` and files in parent / subdirectories yield the same outputs as before, and additionally don't trigger any buffer overflow with ASAN.

`prove --shuffle -j8 t*-diff*.sh` also succeeded with your patch, whereas `t4053-diff-no-index.sh` fails on current `master`. I did skip `t4056-diff-order.sh` as it never terminates on my computer both with and without your patch.

← back to recent threads