* [PATCH v2] diff: handle NULL filespecs in run_external_diff [not found] ` <xmqq4km4lppy.fsf@gitster.c.googlers.com> @ 2020-11-06 17:02 ` Jinoh Kang 2020-11-06 17:14 ` [PATCH v3] diff: make diff_free_filespec_data accept NULL Jinoh Kang 2020-11-06 19:18 ` [PATCH v2] diff: handle NULL filespecs in run_external_diff Junio C Hamano 0 siblings, 2 replies; 15+ messages in thread From: Jinoh Kang @ 2020-11-06 17:02 UTC (permalink / raw) To: Junio C Hamano, git; +Cc: Johannes Schindelin [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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] diff: make diff_free_filespec_data accept NULL 2020-11-06 17:02 ` [PATCH v2] diff: handle NULL filespecs in run_external_diff Jinoh Kang @ 2020-11-06 17:14 ` Jinoh Kang 2020-11-10 12:08 ` Johannes Schindelin 2020-11-10 14:06 ` [PATCH v4] " Jinoh Kang 2020-11-06 19:18 ` [PATCH v2] diff: handle NULL filespecs in run_external_diff Junio C Hamano 1 sibling, 2 replies; 15+ messages in thread From: Jinoh Kang @ 2020-11-06 17:14 UTC (permalink / raw) To: Junio C Hamano, git; +Cc: Johannes Schindelin 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL 2020-11-06 17:14 ` [PATCH v3] diff: make diff_free_filespec_data accept NULL Jinoh Kang @ 2020-11-10 12:08 ` Johannes Schindelin 2020-11-10 13:16 ` Jinoh Kang ` (2 more replies) 2020-11-10 14:06 ` [PATCH v4] " Jinoh Kang 1 sibling, 3 replies; 15+ messages in thread From: Johannes Schindelin @ 2020-11-10 12:08 UTC (permalink / raw) To: Jinoh Kang; +Cc: Junio C Hamano, git 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL 2020-11-10 12:08 ` Johannes Schindelin @ 2020-11-10 13:16 ` Jinoh Kang 2020-11-10 14:21 ` Jinoh Kang 2020-11-10 17:02 ` Junio C Hamano 2 siblings, 0 replies; 15+ messages in thread From: Jinoh Kang @ 2020-11-10 13:16 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL 2020-11-10 12:08 ` Johannes Schindelin 2020-11-10 13:16 ` Jinoh Kang @ 2020-11-10 14:21 ` Jinoh Kang 2020-11-10 17:02 ` Junio C Hamano 2 siblings, 0 replies; 15+ messages in thread From: Jinoh Kang @ 2020-11-10 14:21 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] diff: make diff_free_filespec_data accept NULL 2020-11-10 12:08 ` Johannes Schindelin 2020-11-10 13:16 ` Jinoh Kang 2020-11-10 14:21 ` Jinoh Kang @ 2020-11-10 17:02 ` Junio C Hamano 2 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2020-11-10 17:02 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Jinoh Kang, Junio C Hamano, git 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4] diff: make diff_free_filespec_data accept NULL 2020-11-06 17:14 ` [PATCH v3] diff: make diff_free_filespec_data accept NULL Jinoh Kang 2020-11-10 12:08 ` Johannes Schindelin @ 2020-11-10 14:06 ` Jinoh Kang 2020-11-10 15:38 ` Johannes Schindelin ` (2 more replies) 1 sibling, 3 replies; 15+ messages in thread From: Jinoh Kang @ 2020-11-10 14:06 UTC (permalink / raw) To: Junio C Hamano, git; +Cc: Johannes Schindelin 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL 2020-11-10 14:06 ` [PATCH v4] " Jinoh Kang @ 2020-11-10 15:38 ` Johannes Schindelin 2020-11-11 12:30 ` Jinoh Kang 2020-11-10 19:41 ` Junio C Hamano 2020-11-11 12:15 ` [PATCH v5] " Jinoh Kang 2 siblings, 1 reply; 15+ messages in thread From: Johannes Schindelin @ 2020-11-10 15:38 UTC (permalink / raw) To: Jinoh Kang; +Cc: Junio C Hamano, git 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL 2020-11-10 15:38 ` Johannes Schindelin @ 2020-11-11 12:30 ` Jinoh Kang 2020-11-11 16:28 ` Johannes Schindelin 0 siblings, 1 reply; 15+ messages in thread From: Jinoh Kang @ 2020-11-11 12:30 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL 2020-11-11 12:30 ` Jinoh Kang @ 2020-11-11 16:28 ` Johannes Schindelin 0 siblings, 0 replies; 15+ messages in thread From: Johannes Schindelin @ 2020-11-11 16:28 UTC (permalink / raw) To: Jinoh Kang; +Cc: Junio C Hamano, git 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] diff: make diff_free_filespec_data accept NULL 2020-11-10 14:06 ` [PATCH v4] " Jinoh Kang 2020-11-10 15:38 ` Johannes Schindelin @ 2020-11-10 19:41 ` Junio C Hamano 2020-11-11 12:15 ` [PATCH v5] " Jinoh Kang 2 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2020-11-10 19:41 UTC (permalink / raw) To: Jinoh Kang; +Cc: git, Johannes Schindelin 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 && ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5] diff: make diff_free_filespec_data accept NULL 2020-11-10 14:06 ` [PATCH v4] " Jinoh Kang 2020-11-10 15:38 ` Johannes Schindelin 2020-11-10 19:41 ` Junio C Hamano @ 2020-11-11 12:15 ` Jinoh Kang 2020-11-11 16:27 ` Johannes Schindelin 2 siblings, 1 reply; 15+ messages in thread From: Jinoh Kang @ 2020-11-11 12:15 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, git 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5] diff: make diff_free_filespec_data accept NULL 2020-11-11 12:15 ` [PATCH v5] " Jinoh Kang @ 2020-11-11 16:27 ` Johannes Schindelin 2020-11-11 19:18 ` Junio C Hamano 0 siblings, 1 reply; 15+ messages in thread From: Johannes Schindelin @ 2020-11-11 16:27 UTC (permalink / raw) To: Jinoh Kang; +Cc: Junio C Hamano, git 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 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5] diff: make diff_free_filespec_data accept NULL 2020-11-11 16:27 ` Johannes Schindelin @ 2020-11-11 19:18 ` Junio C Hamano 0 siblings, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2020-11-11 19:18 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Jinoh Kang, Junio C Hamano, git 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 >> >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] diff: handle NULL filespecs in run_external_diff 2020-11-06 17:02 ` [PATCH v2] diff: handle NULL filespecs in run_external_diff Jinoh Kang 2020-11-06 17:14 ` [PATCH v3] diff: make diff_free_filespec_data accept NULL Jinoh Kang @ 2020-11-06 19:18 ` Junio C Hamano 1 sibling, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2020-11-06 19:18 UTC (permalink / raw) To: Jinoh Kang; +Cc: Junio C Hamano, git, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-11-11 19:18 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <aeb24944-17af-cf53-93f4-e727f9fe9988@theori.io> [not found] ` <xmqq4km4lppy.fsf@gitster.c.googlers.com> 2020-11-06 17:02 ` [PATCH v2] diff: handle NULL filespecs in run_external_diff Jinoh Kang 2020-11-06 17:14 ` [PATCH v3] diff: make diff_free_filespec_data accept NULL Jinoh Kang 2020-11-10 12:08 ` Johannes Schindelin 2020-11-10 13:16 ` Jinoh Kang 2020-11-10 14:21 ` Jinoh Kang 2020-11-10 17:02 ` Junio C Hamano 2020-11-10 14:06 ` [PATCH v4] " Jinoh Kang 2020-11-10 15:38 ` Johannes Schindelin 2020-11-11 12:30 ` Jinoh Kang 2020-11-11 16:28 ` Johannes Schindelin 2020-11-10 19:41 ` Junio C Hamano 2020-11-11 12:15 ` [PATCH v5] " Jinoh Kang 2020-11-11 16:27 ` Johannes Schindelin 2020-11-11 19:18 ` Junio C Hamano 2020-11-06 19:18 ` [PATCH v2] diff: handle NULL filespecs in run_external_diff Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).