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

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

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.

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(-)