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 <thomas@slight.dev> wrote:

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 -----

On 2021-08-18 17:44:59+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

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

---- 8< ----

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

Looks sensible.  

The bug is the result of a bad interaction between cd676a51 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) that decided to return NULL from diff_unmerge for an irrelevant path outside the --relative area, and 095ce953 (diff-files: show unmerged entries correctly, 2011-04-22) that assumed diff_unmerge() would always yield a usable unmerged pair.

As the NULL pair would never be added to the diff_queue(), when the big loop around this area of code finishes and hands the processing off to diffcore_std(), the mode futzing the original code was attempting to do would have affected absolutely nothing, so I do not think there would be any subtle breakage coming from this fix.

When you submit an "git am"-able patch, please have my Acked-by.

Thanks.

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 <thomas@slight.dev>
Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---

Cc: Junio C Hamano <gitster@pobox.com>

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(+)

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

Carlo Arenas <carenas@gmail.com> writes:

I do not think it is likely.

It only shows changes to unmerged paths outside the current directory, which does not sound all that useful.

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

Nicely done.

If we look at cd676a51 (diff --relative: output paths as relative to the current subdirectory, 2008-02-12) where the "--relative" feature was introduced a bit more carefully, we notice that it wanted to implement "anything outside the .prefix gets discarded" at diff_addremove(), diff_change(), and diff_unmerge() level, instead of the side that enumerates the paths and calls these helpers, and that way, the "--relative" feature would consistently work across diff-files, diff-tree, and diff-index, as they all share these three helpers.

But filtering upfront before the codepath even has to decide if it needs to call diff_addremove() or diff_change(), like this patch does, makes sense, especially in the context of diff-files where the enumeration of paths is just to walk a single flat array that is the in-core index.

The proposed log message needs a bit more work, though. It would be an 80% OK explanation if the "check diff_unmerge()'s return value" approach was sufficient to correct bugs and we took the approach, but that is not the case. As you found out, it is not sufficient, and it is not the approach you took. The only part in the proposed log that explains the approach that was actually taken was "we can do better by ...".

Until/unless we do similar "filter with diffopt.prefix upfront" in diff-index and diff-tree codepaths, we unfortunately cannot lose the filter added to diff_addremove() and diff_change(), but I think this is a good first step towards such a longer-term clean-up.

Thanks.