# [PATCH 0/1] difftool: fix a use-after-free bug 3 messages from 2017-04-13 to 2017-04-13. Participants: Johannes Schindelin, Jonathan Nieder. Thread: https://gitlist.dev/t/45684 ## Johannes Schindelin, 2017-04-13 19:21 Subject: [PATCH 0/1] difftool: fix a use-after-free bug Message-ID: URL: https://gitlist.dev/e/cover.1492111288.git.johannes.schindelin%40gmx.de ``` 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 ``` ## Johannes Schindelin, 2017-04-13 19:21 Subject: [PATCH 1/1] difftool: fix use-after-free Message-ID: URL: https://gitlist.dev/e/c8a5e41b77f132272eb9a1dc4142279499a54641.1492111288.git.johannes.schindelin%40gmx.de In-Reply-To: ``` 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 --- 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 ``` ## Jonathan Nieder, 2017-04-13 22:01 Subject: Re: [PATCH 1/1] difftool: fix use-after-free Message-ID: <20170413220128.GD10084@aiede.mtv.corp.google.com> URL: https://gitlist.dev/e/20170413220128.GD10084%40aiede.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin wrote: > Signed-off-by: Johannes Schindelin > --- > builtin/difftool.c | 7 +++++-- > t/t7800-difftool.sh | 19 +++++++++++++++++++ > 2 files changed, 24 insertions(+), 2 deletions(-) Reviewed-by: Jonathan Nieder Thank you. ```