Bug: Segmentation fault in git diff

Hello,

This is my first bug report to Git mailing list so let me know if more information is needed.

Running the following command results in a segmentation fault on macOS arm64
$ git diff --name-only --diff-filter=U —relative
Segmentation fault: 11

I was rebasing while it happened, trying to resolve merge conflicts. This is roughly how the status of the repo looked (I changed the file names, but keep them relative the to output, i.e. ../actual-file.txt I change to ../some-file.txt).

$ git status
interactive rebase in progress; onto a4aabaa
Last command done (1 command done):
   pick ad0e02e WIP
No commands remaining.
You are currently rebasing branch 'my_branch' on 'a4aabaa'.
  (fix conflicts and then run "git rebase --continue")
  (use "git rebase --skip" to skip this patch)
  (use "git rebase --abort" to check out the original branch)

Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
        modified:   ../dir1/Makefile
        modified:   ../dir1/http_spec.yaml
        modified:   ../dir1/tests/api/file1.rs
        modified:   file2.proto

Unmerged paths:
  (use "git restore --staged <file>..." to unstage)
  (use "git add <file>..." to mark resolution)
        both modified:   ../dir1/build.rs
        both modified:   ../dir1/tests/data/file3.sql
        both modified:   file4.go

Running it through lldb doesn’t give too much information, but just in case it helps:

$ lldb -- git diff --name-only --diff-filter=U --relative
(lldb) target create "git"
Current executable set to 'git' (arm64).
(lldb) settings set -- target.run-args  "diff" "--name-only" "--diff-filter=U" "--relative"
(lldb) r
Process 10619 launched: '/opt/homebrew/bin/git' (arm64)
Process 10619 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
    frame #0: 0x00000001000ea2ac git`run_diff_files + 864
git`run_diff_files:
->  0x1000ea2ac <+864>: ldr    x8, [x0, #0x8]
    0x1000ea2b0 <+868>: strh   w19, [x8, #0x50]
    0x1000ea2b4 <+872>: ldr    w8, [x28, #0x38]
    0x1000ea2b8 <+876>: ubfx   w8, w8, #12, #2
Target 0: (git) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
  * frame #0: 0x00000001000ea2ac git`run_diff_files + 864
    frame #1: 0x000000010002a65c git`cmd_diff + 1820
    frame #2: 0x00000001000043e4 git`run_builtin + 420
    frame #3: 0x0000000100003948 git`handle_builtin + 272
    frame #4: 0x000000010000322c git`cmd_main + 812
    frame #5: 0x00000001000a6ec4 git`main + 140
    frame #6: 0x00000001a19e9450 libdyld.dylib`start + 4

I got my Git binary from Homebrew, tried version 2.32.0 and 2.33.0 (currently the latest on Homebrew).

OS information:
macOS Big Sur Version 11.4
$ uname -a
Darwin MacBook-Pro.local 20.5.0 Darwin Kernel Version 20.5.0: Sat May  8 05:10:31 PDT 2021; root:xnu-7195.121.3~9/RELEASE_ARM64_T8101 arm64

—

Regards,

Thomas de Zeeuw
thomas@slight.dev

On 2021-08-18 10:42:45+0200, Thomas De Zeeuw [off-list ref] wrote:

Hello,

This is my first bug report to Git mailing list so let me know if more information is needed.

Running the following command results in a segmentation fault on macOS arm64
$ git diff --name-only --diff-filter=U —relative
Segmentation fault: 11

MVCE:

---- 8< ---
#!/bin/sh

rm -rf /tmp/diff-bug
git init /tmp/diff-bug
cd /tmp/diff-bug
mkdir -p dir

printf '%s\n' one two three >file
printf '%s\n' inner >dir/file
git add file dir/file
git commit -m first

git branch side

printf '%s\n' one two >file
git add file
git commit -m checkpoint
git tag checkpoint

git switch side
printf '%s\n' two two four >file
git add file
git commit -m side

cd dir
git rebase checkpoint

git diff --name-only --relative
---- >8 -----

It's NULL pointer dereference bug because pair is NULL.
I haven't check further:

---- 8< -----
#0  run_diff_files (revs=revs@entry=0x7ffcc85ae270, option=option@entry=0)
    at diff-lib.c:196
196                                     pair->two->mode = wt_mode;
----- >8 -----

-- 
Danh

On 2021-08-18 17:44:59+0700, Đoàn Trần Công Danh [off-list ref] wrote:

On 2021-08-18 10:42:45+0200, Thomas De Zeeuw [off-list ref] wrote:

quoted

Hello,

This is my first bug report to Git mailing list so let me know if more information is needed.

Running the following command results in a segmentation fault on macOS arm64
$ git diff --name-only --diff-filter=U —relative
Segmentation fault: 11

MVCE:

---- 8< ---
#!/bin/sh

rm -rf /tmp/diff-bug
git init /tmp/diff-bug
cd /tmp/diff-bug
mkdir -p dir

printf '%s\n' one two three >file
printf '%s\n' inner >dir/file
git add file dir/file
git commit -m first

git branch side

printf '%s\n' one two >file
git add file
git commit -m checkpoint
git tag checkpoint

git switch side
printf '%s\n' two two four >file
git add file
git commit -m side

cd dir
git rebase checkpoint

git diff --name-only --relative
---- >8 -----

It's NULL pointer dereference bug because pair is NULL.
I haven't check further:

---- 8< -----
#0  run_diff_files (revs=revs@entry=0x7ffcc85ae270, option=option@entry=0)
    at diff-lib.c:196
196                                     pair->two->mode = wt_mode;
----- >8 -----

This diff could fix the issue, and the test suite still passes:

---- 8< ----

diff --git a/diff-lib.c b/diff-lib.c
index f9eadc4fc1..8f303958dd 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -192,7 +192,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			 * from the desired stage.
 			 */
 			pair = diff_unmerge(&revs->diffopt, ce->name);
-			if (wt_mode)
+			if (pair && wt_mode)
 				pair->two->mode = wt_mode;
 			if (ce_stage(ce) != diff_unmerged_stage)
 				continue;
---- >8 -----

-- 
Danh

For diff family commands, we can tell them to exclude changes outside
of some directories if --relative is requested.

In diff_unmerge(), NULL will be returned if the requested path is
outside of the interesting directories, thus we'll run into NULL
pointer dereference in run_diff_files when trying to dereference
its return value.

We can simply check for NULL there before dereferencing said
return value.  However, we can do better by not running diff
on those unintesting entries.  Let's do that instead.

Reported-by: Thomas De Zeeuw <redacted>
Signed-off-by: Đoàn Trần Công Danh <redacted>
---

Cc: Junio C Hamano <redacted>

Notes:
    Check for return value of diff_unmerge is not enough.
    
    Yes, it works with --name-only, however, with only --relative,
    git-diff shows unmerged entries outside of subdir, too.
    
    Furthermore, the filename in "diff --cc" ignores the relative prefix.
    Fixing this requires touching all over places, at least from my study.
    Let's fix the crash, first.
    
    We have two choices here:
    
    * Check pair, aka return value of diff_unmerge, like my original
      suggestion, and the unmerged entries from outside will be shown, too.
      Some inconsistent will be observed, --name-only won't list files
      outside of subdir, while the patch shows them.  At least, it doesn't
      create false impression of no change outside of subdir.
    
    * Skip all outsiders, like this patch.
    
    While I prefer this approach, I don't know all ramifications of this change,
    let's say an entry moved to outside of subdir in one side, and modified in
    other side.
    
    Because, I pick the different approach, Junio's ack isn't included here.
    

 diff-lib.c               |  4 +++
 t/t4045-diff-relative.sh | 53 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index f9eadc4fc1..ca085a03ef 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -117,6 +117,10 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		if (!ce_path_match(istate, ce, &revs->prune_data, NULL))
 			continue;
 
+		if (revs->diffopt.prefix &&
+		    strncmp(ce->name, revs->diffopt.prefix, revs->diffopt.prefix_length))
+			continue;
+
 		if (ce_stage(ce)) {
 			struct combine_diff_path *dpath;
 			struct diff_filepair *pair;

diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 61ba5f707f..8cbbe53262 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh

@@ -162,4 +162,57 @@ check_diff_relative_option subdir file2 true --no-relative --relative
 check_diff_relative_option . file2 false --no-relative --relative=subdir
 check_diff_relative_option . file2 true --no-relative --relative=subdir
 
+test_expect_success 'setup diff --relative unmerged' '
+	test_commit zero file0 &&
+	test_commit base subdir/file0 &&
+	git switch -c br1 &&
+	test_commit one file0 &&
+	test_commit sub1 subdir/file0 &&
+	git switch -c br2 base &&
+	test_commit two file0 &&
+	git switch -c br3 &&
+	test_commit sub3 subdir/file0
+'
+
+test_expect_success 'diff --relative without change in subdir' '
+	git switch br2 &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge one &&
+	git -C subdir diff --relative >out &&
+	test_must_be_empty out &&
+	git -C subdir diff --relative --name-only >out &&
+	test_must_be_empty out
+'
+
+test_expect_success 'diff --relative --name-only with change in subdir' '
+	git switch br3 &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge sub1 &&
+	test_write_lines file0 file0 >expected &&
+	git -C subdir diff --relative --name-only >out &&
+	test_cmp expected out
+'
+
+test_expect_failure 'diff --relative with change in subdir' '
+	git switch br3 &&
+	br1_blob=$(git rev-parse --short --verify br1:subdir/file0) &&
+	br3_blob=$(git rev-parse --short --verify br3:subdir/file0) &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge br1 &&
+	cat >expected <<-EOF &&
+	diff --cc file0
+	index $br3_blob,$br1_blob..0000000
+	--- a/file0
+	+++ b/file0
+	@@@ -1,1 -1,1 +1,5 @@@
+	++<<<<<<< HEAD
+	 +sub3
+	++=======
+	+ sub1
+	++>>>>>>> sub1
+	EOF
+	git -C subdir diff --relative >out &&
+	test_cmp expected out
+'
+
 test_done

-- 
2.33.0.254.g68ee769121

Awesome work, and this not only fixes the crash but a bug in
--relative that has been going for a while, after all the
documentation clearly says that --relative should filter out entries
outside the tree and it does unless in this crashing scenario, which
as pointed out was also showing the wrong paths for the diff chunks
outside of the directory.

My only concern is that it seems this has been broken for a while
(couldn't bisect, but AFAIK the first implementation did filter like
this one does), so some people might be expecting the broken
behaviour.

got my Tested-by or Reviewed-by and gratitude.

Carlo