| Message ID | 5d4315c5-a0ae-2857-fbcc-ec6166d025b6@theori.io (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d5e35329dd5305d611478e8d5076a8ca75e25f0d |
| Headers | show |
| Series | [v5] diff: make diff_free_filespec_data accept NULL | expand |
Hi Jinoh, On Wed, 11 Nov 2020, Jinoh Kang wrote: > diff_free_filespec_data crashes when passed a NULL fillspec pointer. > Commit 3aef54e8b8 ("diff: munmap() file contents before running external > diff", 2019-07-11) introduced calls to diff_free_filespec_data in > run_external_diff without also checking if the argument is NULL. > > Git uses NULL filespecs to indicate unmerged files when merge conflict > resolution is in progress. Fortunately, other code paths bail out early > even before NULL can reach diff_free_filespec_data(); however, difftool > is expected to do a full-blown diff anyway regardless of conflict > status. > > Fix this and prevent any similar bugs in the future by making > `diff_free_filespec_data(NULL)` a no-op. > > Add a test case that confirms that running difftool --cached with > unmerged files does not result in a SIGSEGV. > > Signed-off-by: Jinoh Kang <luke1337@theori.io> ACK! The patch looks good to go to me. Thank you! Dscho > --- > diff.c | 3 +++ > t/t7800-difftool.sh | 13 +++++++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/diff.c b/diff.c > index d24f47df99..ace4a1d387 100644 > --- a/diff.c > +++ b/diff.c > @@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s) > > void diff_free_filespec_data(struct diff_filespec *s) > { > + if (!s) > + return; > + > diff_free_filespec_blob(s); > FREE_AND_NULL(s->cnt_data); > } > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh > index 524f30f7dc..a578b35761 100755 > --- a/t/t7800-difftool.sh > +++ b/t/t7800-difftool.sh > @@ -728,6 +728,19 @@ test_expect_success 'add -N and difftool -d' ' > git difftool --dir-diff --extcmd ls > ' > > +test_expect_success 'difftool --cached with unmerged files' ' > + test_when_finished git reset --hard && > + > + test_commit conflicting && > + test_commit conflict-a conflict.t a && > + git reset --hard conflicting && > + test_commit conflict-b conflict.t b && > + test_must_fail git merge conflict-a && > + > + git difftool --cached --no-prompt >output && > + test_must_be_empty output > +' > + > test_expect_success 'outside worktree' ' > echo 1 >1 && > echo 2 >2 && > -- > 2.26.2 > >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Jinoh, > > On Wed, 11 Nov 2020, Jinoh Kang wrote: > >> diff_free_filespec_data crashes when passed a NULL fillspec pointer. >> Commit 3aef54e8b8 ("diff: munmap() file contents before running external >> diff", 2019-07-11) introduced calls to diff_free_filespec_data in >> run_external_diff without also checking if the argument is NULL. >> >> Git uses NULL filespecs to indicate unmerged files when merge conflict >> resolution is in progress. Fortunately, other code paths bail out early >> even before NULL can reach diff_free_filespec_data(); however, difftool >> is expected to do a full-blown diff anyway regardless of conflict >> status. >> >> Fix this and prevent any similar bugs in the future by making >> `diff_free_filespec_data(NULL)` a no-op. >> >> Add a test case that confirms that running difftool --cached with >> unmerged files does not result in a SIGSEGV. >> >> Signed-off-by: Jinoh Kang <luke1337@theori.io> > > ACK! > > The patch looks good to go to me. > > Thank you! > Dscho Thanks, both. Will queue. > >> --- >> diff.c | 3 +++ >> t/t7800-difftool.sh | 13 +++++++++++++ >> 2 files changed, 16 insertions(+) >> >> diff --git a/diff.c b/diff.c >> index d24f47df99..ace4a1d387 100644 >> --- a/diff.c >> +++ b/diff.c >> @@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s) >> >> void diff_free_filespec_data(struct diff_filespec *s) >> { >> + if (!s) >> + return; >> + >> diff_free_filespec_blob(s); >> FREE_AND_NULL(s->cnt_data); >> } >> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh >> index 524f30f7dc..a578b35761 100755 >> --- a/t/t7800-difftool.sh >> +++ b/t/t7800-difftool.sh >> @@ -728,6 +728,19 @@ test_expect_success 'add -N and difftool -d' ' >> git difftool --dir-diff --extcmd ls >> ' >> >> +test_expect_success 'difftool --cached with unmerged files' ' >> + test_when_finished git reset --hard && >> + >> + test_commit conflicting && >> + test_commit conflict-a conflict.t a && >> + git reset --hard conflicting && >> + test_commit conflict-b conflict.t b && >> + test_must_fail git merge conflict-a && >> + >> + git difftool --cached --no-prompt >output && >> + test_must_be_empty output >> +' >> + >> test_expect_success 'outside worktree' ' >> echo 1 >1 && >> echo 2 >2 && >> -- >> 2.26.2 >> >>
diff --git a/diff.c b/diff.c index d24f47df99..ace4a1d387 100644 --- a/diff.c +++ b/diff.c @@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s) void diff_free_filespec_data(struct diff_filespec *s) { + if (!s) + return; + diff_free_filespec_blob(s); FREE_AND_NULL(s->cnt_data); } diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 524f30f7dc..a578b35761 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -728,6 +728,19 @@ test_expect_success 'add -N and difftool -d' ' git difftool --dir-diff --extcmd ls ' +test_expect_success 'difftool --cached with unmerged files' ' + test_when_finished git reset --hard && + + test_commit conflicting && + test_commit conflict-a conflict.t a && + git reset --hard conflicting && + test_commit conflict-b conflict.t b && + test_must_fail git merge conflict-a && + + git difftool --cached --no-prompt >output && + test_must_be_empty output +' + test_expect_success 'outside worktree' ' echo 1 >1 && echo 2 >2 &&
diff_free_filespec_data crashes when passed a NULL fillspec pointer. Commit 3aef54e8b8 ("diff: munmap() file contents before running external diff", 2019-07-11) introduced calls to diff_free_filespec_data in run_external_diff without also checking if the argument is NULL. Git uses NULL filespecs to indicate unmerged files when merge conflict resolution is in progress. Fortunately, other code paths bail out early even before NULL can reach diff_free_filespec_data(); however, difftool is expected to do a full-blown diff anyway regardless of conflict status. Fix this and prevent any similar bugs in the future by making `diff_free_filespec_data(NULL)` a no-op. Add a test case that confirms that running difftool --cached with unmerged files does not result in a SIGSEGV. Signed-off-by: Jinoh Kang <luke1337@theori.io> --- diff.c | 3 +++ t/t7800-difftool.sh | 13 +++++++++++++ 2 files changed, 16 insertions(+)