[PATCH] diff: check for merge bases before assigning sym->base

[Jeff King <peff@peff.net>]

On Tue, Jul 07, 2020 at 11:54:36PM +0000, brian m. carlson wrote:

> I was kicking off a build on a cloud instance for the final series of my
> SHA-256 work and I noticed that t4068 fails with -fsanitize=address
> using Debian's GCC 9.3.0-14 (the version in unstable).  It has failed
> since the test was introduced and currently fails on the latest HEAD.

Thanks for reporting. I occasionally do an ASan (and UBSan) run, but I
haven't in a while. It's definitely good to catch this before the
release.

> Reproduction steps:
> 
>   printf 'DEVELOPER=1\nDC_SHA1=1\nBLK_SHA256=1\nSANITIZE=address\n' >config.mak
>   make -j6 all && (cd t && ./t4068-*.sh --verbose)
> 
> Output with --verbose:
> 
> expecting success of 4068.4 'diff with no merge bases':
>         test_must_fail git diff br2...br3 >tmp 2>err &&
>         test_i18ngrep "fatal: br2...br3: no merge base" err
> 
> test_must_fail: died by signal 6: git diff br2...br3
> not ok 4 - diff with no merge bases
> #
> #               test_must_fail git diff br2...br3 >tmp 2>err &&
> #               test_i18ngrep "fatal: br2...br3: no merge base" err
> #

The goodies are in "err", which shows:

  $ cat trash\ directory.t4068-diff-symmetric/err
  ==2319726==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d000000068 at pc 0x556a45b790b4 bp 0x7fff59d414f0 sp 0x7fff59d414e8
  READ of size 8 at 0x61d000000068 thread T0
      #0 0x556a45b790b3 in symdiff_prepare builtin/diff.c:358
      #1 0x556a45b7a1a2 in cmd_diff builtin/diff.c:509
      [...etc...]

The offending line is:

     sym->base = rev->pending.objects[basepos].name;

and indeed, if we instrument it like so:

  diff --git a/builtin/diff.c b/builtin/diff.c
  index 8c36da09b6..698e81e01b 100644
  --- a/builtin/diff.c
  +++ b/builtin/diff.c
  @@ -292,6 +292,9 @@ static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)
   	int lpos = -1, rpos = -1, basepos = -1;
   	struct bitmap *map = NULL;
   
  +	trace_printf("cmdline.nr = %d, pending.nr = %d",
  +		     rev->cmdline.nr, rev->pending.nr);
  +
   	/*
   	 * Use the whence fields to find merge bases and left and
   	 * right parts of symmetric difference, so that we do not
  @@ -353,6 +356,8 @@ static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)
   		return;
   	}
   
  +	trace_printf("lpos = %d, rpos = %d, basepos = %d",
  +		     lpos, rpos, basepos);
   	sym->left = rev->pending.objects[lpos].name;
   	sym->right = rev->pending.objects[rpos].name;
   	sym->base = rev->pending.objects[basepos].name;

I get:

  $ GIT_TRACE=1 ./t4068-diff-symmetric.sh -v -i
  [...]
  trace: built-in: git diff br2...br3
  cmdline.nr = 2, pending.nr = 2
  lpos = 0, rpos = 1, basepos = -1
  test_must_fail: died by signal 6: git diff br2...br3
  not ok 4 - diff with no merge bases

I suspect the solution is this:

-- >8 --
Subject: diff: check for merge bases before assigning sym->base

In symdiff_prepare(), we iterate over the set of parsed objects to pick
out any symmetric differences, including the left, right, and base
elements. We assign the results into pointers in a "struct symdiff", and
then complain if we didn't find a base, like so:

    sym->left = rev->pending.objects[lpos].name;
    sym->right = rev->pending.objects[rpos].name;
    sym->base = rev->pending.objects[basepos].name;
    if (basecount == 0)
            die(_("%s...%s: no merge base"), sym->left, sym->right);

But the least lines are backwards. If basecount is 0, then basepos will
be -1, and we will access memory outside of the pending array. This
isn't usually that big a deal, since we don't do anything besides a
single pointer-sized read before exiting anyway, but it does violate the
C standard, and of course memory-checking tools like ASan complain.

Let's put the basecount check first. Note that we haveto split it from
the other assignments, since the die() relies on sym->left and
sym->right having been assigned (this isn't strictly necessary, but is
easier to read than dereferencing the pending array again).

Reported-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Jeff King <peff@peff.net>
---
I don't see anything within this function guaranteeing that rpos is set,
either, though I suspect it is OK due to how the revision parser works.
We have to see a left-side in order to set is_symdiff, and if we don't
do that, we'd bail before hitting this code. But if we saw REV_CMD_LEFT
but not REV_CMD_RIGHT, we'd have lpos set to something valid and rpos
not. It might be worth adding an assert() or BUG() to document and check
our assumption there.

 builtin/diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 8c36da09b6..cb98811c21 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -355,9 +355,9 @@ static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)
 
 	sym->left = rev->pending.objects[lpos].name;
 	sym->right = rev->pending.objects[rpos].name;
-	sym->base = rev->pending.objects[basepos].name;
 	if (basecount == 0)
 		die(_("%s...%s: no merge base"), sym->left, sym->right);
+	sym->base = rev->pending.objects[basepos].name;
 	bitmap_unset(map, basepos);	/* unmark the base we want */
 	sym->warn = basecount > 1;
 	sym->skip = map;
-- 
2.27.0.730.gcc195a083d