All of lore.kernel.org
 help / color / mirror / Atom feed
* Bug: Segmentation fault in git diff
@ 2021-08-18  8:42 ` Thomas De Zeeuw
  2021-08-18 10:44   ` Đoàn Trần Công Danh
  2021-08-19  8:29   ` [PATCH] diff-lib: ignore all outsider if --relative asked Đoàn Trần Công Danh
  0 siblings, 2 replies; 10+ messages in thread
From: Thomas De Zeeuw @ 2021-08-18  8:42 UTC (permalink / raw)
  To: git

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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Bug: Segmentation fault in git diff
  2021-08-18  8:42 ` Bug: Segmentation fault in git diff Thomas De Zeeuw
@ 2021-08-18 10:44   ` Đoàn Trần Công Danh
  2021-08-18 12:52     ` Đoàn Trần Công Danh
  2021-08-19  8:29   ` [PATCH] diff-lib: ignore all outsider if --relative asked Đoàn Trần Công Danh
  1 sibling, 1 reply; 10+ messages in thread
From: Đoàn Trần Công Danh @ 2021-08-18 10:44 UTC (permalink / raw)
  To: Thomas De Zeeuw; +Cc: git

On 2021-08-18 10:42:45+0200, Thomas De Zeeuw <thomas@slight.dev> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Bug: Segmentation fault in git diff
  2021-08-18 10:44   ` Đoàn Trần Công Danh
@ 2021-08-18 12:52     ` Đoàn Trần Công Danh
  2021-08-18 20:30       ` Junio C Hamano
  0 siblings, 1 reply; 10+ messages in thread
From: Đoàn Trần Công Danh @ 2021-08-18 12:52 UTC (permalink / raw)
  To: Thomas De Zeeuw; +Cc: git

On 2021-08-18 17:44:59+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> On 2021-08-18 10:42:45+0200, Thomas De Zeeuw <thomas@slight.dev> 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 -----

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: Bug: Segmentation fault in git diff
  2021-08-18 12:52     ` Đoàn Trần Công Danh
@ 2021-08-18 20:30       ` Junio C Hamano
  0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2021-08-18 20:30 UTC (permalink / raw)
  To: Đoàn Trần Công Danh; +Cc: Thomas De Zeeuw, git

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

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

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] diff-lib: ignore all outsider if --relative asked
  2021-08-18  8:42 ` Bug: Segmentation fault in git diff Thomas De Zeeuw
  2021-08-18 10:44   ` Đoàn Trần Công Danh
@ 2021-08-19  8:29   ` Đoàn Trần Công Danh
  2021-08-19  9:02     ` Carlo Arenas
                       ` (2 more replies)
  1 sibling, 3 replies; 10+ messages in thread
From: Đoàn Trần Công Danh @ 2021-08-19  8:29 UTC (permalink / raw)
  To: git
  Cc: Đoàn Trần Công Danh, Thomas De Zeeuw,
	Junio C Hamano

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

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] diff-lib: ignore all outsider if --relative asked
  2021-08-19  8:29   ` [PATCH] diff-lib: ignore all outsider if --relative asked Đoàn Trần Công Danh
@ 2021-08-19  9:02     ` Carlo Arenas
  2021-08-19 16:58       ` Junio C Hamano
  2021-08-19 16:55     ` Junio C Hamano
  2021-08-22  8:49     ` [PATCH v3] " Đoàn Trần Công Danh
  2 siblings, 1 reply; 10+ messages in thread
From: Carlo Arenas @ 2021-08-19  9:02 UTC (permalink / raw)
  To: Đoàn Trần Công Danh
  Cc: git, Thomas De Zeeuw, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] diff-lib: ignore all outsider if --relative asked
  2021-08-19  8:29   ` [PATCH] diff-lib: ignore all outsider if --relative asked Đoàn Trần Công Danh
  2021-08-19  9:02     ` Carlo Arenas
@ 2021-08-19 16:55     ` Junio C Hamano
  2021-08-22  8:49     ` [PATCH v3] " Đoàn Trần Công Danh
  2 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2021-08-19 16:55 UTC (permalink / raw)
  To: Đoàn Trần Công Danh; +Cc: git, Thomas De Zeeuw

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

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

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] diff-lib: ignore all outsider if --relative asked
  2021-08-19  9:02     ` Carlo Arenas
@ 2021-08-19 16:58       ` Junio C Hamano
  0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2021-08-19 16:58 UTC (permalink / raw)
  To: Carlo Arenas
  Cc: Đoàn Trần Công Danh, git, Thomas De Zeeuw

Carlo Arenas <carenas@gmail.com> writes:

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

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] diff-lib: ignore all outsider if --relative asked
@ 2021-08-21  4:03 Đoàn Trần Công Danh
  2021-08-18  8:42 ` Bug: Segmentation fault in git diff Thomas De Zeeuw
  0 siblings, 1 reply; 10+ messages in thread
From: Đoàn Trần Công Danh @ 2021-08-21  4:03 UTC (permalink / raw)
  To: git
  Cc: Đoàn Trần Công Danh, Junio C Hamano,
	Thomas De Zeeuw, Carlo Arenas

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.

Checking for return value of diff_unmerge before dereferencing
is not sufficient, though. Since, diff engine will try to work on such
pathspec later.

Let's not run diff on those unintesting entries, instead.
As a side effect, by skipping like that, we can save some CPU cycles.

Reported-by: Thomas De Zeeuw <thomas@slight.dev>
Tested-by: Carlo Arenas <carenas@gmail.com>
Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
Range-diff against v1:
1:  57a9edc3af ! 1:  0d73c71819 diff-lib: ignore all outsider if --relative asked
    @@ Commit message
         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.
    +    Checking for return value of diff_unmerge before dereferencing
    +    is not sufficient, though. Since, diff engine will try to work on such
    +    pathspec later.
    +
    +    Let's not run diff on those unintesting entries, instead.
    +    As a side effect, by skipping like that, we can save some CPU cycles.
     
         Reported-by: Thomas De Zeeuw <thomas@slight.dev>
    +    Tested-by: Carlo Arenas <carenas@gmail.com>
         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.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.
    -
    -    Cc: Junio C Hamano <gitster@pobox.com>
    -
      ## diff-lib.c ##
     @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)
      		if (!ce_path_match(istate, ce, &revs->prune_data, NULL))

 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3] diff-lib: ignore all outsider if --relative asked
  2021-08-19  8:29   ` [PATCH] diff-lib: ignore all outsider if --relative asked Đoàn Trần Công Danh
  2021-08-19  9:02     ` Carlo Arenas
  2021-08-19 16:55     ` Junio C Hamano
@ 2021-08-22  8:49     ` Đoàn Trần Công Danh
  2 siblings, 0 replies; 10+ messages in thread
From: Đoàn Trần Công Danh @ 2021-08-22  8:49 UTC (permalink / raw)
  To: git
  Cc: Đoàn Trần Công Danh, Junio C Hamano,
	Thomas De Zeeuw, Carlo Arenas

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.

Checking for return value of diff_unmerge before dereferencing
is not sufficient, though. Since, diff engine will try to work on such
pathspec later.

Let's not run diff on those unintesting entries, instead.
As a side effect, by skipping like that, we can save some CPU cycles.

Reported-by: Thomas De Zeeuw <thomas@slight.dev>
Tested-by: Carlo Arenas <carenas@gmail.com>
Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---

Sorry for the noise, I need to send v3, because:

Change in v3 from v2:
 Fix the conflict marker in failure test. Originally, I wrote the test with:
 "git merge sub1" not "git merge br1"

Range-diff against v2:
1:  0d73c71819 ! 1:  a140b292f9 diff-lib: ignore all outsider if --relative asked
    @@ t/t4045-diff-relative.sh: check_diff_relative_option subdir file2 true --no-rela
     +	 +sub3
     +	++=======
     +	+ sub1
    -+	++>>>>>>> sub1
    ++	++>>>>>>> br1
     +	EOF
     +	git -C subdir diff --relative >out &&
     +	test_cmp expected out

Range-diff of v2 against v1:
1:  57a9edc3af ! 1:  0d73c71819 diff-lib: ignore all outsider if --relative asked
    @@ Commit message
         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.
    +    Checking for return value of diff_unmerge before dereferencing
    +    is not sufficient, though. Since, diff engine will try to work on such
    +    pathspec later.
    +
    +    Let's not run diff on those unintesting entries, instead.
    +    As a side effect, by skipping like that, we can save some CPU cycles.
     
         Reported-by: Thomas De Zeeuw <thomas@slight.dev>
    +    Tested-by: Carlo Arenas <carenas@gmail.com>
         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
 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..fab351b48a 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
+	++>>>>>>> br1
+	EOF
+	git -C subdir diff --relative >out &&
+	test_cmp expected out
+'
+
 test_done
-- 
2.33.0.254.g68ee769121


^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-08-22  8:49 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-21  4:03 [PATCH v2] diff-lib: ignore all outsider if --relative asked Đoàn Trần Công Danh
2021-08-18  8:42 ` Bug: Segmentation fault in git diff Thomas De Zeeuw
2021-08-18 10:44   ` Đoàn Trần Công Danh
2021-08-18 12:52     ` Đoàn Trần Công Danh
2021-08-18 20:30       ` Junio C Hamano
2021-08-19  8:29   ` [PATCH] diff-lib: ignore all outsider if --relative asked Đoàn Trần Công Danh
2021-08-19  9:02     ` Carlo Arenas
2021-08-19 16:58       ` Junio C Hamano
2021-08-19 16:55     ` Junio C Hamano
2021-08-22  8:49     ` [PATCH v3] " Đoàn Trần Công Danh

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.