* 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-21 4:03 ` [PATCH v2] " Đ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 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
* 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-21 4:03 ` [PATCH v2] " Đ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
* [PATCH v2] diff-lib: ignore all outsider if --relative asked @ 2021-08-21 4:03 ` Đoàn Trần Công Danh 2021-08-22 8:49 ` [PATCH v3] " Đoàn Trần Công Danh 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-21 4:03 ` [PATCH v2] " Đoàn Trần Công Danh @ 2021-08-22 8:49 ` Đoàn Trần Công Danh 0 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-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-21 4:03 ` [PATCH v2] " Đoàn Trần Công Danh 2021-08-22 8:49 ` [PATCH v3] " Đoàn Trần Công Danh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox