git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v5] diff: make diff_free_filespec_data accept NULL

From
Jinoh Kang <luke1337@theori.io>
Date
Nov 11, 2020, 12:15 UTC
Message-ID
<5d4315c5-a0ae-2857-fbcc-ec6166d025b6@theori.io>
In-Reply-To
<137f0fc1-fbd9-a62c-bd52-cffd26c364bf@theori.io>

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
Previous: Junio C HamanoNext: Johannes Schindelin
Message 12 of 15 in “diff: handle NULL filespecs in run_external_diff”
  1. diff: handle NULL filespecs in run_external_diffJinoh Kang, Nov 6, 2020
  2. diff: make diff_free_filespec_data accept NULLJinoh Kang, Nov 6, 2020
  3. Johannes SchindelinNov 10, 2020
  4. Jinoh KangNov 10, 2020
  5. Jinoh KangNov 10, 2020
  6. Junio C HamanoNov 10, 2020
  7. diff: make diff_free_filespec_data accept NULLJinoh Kang, Nov 10, 2020
  8. Johannes SchindelinNov 10, 2020
  9. Jinoh KangNov 11, 2020
  10. Johannes SchindelinNov 11, 2020
  11. Junio C HamanoNov 10, 2020
  12. diff: make diff_free_filespec_data accept NULLJinoh Kang, Nov 11, 2020
  13. Johannes SchindelinNov 11, 2020
  14. Junio C HamanoNov 11, 2020
  15. Junio C HamanoNov 6, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.