t4068 test failure with -fsanitize=address

t4068 test failure with -fsanitize=address

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

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.

I haven't had time to look at it, but I thought I'd pass the information
on so we can get it fixed before the release.  I'll take a look if
nobody else gets to it first, but it may be a few days.

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
#
-- 
brian m. carlson: Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

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

[Jeff King]

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

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

[Chris Torek]

On Tue, Jul 7, 2020 at 9:38 PM Jeff King <peff@peff.net> wrote:
> I suspect the solution is [to call die one line earlier]:

Oops, yes.

> 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.

Yes, I am depending on the revision parser to balance the left and
right items.  I thought about having a BUG check but it seemed
unnecessary.  On the other hand, I'm avoiding depending on its
*placement* of left and right items.  Being more defensive might
be good here.

(I had a version of this that also implemented Junio's suggestion
of not first marking all bases, then unmarking the chosen base,
but in the end it came out to the same amount of code.  I could
resurrect that if anyone cares.)

Chris

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

[Jeff King]

On Wed, Jul 08, 2020 at 10:05:44AM -0700, Chris Torek wrote:

> > 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.
> 
> Yes, I am depending on the revision parser to balance the left and
> right items.  I thought about having a BUG check but it seemed
> unnecessary.  On the other hand, I'm avoiding depending on its
> *placement* of left and right items.  Being more defensive might
> be good here.

I could go either way with adding a BUG (and it definitely doesn't need
to hold up this fix). I strongly suspect that getting a LEFT without a
RIGHT would be a bug in the rev parser, not this code. But seeing a
BUG() here might be a more friendly way of being informed of that than a
segfault. :)

-Peff