Gregoire Geis <opensource@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
* 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(+)
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 &&
