[PATCH 0/1] difftool: fix a use-after-free bug

[PATCH 0/1] difftool: fix a use-after-free bug

[Johannes Schindelin]

It has been reported previously that the base_dir recorded at the
beginning of run_dir_diff() may go stale due to the buffer to which it
points potentially being realloc()ed.

This bug has been fixed in Git for Windows 2.12.2(2) already. It took me
this long (!!!) to come up with a reliable test case... But now that I
have it, it can be easily verified.


Johannes Schindelin (1):
  difftool: fix use-after-free

 builtin/difftool.c  |  7 +++++--
 t/t7800-difftool.sh | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)


base-commit: cf11a67975b057a144618badf16dc4e3d25b9407
Published-As: https://github.com/dscho/git/releases/tag/fix-difftool-d-crash-v1
Fetch-It-Via: git fetch https://github.com/dscho/git fix-difftool-d-crash-v1

-- 
2.12.2.windows.2.1.g7df5db8d31e

[PATCH 1/1] difftool: fix use-after-free

[Johannes Schindelin]

The left and right base directories were pointed to the buf field of
two strbufs, which were subject to change.

A contrived test case shows the problem where a file with a long enough
name to force the strbuf to grow is up-to-date (hence the code path is
used where the work tree's version of the file is reused), and then a
file that is not up-to-date needs to be written (hence the code path is
used where checkout_entry() uses the previously recorded base_dir that
is invalid by now).

Let's just copy the base_dir strings for use with checkout_entry(),
never touch them until the end, and release them then. This is an easily
verifiable fix (as opposed to the next-obvious alternative: to re-set
base_dir after every loop iteration).

This fixes https://github.com/git-for-windows/git/issues/1124

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/difftool.c  |  7 +++++--
 t/t7800-difftool.sh | 19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 25e54ad3edb..568294aac01 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -305,6 +305,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
 	struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
 	struct strbuf wtdir = STRBUF_INIT;
+	char *lbase_dir, *rbase_dir;
 	size_t ldir_len, rdir_len, wtdir_len;
 	struct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);
 	const char *workdir, *tmp;
@@ -339,11 +340,11 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	memset(&wtindex, 0, sizeof(wtindex));
 
 	memset(&lstate, 0, sizeof(lstate));
-	lstate.base_dir = ldir.buf;
+	lstate.base_dir = lbase_dir = xstrdup(ldir.buf);
 	lstate.base_dir_len = ldir.len;
 	lstate.force = 1;
 	memset(&rstate, 0, sizeof(rstate));
-	rstate.base_dir = rdir.buf;
+	rstate.base_dir = rbase_dir = xstrdup(rdir.buf);
 	rstate.base_dir_len = rdir.len;
 	rstate.force = 1;
 
@@ -626,6 +627,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 
 finish:
 	free(ce);
+	free(lbase_dir);
+	free(rbase_dir);
 	strbuf_release(&ldir);
 	strbuf_release(&rdir);
 	strbuf_release(&wtdir);
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 0e7f30db2d9..7f09867478c 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -393,6 +393,25 @@ test_expect_success 'setup change in subdirectory' '
 	git commit -m "modified both"
 '
 
+test_expect_success 'difftool -d with growing paths' '
+	a=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
+	git init growing &&
+	(
+		cd growing &&
+		echo "test -f \"\$2/b\"" | write_script .git/test-for-b.sh &&
+		one=$(printf 1 | git hash-object -w --stdin) &&
+		two=$(printf 2 | git hash-object -w --stdin) &&
+		git update-index --add \
+			--cacheinfo 100644,$one,$a --cacheinfo 100644,$two,b &&
+		tree1=$(git write-tree) &&
+		git update-index --add \
+			--cacheinfo 100644,$two,$a --cacheinfo 100644,$one,b &&
+		tree2=$(git write-tree) &&
+		git checkout -- $a &&
+		git difftool -d --extcmd .git/test-for-b.sh $tree1 $tree2
+	)
+'
+
 run_dir_diff_test () {
 	test_expect_success "$1 --no-symlinks" "
 		symlinks=--no-symlinks &&
-- 
2.12.2.windows.2.1.g7df5db8d31e

Re: [PATCH 1/1] difftool: fix use-after-free

[Jonathan Nieder]

Johannes Schindelin wrote:

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/difftool.c  |  7 +++++--
>  t/t7800-difftool.sh | 19 +++++++++++++++++++
>  2 files changed, 24 insertions(+), 2 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thank you.