{"thread":{"id":"45684","subject":"[PATCH 0/1] difftool: fix a use-after-free bug","startedAt":"2017-04-13T19:22:10Z","lastAt":"2017-04-13T22:01:37Z","messageCount":3,"participants":["Johannes Schindelin","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"316771","messageId":"cover.1492111288.git.johannes.schindelin@gmx.de","threadId":"45684","inReplyTo":null,"subject":"[PATCH 0/1] difftool: fix a use-after-free bug","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-13T19:21:53Z","receivedAt":"2017-04-13T19:22:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It has been reported previously that the base_dir recorded at the\nbeginning of run_dir_diff() may go stale due to the buffer to which it\npoints potentially being realloc()ed.\n\nThis bug has been fixed in Git for Windows 2.12.2(2) already. It took me\nthis long (!!!) to come up with a reliable test case... But now that I\nhave it, it can be easily verified.\n\n\nJohannes Schindelin (1):\n difftool: fix use-after-free\n\n builtin/difftool.c | 7 +++++--\n t/t7800-difftool.sh | 19 +++++++++++++++++++\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\n\nbase-commit: cf11a67975b057a144618badf16dc4e3d25b9407\nPublished-As: https://github.com/dscho/git/releases/tag/fix-difftool-d-crash-v1\nFetch-It-Via: git fetch https://github.com/dscho/git fix-difftool-d-crash-v1\n\n-- \n2.12.2.windows.2.1.g7df5db8d31e\n\n"},{"id":"316772","messageId":"c8a5e41b77f132272eb9a1dc4142279499a54641.1492111288.git.johannes.schindelin@gmx.de","threadId":"45684","inReplyTo":"cover.1492111288.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/1] difftool: fix use-after-free","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-13T19:21:58Z","receivedAt":"2017-04-13T19:22:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The left and right base directories were pointed to the buf field of\ntwo strbufs, which were subject to change.\n\nA contrived test case shows the problem where a file with a long enough\nname to force the strbuf to grow is up-to-date (hence the code path is\nused where the work tree's version of the file is reused), and then a\nfile that is not up-to-date needs to be written (hence the code path is\nused where checkout_entry() uses the previously recorded base_dir that\nis invalid by now).\n\nLet's just copy the base_dir strings for use with checkout_entry(),\nnever touch them until the end, and release them then. This is an easily\nverifiable fix (as opposed to the next-obvious alternative: to re-set\nbase_dir after every loop iteration).\n\nThis fixes https://github.com/git-for-windows/git/issues/1124\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/difftool.c | 7 +++++--\n t/t7800-difftool.sh | 19 +++++++++++++++++++\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 25e54ad3edb..568294aac01 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -305,6 +305,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;\n \tstruct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;\n \tstruct strbuf wtdir = STRBUF_INIT;\n+\tchar *lbase_dir, *rbase_dir;\n \tsize_t ldir_len, rdir_len, wtdir_len;\n \tstruct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);\n \tconst char *workdir, *tmp;\n@@ -339,11 +340,11 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tmemset(&wtindex, 0, sizeof(wtindex));\n \n \tmemset(&lstate, 0, sizeof(lstate));\n-\tlstate.base_dir = ldir.buf;\n+\tlstate.base_dir = lbase_dir = xstrdup(ldir.buf);\n \tlstate.base_dir_len = ldir.len;\n \tlstate.force = 1;\n \tmemset(&rstate, 0, sizeof(rstate));\n-\trstate.base_dir = rdir.buf;\n+\trstate.base_dir = rbase_dir = xstrdup(rdir.buf);\n \trstate.base_dir_len = rdir.len;\n \trstate.force = 1;\n \n@@ -626,6 +627,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \n finish:\n \tfree(ce);\n+\tfree(lbase_dir);\n+\tfree(rbase_dir);\n \tstrbuf_release(&ldir);\n \tstrbuf_release(&rdir);\n \tstrbuf_release(&wtdir);\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 0e7f30db2d9..7f09867478c 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -393,6 +393,25 @@ test_expect_success 'setup change in subdirectory' '\n \tgit commit -m \"modified both\"\n '\n \n+test_expect_success 'difftool -d with growing paths' '\n+\ta=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n+\tgit init growing &&\n+\t(\n+\t\tcd growing &&\n+\t\techo \"test -f \\\"\\$2/b\\\"\" | write_script .git/test-for-b.sh &&\n+\t\tone=$(printf 1 | git hash-object -w --stdin) &&\n+\t\ttwo=$(printf 2 | git hash-object -w --stdin) &&\n+\t\tgit update-index --add \\\n+\t\t\t--cacheinfo 100644,$one,$a --cacheinfo 100644,$two,b &&\n+\t\ttree1=$(git write-tree) &&\n+\t\tgit update-index --add \\\n+\t\t\t--cacheinfo 100644,$two,$a --cacheinfo 100644,$one,b &&\n+\t\ttree2=$(git write-tree) &&\n+\t\tgit checkout -- $a &&\n+\t\tgit difftool -d --extcmd .git/test-for-b.sh $tree1 $tree2\n+\t)\n+'\n+\n run_dir_diff_test () {\n \ttest_expect_success \"$1 --no-symlinks\" \"\n \t\tsymlinks=--no-symlinks &&\n-- \n2.12.2.windows.2.1.g7df5db8d31e\n"},{"id":"316791","messageId":"20170413220128.GD10084@aiede.mtv.corp.google.com","threadId":"45684","inReplyTo":"c8a5e41b77f132272eb9a1dc4142279499a54641.1492111288.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/1] difftool: fix use-after-free","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-13T22:01:28Z","receivedAt":"2017-04-13T22:01:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Schindelin wrote:\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/difftool.c | 7 +++++--\n> t/t7800-difftool.sh | 19 +++++++++++++++++++\n> 2 files changed, 24 insertions(+), 2 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\nThank you.\n"}]}