`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@xxxxxxxxxxxxx>
---
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
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
