From 0730dd4ffb39358f30b1a956cd9182aed1958b47 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 30 Mar 2017 06:35:50 -0400 Subject: [PATCH 1/2] difftool: avoid strcpy In order to checkout files, difftool reads "diff --raw" output and feeds the names to checkout_entry(). That function requires us to have a "struct cache_entry". And because that struct uses a FLEX_ARRAY for the name field, we have to actually copy in our new name. The current code allocates a single re-usable cache_entry that can hold a name up to PATH_MAX, and then copies filenames into it using strcpy(). But there's no guarantee that incoming names are smaller than PATH_MAX. They've come from "diff --raw" output which might be diffing between two trees (and hence we'd be subject to the PATH_MAX of some other system, or even none at all if they were created directly via "update-index"). We can fix this by using make_cache_entry() to create a correctly-sized cache_entry for each name. This incurs an extra allocation per file, but this is negligible compared to actually writing out the file contents. To make this simpler, we can push this procedure into a new helper function. Note that we can also get rid of the "len" variables for src_path and dst_path (and in fact we must, as the compiler complains that they are unused). Signed-off-by: Jeff King Acked-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/difftool.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index 25e54ad3ed..b350b3d397 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -297,6 +297,19 @@ static char *get_symlink(const struct object_id *oid, const char *path) return data; } +static int checkout_path(unsigned mode, struct object_id *oid, + const char *path, const struct checkout *state) +{ + struct cache_entry *ce; + int ret; + + ce = make_cache_entry(mode, oid->hash, path, 0, 0); + ret = checkout_entry(ce, state, NULL); + + free(ce); + return ret; +} + static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, int argc, const char **argv) { @@ -306,7 +319,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT; struct strbuf wtdir = STRBUF_INIT; size_t ldir_len, rdir_len, wtdir_len; - struct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1); const char *workdir, *tmp; int ret = 0, i; FILE *fp; @@ -377,7 +389,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, struct object_id loid, roid; char status; const char *src_path, *dst_path; - size_t src_path_len, dst_path_len; if (starts_with(info.buf, "::")) die(N_("combined diff formats('-c' and '--cc') are " @@ -390,17 +401,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, if (strbuf_getline_nul(&lpath, fp)) break; src_path = lpath.buf; - src_path_len = lpath.len; i++; if (status != 'C' && status != 'R') { dst_path = src_path; - dst_path_len = src_path_len; } else { if (strbuf_getline_nul(&rpath, fp)) break; dst_path = rpath.buf; - dst_path_len = rpath.len; } if (S_ISGITLINK(lmode) || S_ISGITLINK(rmode)) { @@ -430,11 +438,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, } if (lmode && status != 'C') { - ce->ce_mode = lmode; - oidcpy(&ce->oid, &loid); - strcpy(ce->name, src_path); - ce->ce_namelen = src_path_len; - if (checkout_entry(ce, &lstate, NULL)) + if (checkout_path(lmode, &loid, src_path, &lstate)) return error("could not write '%s'", src_path); } @@ -451,11 +455,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, hashmap_add(&working_tree_dups, entry); if (!use_wt_file(workdir, dst_path, &roid)) { - ce->ce_mode = rmode; - oidcpy(&ce->oid, &roid); - strcpy(ce->name, dst_path); - ce->ce_namelen = dst_path_len; - if (checkout_entry(ce, &rstate, NULL)) + if (checkout_path(rmode, &roid, dst_path, &rstate)) return error("could not write '%s'", dst_path); } else if (!is_null_oid(&roid)) { @@ -625,7 +625,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, exit_cleanup(tmpdir, rc); finish: - free(ce); strbuf_release(&ldir); strbuf_release(&rdir); strbuf_release(&wtdir); From 882add136fa8319832ef373b8797ef58edb80efc Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 13 Apr 2017 21:21:58 +0200 Subject: [PATCH 2/2] difftool: fix use-after-free 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 Reviewed-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- 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 b350b3d397..1354d0e462 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -318,6 +318,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; const char *workdir, *tmp; int ret = 0, i; @@ -351,11 +352,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; @@ -625,6 +626,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, exit_cleanup(tmpdir, rc); finish: + 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 0e7f30db2d..7f09867478 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 &&