{"thread":{"id":"54590","subject":"[PATCH v2] diff: handle NULL filespecs in run_external_diff","startedAt":"2020-11-06T17:02:32Z","lastAt":"2020-11-11T19:18:56Z","messageCount":15,"participants":["Jinoh Kang","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"409274","messageId":"a0513d6f-1f69-683d-d6c5-75b17b8b6890@theori.io","threadId":"54590","inReplyTo":"xmqq4km4lppy.fsf@gitster.c.googlers.com","subject":"[PATCH v2] diff: handle NULL filespecs in run_external_diff","fromName":"Jinoh Kang","fromEmail":"luke1337@theori.io","sentAt":"2020-11-06T17:02:19Z","receivedAt":"2020-11-06T17:02:32Z","isPatch":true,"sender":{"key":"luke1337@theori.io","avatar":"https://avatars.githubusercontent.com/u/104885525?v=4"},"body":"[forwarding from git-security@googlegroups.com to git@vger.kernel.org]\n\nOn 11/4/20 10:30 PM, Junio C Hamano wrote:\n> Jinoh Kang <luke1337@theori.io> writes:\n> \n>> `one` and `two` can be NULL when comparing an unmerged pair.\n>> There is a conditional above that already tests for this.\n>>\n>> Fixes: 3aef54e8b8 (\"diff: munmap() file contents before running external diff\")\n>> Signed-off-by: Jinoh Kang <luke1337@theori.io>\n>> ---\n>>  diff.c              |  6 ++++--\n>>  t/t7800-difftool.sh | 23 +++++++++++++++++++++++\n>>  2 files changed, 27 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index d24f47df99..ae1ec2d6c8 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -4267,8 +4267,10 @@ static void run_external_diff(const char *pgm,\n>>  \tstrvec_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n>>  \tstrvec_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n>>  \n>> -\tdiff_free_filespec_data(one);\n>> -\tdiff_free_filespec_data(two);\n>> +\tif (one)\n>> +\t\tdiff_free_filespec_data(one);\n>> +\tif (two)\n>> +\t\tdiff_free_filespec_data(two);\n>>  \tif (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))\n>>  \t\tdie(_(\"external diff died, stopping at %s\"), name);\n> \n> Have you considered allowing diff_free_filespec_data() to take NULL\n> and return safely without doing anything?\n\nYes, I have. In fact, this kind of behavior is exactly what I would\nexpect from a function that \"frees\" something.  However, I was not\nentirely sure if this applies here, for several reasons that follow:\n\n> That models after free() and other \"we are done with the resource\n> and it is time to clean it up\" functions,\n\nThis corresponds to `free_filespec`. In this particular case,\nI strongly agree that it makes perfect sense to do nothing when\npassed a NULL pointer.\n\nHowever, I humbly opine that the free() semantics do not apply to\n`diff_free_filespec_data`; rather, I prefer to see it as a function\nthat simply transitions a diff_filespec from one state to another.\n\nFor this reason, I'd consider the act of passing NULL to\ndiff_free_filespec_data as a bug in the first place.  Further,\nif it does not entail a security issue, why not just crash *right\nnow* rather than (possibly) causing more obscure bugs later?\n\nI would put the blame on its name, since \"data\" feels too generic\nand makes the function sound like freeing the filespec _itself_.\ndiff_filespec carries a lot of other things besides just `data`\nand `cnt_data`.\n\nPlease feel free to correct me if I'm wrong; after all, I am not\nexactly one of the long-time maintainers.\n\n> fixes this particular bug, and possibly simplifies existing\n> callers that check the NULL-ness before calling it.\n\nI was unable to find any callsites that explicitly check for\nNULL-ness _immediately_ before calling diff_free_filespec_data.\nExcluding 3aef54e8b8, `GCC -fanalyze` suggests to me that\nall the callers have already dereferenced the pointer by the time\nthe call is made; therefore, a NULL pointer dereference would have\nalready happened before the flow could even get to\nthe diff_free_filespec_data function.\n\nThe NULL checks are usually placed in the beginning of caller\nfunctions rather than close to the exit path (which then calls\ndiff_free_filespec_data).\n\n> \n>> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n>> index 524f30f7dc..8cc1c9117c 100755\n>> --- a/t/t7800-difftool.sh\n>> +++ b/t/t7800-difftool.sh\n>> @@ -728,6 +728,29 @@ test_expect_success 'add -N and difftool -d' '\n>>  \tgit difftool --dir-diff --extcmd ls\n>>  '\n>>  \n>> +test_expect_success 'difftool --cached with unmerged files' '\n>> +\ttest_when_finished git reset --hard &&\n>> +\techo base > file &&\n> \n> Style.  \"echo base >file &&\" (no SP between redirect operator and\n> its target).\n\nMy bad. Thanks!\n\n> \n> I do not think this is \"security\" matter.  I do appreciate you\n> erring on the side of being cautious and sending the patch first\n> here, but please take it to the regular mailing list.\n\nYes, no sensible web-based git browser would prepare a working tree,\nstart a conflicting merge *AND* run an external diff on it...\n\n> \n> Thanks.\n> \n\n-- \nJinoh Kang\nTheori\n"},{"id":"409276","messageId":"a096d122-52a3-700a-3a14-30a81b099cd8@theori.io","threadId":"54590","inReplyTo":"a0513d6f-1f69-683d-d6c5-75b17b8b6890@theori.io","subject":"[PATCH v3] diff: make diff_free_filespec_data accept NULL","fromName":"Jinoh Kang","fromEmail":"luke1337@theori.io","sentAt":"2020-11-06T17:14:52Z","receivedAt":"2020-11-06T17:15:01Z","isPatch":true,"sender":{"key":"luke1337@theori.io","avatar":"https://avatars.githubusercontent.com/u/104885525?v=4"},"body":"Commit 3aef54e8b8 (\"diff: munmap() file contents before running external\ndiff\") introduced calls to diff_free_filespec_data in\nrun_external_diff, which may pass NULL pointers.\n\nFix this and prevent any such bugs in the future by making\n`diff_free_filespec_data(NULL)` a no-op.\n\nFixes: 3aef54e8b8 (\"diff: munmap() file contents before running external diff\")\nSigned-off-by: Jinoh Kang <luke1337@theori.io>\n---\n diff.c              |  3 +++\n t/t7800-difftool.sh | 23 +++++++++++++++++++++++\n 2 files changed, 26 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex d24f47df99..ace4a1d387 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s)\n \n void diff_free_filespec_data(struct diff_filespec *s)\n {\n+\tif (!s)\n+\t\treturn;\n+\n \tdiff_free_filespec_blob(s);\n \tFREE_AND_NULL(s->cnt_data);\n }\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 524f30f7dc..e9391abb54 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -728,6 +728,29 @@ test_expect_success 'add -N and difftool -d' '\n \tgit difftool --dir-diff --extcmd ls\n '\n \n+test_expect_success 'difftool --cached with unmerged files' '\n+\ttest_when_finished git reset --hard &&\n+\techo base >file &&\n+\tgit add file &&\n+\tgit commit -m base &&\n+\tgit checkout -B conflict-a &&\n+\tgit checkout -B conflict-b &&\n+\tgit checkout conflict-a &&\n+\techo conflict-a >>file &&\n+\tgit add file &&\n+\tgit commit -m conflict-a &&\n+\tgit checkout conflict-b &&\n+\techo conflict-b >>file &&\n+\tgit add file &&\n+\tgit commit -m conflict-b &&\n+\tgit checkout master &&\n+\tgit merge conflict-a &&\n+\ttest_must_fail git merge conflict-b &&\n+\t: >expect &&\n+\tgit difftool --cached --no-prompt >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'outside worktree' '\n \techo 1 >1 &&\n \techo 2 >2 &&\n-- \n2.26.2\n\n"},{"id":"409288","messageId":"xmqqimaifg5m.fsf@gitster.c.googlers.com","threadId":"54590","inReplyTo":"a0513d6f-1f69-683d-d6c5-75b17b8b6890@theori.io","subject":"Re: [PATCH v2] diff: handle NULL filespecs in run_external_diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-06T19:18:13Z","receivedAt":"2020-11-06T19:18:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jinoh Kang <luke1337@theori.io> writes:\n\n> However, I humbly opine that the free() semantics do not apply to\n> `diff_free_filespec_data`; rather, I prefer to see it as a function\n> that simply transitions a diff_filespec from one state to another.\n\nThe reason why you prefer is unclear, but let's suppress puzzlement\nand read on.\n\n> I would put the blame on its name, since \"data\" feels too generic\n> and makes the function sound like freeing the filespec _itself_.\n> diff_filespec carries a lot of other things besides just `data`\n> and `cnt_data`.\n\nIt frees resources held by its content data without freeing the\nshell.  \"struct diff_filespec\" has a handful of pointer fields,\nbut data and cnt_data are the only allocated fields, no?\n\n> I was unable to find any callsites that explicitly check for\n> NULL-ness _immediately_ before calling diff_free_filespec_data.\n\nOK, so a change to make diff_free_filespec_data() more cautious does\n*not* help existing code; it changes the (so far) wrong assumption\nmade by 3aef54e8 (diff: munmap() file contents before running\nexternal diff, 2019-07-11) that calling it with NULL was a safe\nthing to do---after such a change, the assumption two calls added by\nthe commit makes become valid.\n\nI dunno.  I am OK with either direction, but it feels to me that\nmaking the helper more cautious would help us avoid similar\nmistakes in the future.  Dscho, what do you think?\n\nThanks.\n"},{"id":"409478","messageId":"nycvar.QRO.7.76.6.2011101257540.18437@tvgsbejvaqbjf.bet","threadId":"54590","inReplyTo":"a096d122-52a3-700a-3a14-30a81b099cd8@theori.io","subject":"Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-10T12:08:22Z","receivedAt":"2020-11-10T12:08:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jinoh,\n\nOn Fri, 6 Nov 2020, Jinoh Kang wrote:\n\n> Commit 3aef54e8b8 (\"diff: munmap() file contents before running external\n> diff\") introduced calls to diff_free_filespec_data in\n> run_external_diff, which may pass NULL pointers.\n\nSorry for the breakage!\n\nMaybe the commit message could talk a bit about the circumstances when\nthis happens? Of course, I (and every other reader...) could analyze your\npatch to guess what it is that triggers the bug, but it's really a good\nuse of the commit message to describe it in plain English.\n\n>\n> Fix this and prevent any such bugs in the future by making\n> `diff_free_filespec_data(NULL)` a no-op.\n>\n> Fixes: 3aef54e8b8 (\"diff: munmap() file contents before running external diff\")\n\nI am unaware of any other commit having a `Fixes:` trailer in the commit\nmessage. In any case, I would have expected `Fixes:` to mention a ticket\nor a bug report, not the commit that fixed _a separate_ issue (but\nunfortunately introduced this regression).\n\n> Signed-off-by: Jinoh Kang <luke1337@theori.io>\n\n> ---\n>  diff.c              |  3 +++\n>  t/t7800-difftool.sh | 23 +++++++++++++++++++++++\n>  2 files changed, 26 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index d24f47df99..ace4a1d387 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s)\n>\n>  void diff_free_filespec_data(struct diff_filespec *s)\n>  {\n> +\tif (!s)\n> +\t\treturn;\n> +\n>  \tdiff_free_filespec_blob(s);\n>  \tFREE_AND_NULL(s->cnt_data);\n\nWe can write this in a more canonical way without wasting all that many\nlines:\n\n\tif (s) {\n\t\tdiff_free_filespec_blob(s);\n\t\tFREE_AND_NULL(s->cnt_data);\n\t}\n\n>  }\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 524f30f7dc..e9391abb54 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -728,6 +728,29 @@ test_expect_success 'add -N and difftool -d' '\n>  \tgit difftool --dir-diff --extcmd ls\n>  '\n>\n> +test_expect_success 'difftool --cached with unmerged files' '\n> +\ttest_when_finished git reset --hard &&\n> +\techo base >file &&\n> +\tgit add file &&\n> +\tgit commit -m base &&\n\nThis does not advance the committer date. Let's just use the helper\nfunction we invented to make this much easier:\n\n\ttest_commit base\n\nThis has also the advantage of already tagging the outcome.\n\n> +\tgit checkout -B conflict-a &&\n> +\tgit checkout -B conflict-b &&\n> +\tgit checkout conflict-a &&\n> +\techo conflict-a >>file &&\n> +\tgit add file &&\n> +\tgit commit -m conflict-a &&\n> +\tgit checkout conflict-b &&\n> +\techo conflict-b >>file &&\n> +\tgit add file &&\n> +\tgit commit -m conflict-b &&\n> +\tgit checkout master &&\n> +\tgit merge conflict-a &&\n> +\ttest_must_fail git merge conflict-b &&\n> +\t: >expect &&\n> +\tgit difftool --cached --no-prompt >actual &&\n> +\ttest_cmp expect actual\n\nShouldn't this use the `test_must_be_empty` function instead?\n\nHow about writing the test case this way:\n\ntest_expect_success 'difftool --cached with unmerged files' '\n\ttest_when_finished git reset --hard &&\n\n\ttest_commit conflicting &&\n\ttest_commit conflict-a a conflicting.t &&\n\tgit reset --hard conflicting &&\n\ttest_commit conflict-b b conflicting.t &&\n\ttest_must_fail git merge conflict-a &&\n\n\tgit difftool --cached --no-prompt >out &&\n\ttest_must_be_empty out\n'\n\nCiao,\nDscho\n\n> +'\n> +\n>  test_expect_success 'outside worktree' '\n>  \techo 1 >1 &&\n>  \techo 2 >2 &&\n> --\n> 2.26.2\n>\n>\n"},{"id":"409481","messageId":"17491ac3-ce1b-ca5d-d4e8-9fa8ab72d7e3@theori.io","threadId":"54590","inReplyTo":"nycvar.QRO.7.76.6.2011101257540.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL","fromName":"Jinoh Kang","fromEmail":"luke1337@theori.io","sentAt":"2020-11-10T13:16:56Z","receivedAt":"2020-11-10T13:17:04Z","isPatch":true,"sender":{"key":"luke1337@theori.io","avatar":"https://avatars.githubusercontent.com/u/104885525?v=4"},"body":"On 11/10/20 12:08 PM, Johannes Schindelin wrote:\n> Hi Jinoh,\n> \n> On Fri, 6 Nov 2020, Jinoh Kang wrote:\n> \n>> Commit 3aef54e8b8 (\"diff: munmap() file contents before running external\n>> diff\") introduced calls to diff_free_filespec_data in\n>> run_external_diff, which may pass NULL pointers.\n> \n> Sorry for the breakage!\n\nNo worries. Those functions were indeed confusing...\n\n> \n> Maybe the commit message could talk a bit about the circumstances when\n> this happens? Of course, I (and every other reader...) could analyze your\n> patch to guess what it is that triggers the bug, but it's really a good\n> use of the commit message to describe it in plain English.\n\nAgreed. The v1 PATCH, which was off-list, did explain that NULL filespecs\nare used to indicate unmerged files (i.e. with unresolved conflicts).\n\n> \n>>\n>> Fix this and prevent any such bugs in the future by making\n>> `diff_free_filespec_data(NULL)` a no-op.\n>>\n>> Fixes: 3aef54e8b8 (\"diff: munmap() file contents before running external diff\")\n> \n> I am unaware of any other commit having a `Fixes:` trailer in the commit\n> message. In any case, I would have expected `Fixes:` to mention a ticket\n> or a bug report, not the commit that fixed _a separate_ issue (but\n> unfortunately introduced this regression).\n\nSorry for this one; somehow I didn't notice that git and linux use\ndifferent conventions.\n\n-- \nJinoh Kang\nTheori\n"},{"id":"409488","messageId":"137f0fc1-fbd9-a62c-bd52-cffd26c364bf@theori.io","threadId":"54590","inReplyTo":"a096d122-52a3-700a-3a14-30a81b099cd8@theori.io","subject":"[PATCH v4] diff: make diff_free_filespec_data accept NULL","fromName":"Jinoh Kang","fromEmail":"luke1337@theori.io","sentAt":"2020-11-10T14:06:56Z","receivedAt":"2020-11-10T14:07:04Z","isPatch":true,"sender":{"key":"luke1337@theori.io","avatar":"https://avatars.githubusercontent.com/u/104885525?v=4"},"body":"Today, diff_free_filespec_data crashes when passed a NULL pointer.\nCommit 3aef54e8b8 (\"diff: munmap() file contents before running external\ndiff\") introduced calls to diff_free_filespec_data in run_external_diff,\nwhich may pass NULL pointers.\n\nGit uses NULL filespecs to indicate unmerged files when merge conflict\nresolution is in progress.  Fortunately, other code paths bail out early\neven before NULL can reach diff_free_filespec_data(); however, difftool\nis expected to do a full-blown diff anyway regardless of conflict\nstatus.\n\nFix this and prevent any similar bugs in the future by making\n`diff_free_filespec_data(NULL)` a no-op.\n\nAlso, add a test case that confirms that running difftool --cached with\nunmerged files does not SIGSEGV.\n\nSigned-off-by: Jinoh Kang <luke1337@theori.io>\n---\n diff.c              |  3 +++\n t/t7800-difftool.sh | 23 +++++++++++++++++++++++\n 2 files changed, 26 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex d24f47df99..ace4a1d387 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s)\n \n void diff_free_filespec_data(struct diff_filespec *s)\n {\n+\tif (!s)\n+\t\treturn;\n+\n \tdiff_free_filespec_blob(s);\n \tFREE_AND_NULL(s->cnt_data);\n }\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 524f30f7dc..e9391abb54 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -728,6 +728,29 @@ test_expect_success 'add -N and difftool -d' '\n \tgit difftool --dir-diff --extcmd ls\n '\n \n+test_expect_success 'difftool --cached with unmerged files' '\n+\ttest_when_finished git reset --hard &&\n+\techo base >file &&\n+\tgit add file &&\n+\tgit commit -m base &&\n+\tgit checkout -B conflict-a &&\n+\tgit checkout -B conflict-b &&\n+\tgit checkout conflict-a &&\n+\techo conflict-a >>file &&\n+\tgit add file &&\n+\tgit commit -m conflict-a &&\n+\tgit checkout conflict-b &&\n+\techo conflict-b >>file &&\n+\tgit add file &&\n+\tgit commit -m conflict-b &&\n+\tgit checkout master &&\n+\tgit merge conflict-a &&\n+\ttest_must_fail git merge conflict-b &&\n+\t: >expect &&\n+\tgit difftool --cached --no-prompt >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'outside worktree' '\n \techo 1 >1 &&\n \techo 2 >2 &&\n-- \n2.26.2\n"},{"id":"409489","messageId":"406549bb-1ab6-eb8a-7383-4715666071f9@theori.io","threadId":"54590","inReplyTo":"nycvar.QRO.7.76.6.2011101257540.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL","fromName":"Jinoh Kang","fromEmail":"luke1337@theori.io","sentAt":"2020-11-10T14:21:14Z","receivedAt":"2020-11-10T14:21:22Z","isPatch":true,"sender":{"key":"luke1337@theori.io","avatar":"https://avatars.githubusercontent.com/u/104885525?v=4"},"body":"On 11/10/20 1:16 PM, Jinoh Kang wrote:\n> On 11/10/20 12:08 PM, Johannes Schindelin wrote:\n>> I am unaware of any other commit having a `Fixes:` trailer in the commit\n>> message. In any case, I would have expected `Fixes:` to mention a ticket\n>> or a bug report, not the commit that fixed _a separate_ issue (but\n>> unfortunately introduced this regression).\n> \n> Sorry for this one; somehow I didn't notice that git and linux use\n> different conventions.\n> \n\nAfter leaving it out, I realized there are actually five commits using\n\"Fixes: <commit>\":\n\n    $ git log --oneline --grep='Fixes: [0-9a-f]\\{8,\\} (\".*\")' master\n    e2bfa50ac3 pack-write/docs: update regarding pack naming\n    7328482253 repack: disable bitmaps-by-default if .keep files exist\n    ba3a08ca0e fsck: fix leak when traversing trees\n    5cae73d5d2 http: release strbuf on disabled alternates\n    c22f620205 xread: retry after poll on EAGAIN/EWOULDBLOCK\n\nSince it's not that common, I guess it's fine to leave it as is?\n\n-- \nJinoh Kang\nTheori\n"},{"id":"409494","messageId":"nycvar.QRO.7.76.6.2011101638080.18437@tvgsbejvaqbjf.bet","threadId":"54590","inReplyTo":"137f0fc1-fbd9-a62c-bd52-cffd26c364bf@theori.io","subject":"Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-10T15:38:39Z","receivedAt":"2020-11-10T15:38:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jinoh,\n\nOn Tue, 10 Nov 2020, Jinoh Kang wrote:\n\n> Today, diff_free_filespec_data crashes when passed a NULL pointer.\n> Commit 3aef54e8b8 (\"diff: munmap() file contents before running external\n> diff\") introduced calls to diff_free_filespec_data in run_external_diff,\n> which may pass NULL pointers.\n>\n> Git uses NULL filespecs to indicate unmerged files when merge conflict\n> resolution is in progress.  Fortunately, other code paths bail out early\n> even before NULL can reach diff_free_filespec_data(); however, difftool\n> is expected to do a full-blown diff anyway regardless of conflict\n> status.\n>\n> Fix this and prevent any similar bugs in the future by making\n> `diff_free_filespec_data(NULL)` a no-op.\n>\n> Also, add a test case that confirms that running difftool --cached with\n> unmerged files does not SIGSEGV.\n>\n> Signed-off-by: Jinoh Kang <luke1337@theori.io>\n> ---\n>  diff.c              |  3 +++\n>  t/t7800-difftool.sh | 23 +++++++++++++++++++++++\n>  2 files changed, 26 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index d24f47df99..ace4a1d387 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s)\n>\n>  void diff_free_filespec_data(struct diff_filespec *s)\n>  {\n> +\tif (!s)\n> +\t\treturn;\n> +\n\nI had suggested an improvement for this hunk as well as for the test case.\nFell through the cracks?\n\nCiao,\nDscho\n\n>  \tdiff_free_filespec_blob(s);\n>  \tFREE_AND_NULL(s->cnt_data);\n>  }\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 524f30f7dc..e9391abb54 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -728,6 +728,29 @@ test_expect_success 'add -N and difftool -d' '\n>  \tgit difftool --dir-diff --extcmd ls\n>  '\n>\n> +test_expect_success 'difftool --cached with unmerged files' '\n> +\ttest_when_finished git reset --hard &&\n> +\techo base >file &&\n> +\tgit add file &&\n> +\tgit commit -m base &&\n> +\tgit checkout -B conflict-a &&\n> +\tgit checkout -B conflict-b &&\n> +\tgit checkout conflict-a &&\n> +\techo conflict-a >>file &&\n> +\tgit add file &&\n> +\tgit commit -m conflict-a &&\n> +\tgit checkout conflict-b &&\n> +\techo conflict-b >>file &&\n> +\tgit add file &&\n> +\tgit commit -m conflict-b &&\n> +\tgit checkout master &&\n> +\tgit merge conflict-a &&\n> +\ttest_must_fail git merge conflict-b &&\n> +\t: >expect &&\n> +\tgit difftool --cached --no-prompt >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'outside worktree' '\n>  \techo 1 >1 &&\n>  \techo 2 >2 &&\n> --\n> 2.26.2\n>\n"},{"id":"409498","messageId":"xmqqblg587sa.fsf@gitster.c.googlers.com","threadId":"54590","inReplyTo":"nycvar.QRO.7.76.6.2011101257540.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-10T17:02:13Z","receivedAt":"2020-11-10T17:02:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>>  void diff_free_filespec_data(struct diff_filespec *s)\n>>  {\n>> +\tif (!s)\n>> +\t\treturn;\n>> +\n>>  \tdiff_free_filespec_blob(s);\n>>  \tFREE_AND_NULL(s->cnt_data);\n>\n> We can write this in a more canonical way without wasting all that many\n> lines:\n>\n> \tif (s) {\n> \t\tdiff_free_filespec_blob(s);\n> \t\tFREE_AND_NULL(s->cnt_data);\n> \t}\n\nOn only this part.  Please do not use this one, as what was posted\nis better.\n\nBy making it clear that we do not do anything when given NULL\nupfront, it lets readers concentrate on the main part of the\nfunction---\"now we are done with NULL, what happens on a real case?\"\nAnd when readers are doing so, one extra indentation level is just\nan extra noise that does not help.\n\nIn this particular case, it is important more from coding discipline\nthan anything else---for a small enough function like this one whose\nmain part fits on less than fourth of a screen, extra indentation\ndoes not matter, but all code tends to grow and it starts to matter\nif/when we need to clean more things in the function.\n\nEverything else said in the review was excellent and very helpful.\n\nThanks.\n"},{"id":"409509","messageId":"xmqqpn4l6ltk.fsf@gitster.c.googlers.com","threadId":"54590","inReplyTo":"137f0fc1-fbd9-a62c-bd52-cffd26c364bf@theori.io","subject":"Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-10T19:41:59Z","receivedAt":"2020-11-10T19:42:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jinoh Kang <luke1337@theori.io> writes:\n\n> Today, diff_free_filespec_data crashes when passed a NULL pointer.\n\nNo need to say \"Today\".  We state how things are in the current\ncodebase in the present tense, make observations on the way things\ncan break (i.e. identify a bug), and outline an approach to correct\nit.\n\n> Commit 3aef54e8b8 (\"diff: munmap() file contents before running external\n> diff\") introduced calls to diff_free_filespec_data in run_external_diff,\n> which may pass NULL pointers.\n>\n> Git uses NULL filespecs to indicate unmerged files when merge conflict\n> resolution is in progress.  Fortunately, other code paths bail out early\n> even before NULL can reach diff_free_filespec_data(); however, difftool\n> is expected to do a full-blown diff anyway regardless of conflict\n> status.\n>\n> Fix this and prevent any similar bugs in the future by making\n> `diff_free_filespec_data(NULL)` a no-op.\n\nNicely described.\n\n> Also, add a test case that confirms that running difftool --cached with\n> unmerged files does not SIGSEGV.\n\n> +test_expect_success 'difftool --cached with unmerged files' '\n> +\ttest_when_finished git reset --hard &&\n> +\techo base >file &&\n> +\tgit add file &&\n> +\tgit commit -m base &&\n> +\tgit checkout -B conflict-a &&\n> +\tgit checkout -B conflict-b &&\n\nThe above two are not wrong per-se, but would conceptually be\ncleaner to use \"git branch -f\", because the next thing you do\nimmediately after preparing two branches is to start working on the\n'A' side, below.\n\nYou could alternatively drop the above two lines and then instead\nturn this\n\n> +\tgit checkout conflict-a &&\n\ninto \"git checkout -B conflict-a master\" (and similarly on the 'B'\nside below), which would reduce the test by two lines.  That would\nbe what I would recommend to do under normal circumstances, but\nsince there is a separate topic that wages war on the 'master'\nbranch, I wouldn't recommend it.\n\n> +\techo conflict-a >>file &&\n> +\tgit add file &&\n> +\tgit commit -m conflict-a &&\n\n> +\tgit checkout conflict-b &&\n> +\techo conflict-b >>file &&\n> +\tgit add file &&\n> +\tgit commit -m conflict-b &&\n\n> +\tgit checkout master &&\n> +\tgit merge conflict-a &&\n> +\ttest_must_fail git merge conflict-b &&\n\n> +\t: >expect &&\n> +\tgit difftool --cached --no-prompt >actual &&\n> +\ttest_cmp expect actual\n\nShouldn't we omit 'expect' and use test_must_be_empty helper\ninstead?\n\n\tgit difftool --cached --no-prompt >actual &&\n\ttest_must_be_empty actual\n\n> +'\n> +\n>  test_expect_success 'outside worktree' '\n>  \techo 1 >1 &&\n>  \techo 2 >2 &&\n"},{"id":"409606","messageId":"5d4315c5-a0ae-2857-fbcc-ec6166d025b6@theori.io","threadId":"54590","inReplyTo":"137f0fc1-fbd9-a62c-bd52-cffd26c364bf@theori.io","subject":"[PATCH v5] diff: make diff_free_filespec_data accept NULL","fromName":"Jinoh Kang","fromEmail":"luke1337@theori.io","sentAt":"2020-11-11T12:15:08Z","receivedAt":"2020-11-11T12:16:34Z","isPatch":true,"sender":{"key":"luke1337@theori.io","avatar":"https://avatars.githubusercontent.com/u/104885525?v=4"},"body":"diff_free_filespec_data crashes when passed a NULL fillspec pointer.\nCommit 3aef54e8b8 (\"diff: munmap() file contents before running external\ndiff\", 2019-07-11) introduced calls to diff_free_filespec_data in\nrun_external_diff without also checking if the argument is NULL.\n\nGit uses NULL filespecs to indicate unmerged files when merge conflict\nresolution is in progress.  Fortunately, other code paths bail out early\neven before NULL can reach diff_free_filespec_data(); however, difftool\nis expected to do a full-blown diff anyway regardless of conflict\nstatus.\n\nFix this and prevent any similar bugs in the future by making\n`diff_free_filespec_data(NULL)` a no-op.\n\nAdd a test case that confirms that running difftool --cached with\nunmerged files does not result in a SIGSEGV.\n\nSigned-off-by: Jinoh Kang <luke1337@theori.io>\n---\n diff.c              |  3 +++\n t/t7800-difftool.sh | 13 +++++++++++++\n 2 files changed, 16 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex d24f47df99..ace4a1d387 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s)\n \n void diff_free_filespec_data(struct diff_filespec *s)\n {\n+\tif (!s)\n+\t\treturn;\n+\n \tdiff_free_filespec_blob(s);\n \tFREE_AND_NULL(s->cnt_data);\n }\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 524f30f7dc..a578b35761 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -728,6 +728,19 @@ test_expect_success 'add -N and difftool -d' '\n \tgit difftool --dir-diff --extcmd ls\n '\n \n+test_expect_success 'difftool --cached with unmerged files' '\n+\ttest_when_finished git reset --hard &&\n+\n+\ttest_commit conflicting &&\n+\ttest_commit conflict-a conflict.t a &&\n+\tgit reset --hard conflicting &&\n+\ttest_commit conflict-b conflict.t b &&\n+\ttest_must_fail git merge conflict-a &&\n+\n+\tgit difftool --cached --no-prompt >output &&\n+\ttest_must_be_empty output\n+'\n+\n test_expect_success 'outside worktree' '\n \techo 1 >1 &&\n \techo 2 >2 &&\n-- \n2.26.2\n\n"},{"id":"409619","messageId":"01640022-5bda-9712-8228-501374474d5f@theori.io","threadId":"54590","inReplyTo":"nycvar.QRO.7.76.6.2011101638080.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL","fromName":"Jinoh Kang","fromEmail":"luke1337@theori.io","sentAt":"2020-11-11T12:30:28Z","receivedAt":"2020-11-11T12:30:37Z","isPatch":true,"sender":{"key":"luke1337@theori.io","avatar":"https://avatars.githubusercontent.com/u/104885525?v=4"},"body":"On 11/10/20 3:38 PM, Johannes Schindelin wrote:\n> I had suggested an improvement for this hunk as well as for the test case.\n> Fell through the cracks?\n\nYou guessed it right. My apologies.\n\n> +test_expect_success 'difftool --cached with unmerged files' '\n> +\ttest_when_finished git reset --hard &&\n> +\techo base >file &&\n> +\tgit add file &&\n> +\tgit commit -m base &&\n> \n> This does not advance the committer date. Let's just use the helper\n> function we invented to make this much easier:\n> \n> \ttest_commit base\n> \n> This has also the advantage of already tagging the outcome.\n> \n>> +\tgit checkout -B conflict-a &&\n>> +\tgit checkout -B conflict-b &&\n>> +\tgit checkout conflict-a &&\n>> +\techo conflict-a >>file &&\n>> +\tgit add file &&\n>> +\tgit commit -m conflict-a &&\n>> +\tgit checkout conflict-b &&\n>> +\techo conflict-b >>file &&\n>> +\tgit add file &&\n>> +\tgit commit -m conflict-b &&\n>> +\tgit checkout master &&\n>> +\tgit merge conflict-a &&\n>> +\ttest_must_fail git merge conflict-b &&\n>> +\t: >expect &&\n>> +\tgit difftool --cached --no-prompt >actual &&\n>> +\ttest_cmp expect actual\n> \n> Shouldn't this use the `test_must_be_empty` function instead?\n> \n> How about writing the test case this way:\n> \n> test_expect_success 'difftool --cached with unmerged files' '\n> \ttest_when_finished git reset --hard &&\n> \n> \ttest_commit conflicting &&\n> \ttest_commit conflict-a a conflicting.t &&\n> \tgit reset --hard conflicting &&\n> \ttest_commit conflict-b b conflicting.t &&\n> \ttest_must_fail git merge conflict-a &&\n> \n> \tgit difftool --cached --no-prompt >out &&\n> \ttest_must_be_empty out\n> '\n\nThe original test code was copied from the \"difftool --dir-diff with\nunmerged files\" case above.\n\nIt might be worth cleaning it up too, but let's leave it for another\ntime.\n\nI'm keeping the return-early code as per Junio's request.\n\n-- \nJinoh Kang\nTheori\n"},{"id":"409641","messageId":"nycvar.QRO.7.76.6.2011111727090.18437@tvgsbejvaqbjf.bet","threadId":"54590","inReplyTo":"5d4315c5-a0ae-2857-fbcc-ec6166d025b6@theori.io","subject":"Re: [PATCH v5] diff: make diff_free_filespec_data accept NULL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-11T16:27:48Z","receivedAt":"2020-11-11T16:27:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jinoh,\n\nOn Wed, 11 Nov 2020, Jinoh Kang wrote:\n\n> diff_free_filespec_data crashes when passed a NULL fillspec pointer.\n> Commit 3aef54e8b8 (\"diff: munmap() file contents before running external\n> diff\", 2019-07-11) introduced calls to diff_free_filespec_data in\n> run_external_diff without also checking if the argument is NULL.\n>\n> Git uses NULL filespecs to indicate unmerged files when merge conflict\n> resolution is in progress.  Fortunately, other code paths bail out early\n> even before NULL can reach diff_free_filespec_data(); however, difftool\n> is expected to do a full-blown diff anyway regardless of conflict\n> status.\n>\n> Fix this and prevent any similar bugs in the future by making\n> `diff_free_filespec_data(NULL)` a no-op.\n>\n> Add a test case that confirms that running difftool --cached with\n> unmerged files does not result in a SIGSEGV.\n>\n> Signed-off-by: Jinoh Kang <luke1337@theori.io>\n\nACK!\n\nThe patch looks good to go to me.\n\nThank you!\nDscho\n\n> ---\n>  diff.c              |  3 +++\n>  t/t7800-difftool.sh | 13 +++++++++++++\n>  2 files changed, 16 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index d24f47df99..ace4a1d387 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s)\n>\n>  void diff_free_filespec_data(struct diff_filespec *s)\n>  {\n> +\tif (!s)\n> +\t\treturn;\n> +\n>  \tdiff_free_filespec_blob(s);\n>  \tFREE_AND_NULL(s->cnt_data);\n>  }\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 524f30f7dc..a578b35761 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -728,6 +728,19 @@ test_expect_success 'add -N and difftool -d' '\n>  \tgit difftool --dir-diff --extcmd ls\n>  '\n>\n> +test_expect_success 'difftool --cached with unmerged files' '\n> +\ttest_when_finished git reset --hard &&\n> +\n> +\ttest_commit conflicting &&\n> +\ttest_commit conflict-a conflict.t a &&\n> +\tgit reset --hard conflicting &&\n> +\ttest_commit conflict-b conflict.t b &&\n> +\ttest_must_fail git merge conflict-a &&\n> +\n> +\tgit difftool --cached --no-prompt >output &&\n> +\ttest_must_be_empty output\n> +'\n> +\n>  test_expect_success 'outside worktree' '\n>  \techo 1 >1 &&\n>  \techo 2 >2 &&\n> --\n> 2.26.2\n>\n>\n"},{"id":"409642","messageId":"nycvar.QRO.7.76.6.2011111728040.18437@tvgsbejvaqbjf.bet","threadId":"54590","inReplyTo":"01640022-5bda-9712-8228-501374474d5f@theori.io","subject":"Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-11T16:28:33Z","receivedAt":"2020-11-11T16:28:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jinoh,\n\nOn Wed, 11 Nov 2020, Jinoh Kang wrote:\n\n> On 11/10/20 3:38 PM, Johannes Schindelin wrote:\n> >\n> >> +\tgit checkout -B conflict-a &&\n> >> +\tgit checkout -B conflict-b &&\n> >> +\tgit checkout conflict-a &&\n> >> +\techo conflict-a >>file &&\n> >> +\tgit add file &&\n> >> +\tgit commit -m conflict-a &&\n> >> +\tgit checkout conflict-b &&\n> >> +\techo conflict-b >>file &&\n> >> +\tgit add file &&\n> >> +\tgit commit -m conflict-b &&\n> >> +\tgit checkout master &&\n> >> +\tgit merge conflict-a &&\n> >> +\ttest_must_fail git merge conflict-b &&\n> >> +\t: >expect &&\n> >> +\tgit difftool --cached --no-prompt >actual &&\n> >> +\ttest_cmp expect actual\n> >\n> > Shouldn't this use the `test_must_be_empty` function instead?\n> >\n> > How about writing the test case this way:\n> >\n> > test_expect_success 'difftool --cached with unmerged files' '\n> > \ttest_when_finished git reset --hard &&\n> >\n> > \ttest_commit conflicting &&\n> > \ttest_commit conflict-a a conflicting.t &&\n> > \tgit reset --hard conflicting &&\n> > \ttest_commit conflict-b b conflicting.t &&\n> > \ttest_must_fail git merge conflict-a &&\n> >\n> > \tgit difftool --cached --no-prompt >out &&\n> > \ttest_must_be_empty out\n> > '\n>\n> The original test code was copied from the \"difftool --dir-diff with\n> unmerged files\" case above.\n>\n> It might be worth cleaning it up too, but let's leave it for another\n> time.\n\nIndeed. #leftoverbits\n\nThanks,\nDscho\n"},{"id":"409666","messageId":"xmqqk0ur4s89.fsf@gitster.c.googlers.com","threadId":"54590","inReplyTo":"nycvar.QRO.7.76.6.2011111727090.18437@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v5] diff: make diff_free_filespec_data accept NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-11T19:18:46Z","receivedAt":"2020-11-11T19:18:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi Jinoh,\n>\n> On Wed, 11 Nov 2020, Jinoh Kang wrote:\n>\n>> diff_free_filespec_data crashes when passed a NULL fillspec pointer.\n>> Commit 3aef54e8b8 (\"diff: munmap() file contents before running external\n>> diff\", 2019-07-11) introduced calls to diff_free_filespec_data in\n>> run_external_diff without also checking if the argument is NULL.\n>>\n>> Git uses NULL filespecs to indicate unmerged files when merge conflict\n>> resolution is in progress.  Fortunately, other code paths bail out early\n>> even before NULL can reach diff_free_filespec_data(); however, difftool\n>> is expected to do a full-blown diff anyway regardless of conflict\n>> status.\n>>\n>> Fix this and prevent any similar bugs in the future by making\n>> `diff_free_filespec_data(NULL)` a no-op.\n>>\n>> Add a test case that confirms that running difftool --cached with\n>> unmerged files does not result in a SIGSEGV.\n>>\n>> Signed-off-by: Jinoh Kang <luke1337@theori.io>\n>\n> ACK!\n>\n> The patch looks good to go to me.\n>\n> Thank you!\n> Dscho\n\nThanks, both.\nWill queue.\n\n\n>\n>> ---\n>>  diff.c              |  3 +++\n>>  t/t7800-difftool.sh | 13 +++++++++++++\n>>  2 files changed, 16 insertions(+)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index d24f47df99..ace4a1d387 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -4115,6 +4115,9 @@ void diff_free_filespec_blob(struct diff_filespec *s)\n>>\n>>  void diff_free_filespec_data(struct diff_filespec *s)\n>>  {\n>> +\tif (!s)\n>> +\t\treturn;\n>> +\n>>  \tdiff_free_filespec_blob(s);\n>>  \tFREE_AND_NULL(s->cnt_data);\n>>  }\n>> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n>> index 524f30f7dc..a578b35761 100755\n>> --- a/t/t7800-difftool.sh\n>> +++ b/t/t7800-difftool.sh\n>> @@ -728,6 +728,19 @@ test_expect_success 'add -N and difftool -d' '\n>>  \tgit difftool --dir-diff --extcmd ls\n>>  '\n>>\n>> +test_expect_success 'difftool --cached with unmerged files' '\n>> +\ttest_when_finished git reset --hard &&\n>> +\n>> +\ttest_commit conflicting &&\n>> +\ttest_commit conflict-a conflict.t a &&\n>> +\tgit reset --hard conflicting &&\n>> +\ttest_commit conflict-b conflict.t b &&\n>> +\ttest_must_fail git merge conflict-a &&\n>> +\n>> +\tgit difftool --cached --no-prompt >output &&\n>> +\ttest_must_be_empty output\n>> +'\n>> +\n>>  test_expect_success 'outside worktree' '\n>>  \techo 1 >1 &&\n>>  \techo 2 >2 &&\n>> --\n>> 2.26.2\n>>\n>>\n"}]}