{"thread":{"id":"31959","subject":"[PATCH] mailmap: avoid out-of-bounds memory access","startedAt":"2012-10-27T22:49:55Z","lastAt":"2012-10-28T13:21:33Z","messageCount":3,"participants":["Romain Francoise","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"202015","messageId":"87k3ub4jjg.fsf@silenus.orebokech.com","threadId":"31959","inReplyTo":null,"subject":"[PATCH] mailmap: avoid out-of-bounds memory access","fromName":"Romain Francoise","fromEmail":"romain@orebokech.com","sentAt":"2012-10-27T22:49:55Z","receivedAt":"2012-10-27T22:49:55Z","isPatch":true,"sender":{"key":"romain@orebokech.com","avatar":"https://avatars.githubusercontent.com/u/49197?v=4"},"body":"AddressSanitizer (http://clang.llvm.org/docs/AddressSanitizer.html)\ncomplains of a one-byte buffer underflow in parse_name_and_email() while\nrunning the test suite. And indeed, if one of the lines in the mailmap\nbegins with '<', we dereference the address just before the beginning of\nthe buffer when looking for whitespace to remove, before checking that\nwe aren't going too far.\n\nSo reverse the order of the tests to make sure that we don't read\noutside the buffer.\n\nSigned-off-by: Romain Francoise <romain@orebokech.com>\n---\n mailmap.c |    2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/mailmap.c b/mailmap.c\nindex 47aa419..ea4b471 100644\n--- a/mailmap.c\n+++ b/mailmap.c\n@@ -118,7 +118,7 @@ static char *parse_name_and_email(char *buffer, char **name,\n \twhile (isspace(*nstart) && nstart < left)\n \t\t++nstart;\n \tnend = left-1;\n-\twhile (isspace(*nend) && nend > nstart)\n+\twhile (nend > nstart && isspace(*nend))\n \t\t--nend;\n \n \t*name = (nstart < nend ? nstart : NULL);\n-- \n1.7.10.4\n"},{"id":"202035","messageId":"20121028110207.GA11434@sigill.intra.peff.net","threadId":"31959","inReplyTo":"87k3ub4jjg.fsf@silenus.orebokech.com","subject":"Re: [PATCH] mailmap: avoid out-of-bounds memory access","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-28T11:02:07Z","receivedAt":"2012-10-28T11:02:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 28, 2012 at 12:49:55AM +0200, Romain Francoise wrote:\n\n> AddressSanitizer (http://clang.llvm.org/docs/AddressSanitizer.html)\n> complains of a one-byte buffer underflow in parse_name_and_email() while\n> running the test suite. And indeed, if one of the lines in the mailmap\n> begins with '<', we dereference the address just before the beginning of\n> the buffer when looking for whitespace to remove, before checking that\n> we aren't going too far.\n> \n> So reverse the order of the tests to make sure that we don't read\n> outside the buffer.\n\nThanks, I think your fix is correct.\n\n> diff --git a/mailmap.c b/mailmap.c\n> index 47aa419..ea4b471 100644\n> --- a/mailmap.c\n> +++ b/mailmap.c\n> @@ -118,7 +118,7 @@ static char *parse_name_and_email(char *buffer, char **name,\n>  \twhile (isspace(*nstart) && nstart < left)\n>  \t\t++nstart;\n>  \tnend = left-1;\n> -\twhile (isspace(*nend) && nend > nstart)\n> +\twhile (nend > nstart && isspace(*nend))\n>  \t\t--nend;\n\nThe fix confused me for a moment, because the problem is not actually in\nthe loop condition itself; working backwards from \"nend > nstart\", we\nwill at worst dereference nstart unnecessarily. The real problem is in\nthe \"nend = left-1\" above, which sets the loop precondition outside the\nstring to be examined.\n\nSo you could also check for \"left == nstart\" before the loop even\nbegins. I think your fix (to just make the loop more robust to that\nprecondition) is better, though, as the rest of the code does the right\nthing with such a value of nend.\n\nIt looks like t4203 triggers this problem. Curious that valgrind does\nnot find it. I guess since it does not have compiler support, it cannot\nfind out-of-bound errors on stack buffers. Does the rest of the test\nsuite turn up clean with AddressSanitizer?\n\n-Peff\n"},{"id":"202043","messageId":"87k3ua3f6q.fsf@silenus.orebokech.com","threadId":"31959","inReplyTo":"20121028110207.GA11434@sigill.intra.peff.net","subject":"Re: [PATCH] mailmap: avoid out-of-bounds memory access","fromName":"Romain Francoise","fromEmail":"romain@orebokech.com","sentAt":"2012-10-28T13:21:33Z","receivedAt":"2012-10-28T13:21:33Z","isPatch":true,"sender":{"key":"romain@orebokech.com","avatar":"https://avatars.githubusercontent.com/u/49197?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So you could also check for \"left == nstart\" before the loop even\n> begins. I think your fix (to just make the loop more robust to that\n> precondition) is better, though, as the rest of the code does the right\n> thing with such a value of nend.\n\nYep.\n\n> It looks like t4203 triggers this problem. Curious that valgrind does\n> not find it. I guess since it does not have compiler support, it cannot\n> find out-of-bound errors on stack buffers. Does the rest of the test\n> suite turn up clean with AddressSanitizer?\n\nI tested your 'master' and your 'pu' with expensive tests enabled and both\nare clean after fixing t4203.\n\nThanks!\n"}]}