[PATCH v2] diff: handle NULL filespecs in run_external_diff

[PATCH v2] diff: handle NULL filespecs in run_external_diff

[Jinoh Kang]

[forwarding from git-security@googlegroups.com to git@vger.kernel.org]

On 11/4/20 10:30 PM, Junio C Hamano wrote:
> Jinoh Kang <luke1337@theori.io> writes:
> 
>> `one` and `two` can be NULL when comparing an unmerged pair.
>> There is a conditional above that already tests for this.
>>
>> Fixes: 3aef54e8b8 ("diff: munmap() file contents before running external diff")
>> Signed-off-by: Jinoh Kang <luke1337@theori.io>
>> ---
>>  diff.c              |  6 ++++--
>>  t/t7800-difftool.sh | 23 +++++++++++++++++++++++
>>  2 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/diff.c b/diff.c
>> index d24f47df99..ae1ec2d6c8 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -4267,8 +4267,10 @@ static void run_external_diff(const char *pgm,
>>  	strvec_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
>>  	strvec_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
>>  
>> -	diff_free_filespec_data(one);
>> -	diff_free_filespec_data(two);
>> +	if (one)
>> +		diff_free_filespec_data(one);
>> +	if (two)
>> +		diff_free_filespec_data(two);
>>  	if (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))
>>  		die(_("external diff died, stopping at %s"), name);
> 
> Have you considered allowing diff_free_filespec_data() to take NULL
> and return safely without doing anything?

Yes, I have. In fact, this kind of behavior is exactly what I would
expect from a function that "frees" something.  However, I was not
entirely sure if this applies here, for several reasons that follow:

> That models after free() and other "we are done with the resource
> and it is time to clean it up" functions,

This corresponds to `free_filespec`. In this particular case,
I strongly agree that it makes perfect sense to do nothing when
passed a NULL pointer.

However, I humbly opine that the free() semantics do not apply to
`diff_free_filespec_data`; rather, I prefer to see it as a function
that simply transitions a diff_filespec from one state to another.

For this reason, I'd consider the act of passing NULL to
diff_free_filespec_data as a bug in the first place.  Further,
if it does not entail a security issue, why not just crash *right
now* rather than (possibly) causing more obscure bugs later?

I would put the blame on its name, since "data" feels too generic
and makes the function sound like freeing the filespec _itself_.
diff_filespec carries a lot of other things besides just `data`
and `cnt_data`.

Please feel free to correct me if I'm wrong; after all, I am not
exactly one of the long-time maintainers.

> fixes this particular bug, and possibly simplifies existing
> callers that check the NULL-ness before calling it.

I was unable to find any callsites that explicitly check for
NULL-ness _immediately_ before calling diff_free_filespec_data.
Excluding 3aef54e8b8, `GCC -fanalyze` suggests to me that
all the callers have already dereferenced the pointer by the time
the call is made; therefore, a NULL pointer dereference would have
already happened before the flow could even get to
the diff_free_filespec_data function.

The NULL checks are usually placed in the beginning of caller
functions rather than close to the exit path (which then calls
diff_free_filespec_data).

> 
>> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
>> index 524f30f7dc..8cc1c9117c 100755
>> --- a/t/t7800-difftool.sh
>> +++ b/t/t7800-difftool.sh
>> @@ -728,6 +728,29 @@ 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 &&
>> +	echo base > file &&
> 
> Style.  "echo base >file &&" (no SP between redirect operator and
> its target).

My bad. Thanks!

> 
> I do not think this is "security" matter.  I do appreciate you
> erring on the side of being cautious and sending the patch first
> here, but please take it to the regular mailing list.

Yes, no sensible web-based git browser would prepare a working tree,
start a conflicting merge *AND* run an external diff on it...

> 
> Thanks.
> 

-- 
Jinoh Kang
Theori

[PATCH v3] diff: make diff_free_filespec_data accept NULL

[Jinoh Kang]

Commit 3aef54e8b8 ("diff: munmap() file contents before running external
diff") introduced calls to diff_free_filespec_data in
run_external_diff, which may pass NULL pointers.

Fix this and prevent any such bugs in the future by making
`diff_free_filespec_data(NULL)` a no-op.

Fixes: 3aef54e8b8 ("diff: munmap() file contents before running external diff")
Signed-off-by: Jinoh Kang <luke1337@theori.io>
---
 diff.c              |  3 +++
 t/t7800-difftool.sh | 23 +++++++++++++++++++++++
 2 files changed, 26 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..e9391abb54 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -728,6 +728,29 @@ 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 &&
+	echo base >file &&
+	git add file &&
+	git commit -m base &&
+	git checkout -B conflict-a &&
+	git checkout -B conflict-b &&
+	git checkout conflict-a &&
+	echo conflict-a >>file &&
+	git add file &&
+	git commit -m conflict-a &&
+	git checkout conflict-b &&
+	echo conflict-b >>file &&
+	git add file &&
+	git commit -m conflict-b &&
+	git checkout master &&
+	git merge conflict-a &&
+	test_must_fail git merge conflict-b &&
+	: >expect &&
+	git difftool --cached --no-prompt >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'outside worktree' '
 	echo 1 >1 &&
 	echo 2 >2 &&
-- 
2.26.2

Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL

[Johannes Schindelin]

Hi Jinoh,

On Fri, 6 Nov 2020, Jinoh Kang wrote:

> Commit 3aef54e8b8 ("diff: munmap() file contents before running external
> diff") introduced calls to diff_free_filespec_data in
> run_external_diff, which may pass NULL pointers.

Sorry for the breakage!

Maybe the commit message could talk a bit about the circumstances when
this happens? Of course, I (and every other reader...) could analyze your
patch to guess what it is that triggers the bug, but it's really a good
use of the commit message to describe it in plain English.

>
> Fix this and prevent any such bugs in the future by making
> `diff_free_filespec_data(NULL)` a no-op.
>
> Fixes: 3aef54e8b8 ("diff: munmap() file contents before running external diff")

I am unaware of any other commit having a `Fixes:` trailer in the commit
message. In any case, I would have expected `Fixes:` to mention a ticket
or a bug report, not the commit that fixed _a separate_ issue (but
unfortunately introduced this regression).

> Signed-off-by: Jinoh Kang <luke1337@theori.io>

> ---
>  diff.c              |  3 +++
>  t/t7800-difftool.sh | 23 +++++++++++++++++++++++
>  2 files changed, 26 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);

We can write this in a more canonical way without wasting all that many
lines:

	if (s) {
		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..e9391abb54 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -728,6 +728,29 @@ 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 &&
> +	echo base >file &&
> +	git add file &&
> +	git commit -m base &&

This does not advance the committer date. Let's just use the helper
function we invented to make this much easier:

	test_commit base

This has also the advantage of already tagging the outcome.

> +	git checkout -B conflict-a &&
> +	git checkout -B conflict-b &&
> +	git checkout conflict-a &&
> +	echo conflict-a >>file &&
> +	git add file &&
> +	git commit -m conflict-a &&
> +	git checkout conflict-b &&
> +	echo conflict-b >>file &&
> +	git add file &&
> +	git commit -m conflict-b &&
> +	git checkout master &&
> +	git merge conflict-a &&
> +	test_must_fail git merge conflict-b &&
> +	: >expect &&
> +	git difftool --cached --no-prompt >actual &&
> +	test_cmp expect actual

Shouldn't this use the `test_must_be_empty` function instead?

How about writing the test case this way:

test_expect_success 'difftool --cached with unmerged files' '
	test_when_finished git reset --hard &&

	test_commit conflicting &&
	test_commit conflict-a a conflicting.t &&
	git reset --hard conflicting &&
	test_commit conflict-b b conflicting.t &&
	test_must_fail git merge conflict-a &&

	git difftool --cached --no-prompt >out &&
	test_must_be_empty out
'

Ciao,
Dscho

> +'
> +
>  test_expect_success 'outside worktree' '
>  	echo 1 >1 &&
>  	echo 2 >2 &&
> --
> 2.26.2
>
>

Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL

[Jinoh Kang]

On 11/10/20 12:08 PM, Johannes Schindelin wrote:
> Hi Jinoh,
> 
> On Fri, 6 Nov 2020, Jinoh Kang wrote:
> 
>> Commit 3aef54e8b8 ("diff: munmap() file contents before running external
>> diff") introduced calls to diff_free_filespec_data in
>> run_external_diff, which may pass NULL pointers.
> 
> Sorry for the breakage!

No worries. Those functions were indeed confusing...

> 
> Maybe the commit message could talk a bit about the circumstances when
> this happens? Of course, I (and every other reader...) could analyze your
> patch to guess what it is that triggers the bug, but it's really a good
> use of the commit message to describe it in plain English.

Agreed. The v1 PATCH, which was off-list, did explain that NULL filespecs
are used to indicate unmerged files (i.e. with unresolved conflicts).

> 
>>
>> Fix this and prevent any such bugs in the future by making
>> `diff_free_filespec_data(NULL)` a no-op.
>>
>> Fixes: 3aef54e8b8 ("diff: munmap() file contents before running external diff")
> 
> I am unaware of any other commit having a `Fixes:` trailer in the commit
> message. In any case, I would have expected `Fixes:` to mention a ticket
> or a bug report, not the commit that fixed _a separate_ issue (but
> unfortunately introduced this regression).

Sorry for this one; somehow I didn't notice that git and linux use
different conventions.

-- 
Jinoh Kang
Theori

Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL

[Jinoh Kang]

On 11/10/20 1:16 PM, Jinoh Kang wrote:
> On 11/10/20 12:08 PM, Johannes Schindelin wrote:
>> I am unaware of any other commit having a `Fixes:` trailer in the commit
>> message. In any case, I would have expected `Fixes:` to mention a ticket
>> or a bug report, not the commit that fixed _a separate_ issue (but
>> unfortunately introduced this regression).
> 
> Sorry for this one; somehow I didn't notice that git and linux use
> different conventions.
> 

After leaving it out, I realized there are actually five commits using
"Fixes: <commit>":

    $ git log --oneline --grep='Fixes: [0-9a-f]\{8,\} (".*")' master
    e2bfa50ac3 pack-write/docs: update regarding pack naming
    7328482253 repack: disable bitmaps-by-default if .keep files exist
    ba3a08ca0e fsck: fix leak when traversing trees
    5cae73d5d2 http: release strbuf on disabled alternates
    c22f620205 xread: retry after poll on EAGAIN/EWOULDBLOCK

Since it's not that common, I guess it's fine to leave it as is?

-- 
Jinoh Kang
Theori

Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>>  void diff_free_filespec_data(struct diff_filespec *s)
>>  {
>> +	if (!s)
>> +		return;
>> +
>>  	diff_free_filespec_blob(s);
>>  	FREE_AND_NULL(s->cnt_data);
>
> We can write this in a more canonical way without wasting all that many
> lines:
>
> 	if (s) {
> 		diff_free_filespec_blob(s);
> 		FREE_AND_NULL(s->cnt_data);
> 	}

On only this part.  Please do not use this one, as what was posted
is better.

By making it clear that we do not do anything when given NULL
upfront, it lets readers concentrate on the main part of the
function---"now we are done with NULL, what happens on a real case?"
And when readers are doing so, one extra indentation level is just
an extra noise that does not help.

In this particular case, it is important more from coding discipline
than anything else---for a small enough function like this one whose
main part fits on less than fourth of a screen, extra indentation
does not matter, but all code tends to grow and it starts to matter
if/when we need to clean more things in the function.

Everything else said in the review was excellent and very helpful.

Thanks.

[PATCH v4] diff: make diff_free_filespec_data accept NULL

[Jinoh Kang]

Today, diff_free_filespec_data crashes when passed a NULL pointer.
Commit 3aef54e8b8 ("diff: munmap() file contents before running external
diff") introduced calls to diff_free_filespec_data in run_external_diff,
which may pass NULL pointers.

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.

Also, add a test case that confirms that running difftool --cached with
unmerged files does not SIGSEGV.

Signed-off-by: Jinoh Kang <luke1337@theori.io>
---
 diff.c              |  3 +++
 t/t7800-difftool.sh | 23 +++++++++++++++++++++++
 2 files changed, 26 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..e9391abb54 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -728,6 +728,29 @@ 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 &&
+	echo base >file &&
+	git add file &&
+	git commit -m base &&
+	git checkout -B conflict-a &&
+	git checkout -B conflict-b &&
+	git checkout conflict-a &&
+	echo conflict-a >>file &&
+	git add file &&
+	git commit -m conflict-a &&
+	git checkout conflict-b &&
+	echo conflict-b >>file &&
+	git add file &&
+	git commit -m conflict-b &&
+	git checkout master &&
+	git merge conflict-a &&
+	test_must_fail git merge conflict-b &&
+	: >expect &&
+	git difftool --cached --no-prompt >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'outside worktree' '
 	echo 1 >1 &&
 	echo 2 >2 &&
-- 
2.26.2

Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL

[Johannes Schindelin]

Hi Jinoh,

On Tue, 10 Nov 2020, Jinoh Kang wrote:

> Today, diff_free_filespec_data crashes when passed a NULL pointer.
> Commit 3aef54e8b8 ("diff: munmap() file contents before running external
> diff") introduced calls to diff_free_filespec_data in run_external_diff,
> which may pass NULL pointers.
>
> 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.
>
> Also, add a test case that confirms that running difftool --cached with
> unmerged files does not SIGSEGV.
>
> Signed-off-by: Jinoh Kang <luke1337@theori.io>
> ---
>  diff.c              |  3 +++
>  t/t7800-difftool.sh | 23 +++++++++++++++++++++++
>  2 files changed, 26 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;
> +

I had suggested an improvement for this hunk as well as for the test case.
Fell through the cracks?

Ciao,
Dscho

>  	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..e9391abb54 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -728,6 +728,29 @@ 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 &&
> +	echo base >file &&
> +	git add file &&
> +	git commit -m base &&
> +	git checkout -B conflict-a &&
> +	git checkout -B conflict-b &&
> +	git checkout conflict-a &&
> +	echo conflict-a >>file &&
> +	git add file &&
> +	git commit -m conflict-a &&
> +	git checkout conflict-b &&
> +	echo conflict-b >>file &&
> +	git add file &&
> +	git commit -m conflict-b &&
> +	git checkout master &&
> +	git merge conflict-a &&
> +	test_must_fail git merge conflict-b &&
> +	: >expect &&
> +	git difftool --cached --no-prompt >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'outside worktree' '
>  	echo 1 >1 &&
>  	echo 2 >2 &&
> --
> 2.26.2
>

Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL

[Jinoh Kang]

On 11/10/20 3:38 PM, Johannes Schindelin wrote:
> I had suggested an improvement for this hunk as well as for the test case.
> Fell through the cracks?

You guessed it right. My apologies.

> +test_expect_success 'difftool --cached with unmerged files' '
> +	test_when_finished git reset --hard &&
> +	echo base >file &&
> +	git add file &&
> +	git commit -m base &&
> 
> This does not advance the committer date. Let's just use the helper
> function we invented to make this much easier:
> 
> 	test_commit base
> 
> This has also the advantage of already tagging the outcome.
> 
>> +	git checkout -B conflict-a &&
>> +	git checkout -B conflict-b &&
>> +	git checkout conflict-a &&
>> +	echo conflict-a >>file &&
>> +	git add file &&
>> +	git commit -m conflict-a &&
>> +	git checkout conflict-b &&
>> +	echo conflict-b >>file &&
>> +	git add file &&
>> +	git commit -m conflict-b &&
>> +	git checkout master &&
>> +	git merge conflict-a &&
>> +	test_must_fail git merge conflict-b &&
>> +	: >expect &&
>> +	git difftool --cached --no-prompt >actual &&
>> +	test_cmp expect actual
> 
> Shouldn't this use the `test_must_be_empty` function instead?
> 
> How about writing the test case this way:
> 
> test_expect_success 'difftool --cached with unmerged files' '
> 	test_when_finished git reset --hard &&
> 
> 	test_commit conflicting &&
> 	test_commit conflict-a a conflicting.t &&
> 	git reset --hard conflicting &&
> 	test_commit conflict-b b conflicting.t &&
> 	test_must_fail git merge conflict-a &&
> 
> 	git difftool --cached --no-prompt >out &&
> 	test_must_be_empty out
> '

The original test code was copied from the "difftool --dir-diff with
unmerged files" case above.

It might be worth cleaning it up too, but let's leave it for another
time.

I'm keeping the return-early code as per Junio's request.

-- 
Jinoh Kang
Theori

Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL

[Johannes Schindelin]

Hi Jinoh,

On Wed, 11 Nov 2020, Jinoh Kang wrote:

> On 11/10/20 3:38 PM, Johannes Schindelin wrote:
> >
> >> +	git checkout -B conflict-a &&
> >> +	git checkout -B conflict-b &&
> >> +	git checkout conflict-a &&
> >> +	echo conflict-a >>file &&
> >> +	git add file &&
> >> +	git commit -m conflict-a &&
> >> +	git checkout conflict-b &&
> >> +	echo conflict-b >>file &&
> >> +	git add file &&
> >> +	git commit -m conflict-b &&
> >> +	git checkout master &&
> >> +	git merge conflict-a &&
> >> +	test_must_fail git merge conflict-b &&
> >> +	: >expect &&
> >> +	git difftool --cached --no-prompt >actual &&
> >> +	test_cmp expect actual
> >
> > Shouldn't this use the `test_must_be_empty` function instead?
> >
> > How about writing the test case this way:
> >
> > test_expect_success 'difftool --cached with unmerged files' '
> > 	test_when_finished git reset --hard &&
> >
> > 	test_commit conflicting &&
> > 	test_commit conflict-a a conflicting.t &&
> > 	git reset --hard conflicting &&
> > 	test_commit conflict-b b conflicting.t &&
> > 	test_must_fail git merge conflict-a &&
> >
> > 	git difftool --cached --no-prompt >out &&
> > 	test_must_be_empty out
> > '
>
> The original test code was copied from the "difftool --dir-diff with
> unmerged files" case above.
>
> It might be worth cleaning it up too, but let's leave it for another
> time.

Indeed. #leftoverbits

Thanks,
Dscho

Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL

[Junio C Hamano]

Jinoh Kang <luke1337@theori.io> writes:

> Today, diff_free_filespec_data crashes when passed a NULL pointer.

No need to say "Today".  We state how things are in the current
codebase in the present tense, make observations on the way things
can break (i.e. identify a bug), and outline an approach to correct
it.

> Commit 3aef54e8b8 ("diff: munmap() file contents before running external
> diff") introduced calls to diff_free_filespec_data in run_external_diff,
> which may pass NULL pointers.
>
> 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.

Nicely described.

> Also, add a test case that confirms that running difftool --cached with
> unmerged files does not SIGSEGV.

> +test_expect_success 'difftool --cached with unmerged files' '
> +	test_when_finished git reset --hard &&
> +	echo base >file &&
> +	git add file &&
> +	git commit -m base &&
> +	git checkout -B conflict-a &&
> +	git checkout -B conflict-b &&

The above two are not wrong per-se, but would conceptually be
cleaner to use "git branch -f", because the next thing you do
immediately after preparing two branches is to start working on the
'A' side, below.

You could alternatively drop the above two lines and then instead
turn this

> +	git checkout conflict-a &&

into "git checkout -B conflict-a master" (and similarly on the 'B'
side below), which would reduce the test by two lines.  That would
be what I would recommend to do under normal circumstances, but
since there is a separate topic that wages war on the 'master'
branch, I wouldn't recommend it.

> +	echo conflict-a >>file &&
> +	git add file &&
> +	git commit -m conflict-a &&

> +	git checkout conflict-b &&
> +	echo conflict-b >>file &&
> +	git add file &&
> +	git commit -m conflict-b &&

> +	git checkout master &&
> +	git merge conflict-a &&
> +	test_must_fail git merge conflict-b &&

> +	: >expect &&
> +	git difftool --cached --no-prompt >actual &&
> +	test_cmp expect actual

Shouldn't we omit 'expect' and use test_must_be_empty helper
instead?

	git difftool --cached --no-prompt >actual &&
	test_must_be_empty actual

> +'
> +
>  test_expect_success 'outside worktree' '
>  	echo 1 >1 &&
>  	echo 2 >2 &&

[PATCH v5] diff: make diff_free_filespec_data accept NULL

[Jinoh Kang]

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

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

Re: [PATCH v5] diff: make diff_free_filespec_data accept NULL

[Johannes Schindelin]

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

Re: [PATCH v5] diff: make diff_free_filespec_data accept NULL

[Junio C Hamano]

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

Re: [PATCH v2] diff: handle NULL filespecs in run_external_diff

[Junio C Hamano]

Jinoh Kang <luke1337@theori.io> writes:

> However, I humbly opine that the free() semantics do not apply to
> `diff_free_filespec_data`; rather, I prefer to see it as a function
> that simply transitions a diff_filespec from one state to another.

The reason why you prefer is unclear, but let's suppress puzzlement
and read on.

> I would put the blame on its name, since "data" feels too generic
> and makes the function sound like freeing the filespec _itself_.
> diff_filespec carries a lot of other things besides just `data`
> and `cnt_data`.

It frees resources held by its content data without freeing the
shell.  "struct diff_filespec" has a handful of pointer fields,
but data and cnt_data are the only allocated fields, no?

> I was unable to find any callsites that explicitly check for
> NULL-ness _immediately_ before calling diff_free_filespec_data.

OK, so a change to make diff_free_filespec_data() more cautious does
*not* help existing code; it changes the (so far) wrong assumption
made by 3aef54e8 (diff: munmap() file contents before running
external diff, 2019-07-11) that calling it with NULL was a safe
thing to do---after such a change, the assumption two calls added by
the commit makes become valid.

I dunno.  I am OK with either direction, but it feels to me that
making the helper more cautious would help us avoid similar
mistakes in the future.  Dscho, what do you think?

Thanks.