{"thread":{"id":"53815","subject":"t4068 test failure with -fsanitize=address","startedAt":"2020-07-07T23:54:46Z","lastAt":"2020-07-09T23:02:54Z","messageCount":4,"participants":["brian m. carlson","Jeff King","Chris Torek"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"401167","messageId":"20200707235436.GH9782@camp.crustytoothpaste.net","threadId":"53815","inReplyTo":null,"subject":"t4068 test failure with -fsanitize=address","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-07-07T23:54:36Z","receivedAt":"2020-07-07T23:54:46Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"I was kicking off a build on a cloud instance for the final series of my\nSHA-256 work and I noticed that t4068 fails with -fsanitize=address\nusing Debian's GCC 9.3.0-14 (the version in unstable).  It has failed\nsince the test was introduced and currently fails on the latest HEAD.\n\nI haven't had time to look at it, but I thought I'd pass the information\non so we can get it fixed before the release.  I'll take a look if\nnobody else gets to it first, but it may be a few days.\n\nReproduction steps:\n\n  printf 'DEVELOPER=1\\nDC_SHA1=1\\nBLK_SHA256=1\\nSANITIZE=address\\n' >config.mak\n  make -j6 all && (cd t && ./t4068-*.sh --verbose)\n\nOutput with --verbose:\n\nexpecting success of 4068.4 'diff with no merge bases':\n        test_must_fail git diff br2...br3 >tmp 2>err &&\n        test_i18ngrep \"fatal: br2...br3: no merge base\" err\n\ntest_must_fail: died by signal 6: git diff br2...br3\nnot ok 4 - diff with no merge bases\n#\n#               test_must_fail git diff br2...br3 >tmp 2>err &&\n#               test_i18ngrep \"fatal: br2...br3: no merge base\" err\n#\n-- \nbrian m. carlson: Houston, Texas, US\n"},{"id":"401175","messageId":"20200708043819.GA2303891@coredump.intra.peff.net","threadId":"53815","inReplyTo":"20200707235436.GH9782@camp.crustytoothpaste.net","subject":"[PATCH] diff: check for merge bases before assigning sym->base","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-08T04:38:19Z","receivedAt":"2020-07-08T04:38:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 07, 2020 at 11:54:36PM +0000, brian m. carlson wrote:\n\n> I was kicking off a build on a cloud instance for the final series of my\n> SHA-256 work and I noticed that t4068 fails with -fsanitize=address\n> using Debian's GCC 9.3.0-14 (the version in unstable).  It has failed\n> since the test was introduced and currently fails on the latest HEAD.\n\nThanks for reporting. I occasionally do an ASan (and UBSan) run, but I\nhaven't in a while. It's definitely good to catch this before the\nrelease.\n\n> Reproduction steps:\n> \n>   printf 'DEVELOPER=1\\nDC_SHA1=1\\nBLK_SHA256=1\\nSANITIZE=address\\n' >config.mak\n>   make -j6 all && (cd t && ./t4068-*.sh --verbose)\n> \n> Output with --verbose:\n> \n> expecting success of 4068.4 'diff with no merge bases':\n>         test_must_fail git diff br2...br3 >tmp 2>err &&\n>         test_i18ngrep \"fatal: br2...br3: no merge base\" err\n> \n> test_must_fail: died by signal 6: git diff br2...br3\n> not ok 4 - diff with no merge bases\n> #\n> #               test_must_fail git diff br2...br3 >tmp 2>err &&\n> #               test_i18ngrep \"fatal: br2...br3: no merge base\" err\n> #\n\nThe goodies are in \"err\", which shows:\n\n  $ cat trash\\ directory.t4068-diff-symmetric/err\n  ==2319726==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d000000068 at pc 0x556a45b790b4 bp 0x7fff59d414f0 sp 0x7fff59d414e8\n  READ of size 8 at 0x61d000000068 thread T0\n      #0 0x556a45b790b3 in symdiff_prepare builtin/diff.c:358\n      #1 0x556a45b7a1a2 in cmd_diff builtin/diff.c:509\n      [...etc...]\n\nThe offending line is:\n\n     sym->base = rev->pending.objects[basepos].name;\n\nand indeed, if we instrument it like so:\n\n  diff --git a/builtin/diff.c b/builtin/diff.c\n  index 8c36da09b6..698e81e01b 100644\n  --- a/builtin/diff.c\n  +++ b/builtin/diff.c\n  @@ -292,6 +292,9 @@ static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n   \tint lpos = -1, rpos = -1, basepos = -1;\n   \tstruct bitmap *map = NULL;\n   \n  +\ttrace_printf(\"cmdline.nr = %d, pending.nr = %d\",\n  +\t\t     rev->cmdline.nr, rev->pending.nr);\n  +\n   \t/*\n   \t * Use the whence fields to find merge bases and left and\n   \t * right parts of symmetric difference, so that we do not\n  @@ -353,6 +356,8 @@ static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n   \t\treturn;\n   \t}\n   \n  +\ttrace_printf(\"lpos = %d, rpos = %d, basepos = %d\",\n  +\t\t     lpos, rpos, basepos);\n   \tsym->left = rev->pending.objects[lpos].name;\n   \tsym->right = rev->pending.objects[rpos].name;\n   \tsym->base = rev->pending.objects[basepos].name;\n\nI get:\n\n  $ GIT_TRACE=1 ./t4068-diff-symmetric.sh -v -i\n  [...]\n  trace: built-in: git diff br2...br3\n  cmdline.nr = 2, pending.nr = 2\n  lpos = 0, rpos = 1, basepos = -1\n  test_must_fail: died by signal 6: git diff br2...br3\n  not ok 4 - diff with no merge bases\n\nI suspect the solution is this:\n\n-- >8 --\nSubject: diff: check for merge bases before assigning sym->base\n\nIn symdiff_prepare(), we iterate over the set of parsed objects to pick\nout any symmetric differences, including the left, right, and base\nelements. We assign the results into pointers in a \"struct symdiff\", and\nthen complain if we didn't find a base, like so:\n\n    sym->left = rev->pending.objects[lpos].name;\n    sym->right = rev->pending.objects[rpos].name;\n    sym->base = rev->pending.objects[basepos].name;\n    if (basecount == 0)\n            die(_(\"%s...%s: no merge base\"), sym->left, sym->right);\n\nBut the least lines are backwards. If basecount is 0, then basepos will\nbe -1, and we will access memory outside of the pending array. This\nisn't usually that big a deal, since we don't do anything besides a\nsingle pointer-sized read before exiting anyway, but it does violate the\nC standard, and of course memory-checking tools like ASan complain.\n\nLet's put the basecount check first. Note that we haveto split it from\nthe other assignments, since the die() relies on sym->left and\nsym->right having been assigned (this isn't strictly necessary, but is\neasier to read than dereferencing the pending array again).\n\nReported-by: brian m. carlson <sandals@crustytoothpaste.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI don't see anything within this function guaranteeing that rpos is set,\neither, though I suspect it is OK due to how the revision parser works.\nWe have to see a left-side in order to set is_symdiff, and if we don't\ndo that, we'd bail before hitting this code. But if we saw REV_CMD_LEFT\nbut not REV_CMD_RIGHT, we'd have lpos set to something valid and rpos\nnot. It might be worth adding an assert() or BUG() to document and check\nour assumption there.\n\n builtin/diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 8c36da09b6..cb98811c21 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -355,9 +355,9 @@ static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n \n \tsym->left = rev->pending.objects[lpos].name;\n \tsym->right = rev->pending.objects[rpos].name;\n-\tsym->base = rev->pending.objects[basepos].name;\n \tif (basecount == 0)\n \t\tdie(_(\"%s...%s: no merge base\"), sym->left, sym->right);\n+\tsym->base = rev->pending.objects[basepos].name;\n \tbitmap_unset(map, basepos);\t/* unmark the base we want */\n \tsym->warn = basecount > 1;\n \tsym->skip = map;\n-- \n2.27.0.730.gcc195a083d\n"},{"id":"401191","messageId":"CAPx1GvfLpTR-weVQcWm5iX59cmCh2Gad+vU=ESqqLkCq-O5RmA@mail.gmail.com","threadId":"53815","inReplyTo":"20200708043819.GA2303891@coredump.intra.peff.net","subject":"Re: [PATCH] diff: check for merge bases before assigning sym->base","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-07-08T17:05:44Z","receivedAt":"2020-07-08T17:05:58Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Tue, Jul 7, 2020 at 9:38 PM Jeff King <peff@peff.net> wrote:\n> I suspect the solution is [to call die one line earlier]:\n\nOops, yes.\n\n> I don't see anything within this function guaranteeing that rpos is set,\n> either, though I suspect it is OK due to how the revision parser works.\n\nYes, I am depending on the revision parser to balance the left and\nright items.  I thought about having a BUG check but it seemed\nunnecessary.  On the other hand, I'm avoiding depending on its\n*placement* of left and right items.  Being more defensive might\nbe good here.\n\n(I had a version of this that also implemented Junio's suggestion\nof not first marking all bases, then unmarking the chosen base,\nbut in the end it came out to the same amount of code.  I could\nresurrect that if anyone cares.)\n\nChris\n"},{"id":"401275","messageId":"20200709230251.GC664420@coredump.intra.peff.net","threadId":"53815","inReplyTo":"CAPx1GvfLpTR-weVQcWm5iX59cmCh2Gad+vU=ESqqLkCq-O5RmA@mail.gmail.com","subject":"Re: [PATCH] diff: check for merge bases before assigning sym->base","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-09T23:02:51Z","receivedAt":"2020-07-09T23:02:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 08, 2020 at 10:05:44AM -0700, Chris Torek wrote:\n\n> > I don't see anything within this function guaranteeing that rpos is set,\n> > either, though I suspect it is OK due to how the revision parser works.\n> \n> Yes, I am depending on the revision parser to balance the left and\n> right items.  I thought about having a BUG check but it seemed\n> unnecessary.  On the other hand, I'm avoiding depending on its\n> *placement* of left and right items.  Being more defensive might\n> be good here.\n\nI could go either way with adding a BUG (and it definitely doesn't need\nto hold up this fix). I strongly suspect that getting a LEFT without a\nRIGHT would be a bug in the rev parser, not this code. But seeing a\nBUG() here might be a more friendly way of being informed of that than a\nsegfault. :)\n\n-Peff\n"}]}