@matheusbsbs / matheus-bagliano / commits / 882add136f

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 <johannes.schindelin@gmx.de> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Johannes Schindelin committed Apr 13, 2017 at 21:21 UTC 882add136fa8319832ef373b8797ef58edb80efc

2 files changed +24 -2

builtin/difftool.c

+5 -2

@@ -318,6 +318,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,

318 struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;

319 struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;

320 struct strbuf wtdir = STRBUF_INIT;

321 + char *lbase_dir, *rbase_dir;

322 size_t ldir_len, rdir_len, wtdir_len;

323 const char *workdir, *tmp;

324 int ret = 0, i;

@@ -351,11 +352,11 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,

352 memset(&wtindex, 0, sizeof(wtindex));

353

354 memset(&lstate, 0, sizeof(lstate));

354 - lstate.base_dir = ldir.buf;

355 + lstate.base_dir = lbase_dir = xstrdup(ldir.buf);

356 lstate.base_dir_len = ldir.len;

357 lstate.force = 1;

358 memset(&rstate, 0, sizeof(rstate));

358 - rstate.base_dir = rdir.buf;

359 + rstate.base_dir = rbase_dir = xstrdup(rdir.buf);

360 rstate.base_dir_len = rdir.len;

361 rstate.force = 1;

362

@@ -625,6 +626,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,

626 exit_cleanup(tmpdir, rc);

627

628 finish:

629 + free(lbase_dir);

630 + free(rbase_dir);

631 strbuf_release(&ldir);

632 strbuf_release(&rdir);

633 strbuf_release(&wtdir);

t/t7800-difftool.sh

+19

@@ -393,6 +393,25 @@ test_expect_success 'setup change in subdirectory' '

393 git commit -m "modified both"

394 '

395

396 +test_expect_success 'difftool -d with growing paths' '

397 + a=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&

398 + git init growing &&

399 + (

400 + cd growing &&

401 + echo "test -f \"\$2/b\"" | write_script .git/test-for-b.sh &&

402 + one=$(printf 1 | git hash-object -w --stdin) &&

403 + two=$(printf 2 | git hash-object -w --stdin) &&

404 + git update-index --add \

405 + --cacheinfo 100644,$one,$a --cacheinfo 100644,$two,b &&

406 + tree1=$(git write-tree) &&

407 + git update-index --add \

408 + --cacheinfo 100644,$two,$a --cacheinfo 100644,$one,b &&

409 + tree2=$(git write-tree) &&

410 + git checkout -- $a &&

411 + git difftool -d --extcmd .git/test-for-b.sh $tree1 $tree2

412 + )

413 +'

414 +

415 run_dir_diff_test () {

416 test_expect_success "$1 --no-symlinks" "

417 symlinks=--no-symlinks &&