Re: [PATCH] mailmap: avoid out-of-bounds memory access

From: Jeff King <peff@peff.net>
Date: Oct 28, 2012, 11:02 UTC
Message-ID: <20121028110207.GA11434@sigill.intra.peff.net>
In-Reply-To: <87k3ub4jjg.fsf@silenus.orebokech.com>

On Sun, Oct 28, 2012 at 12:49:55AM +0200, Romain Francoise wrote:

Show 9 quoted lines

> AddressSanitizer (http://clang.llvm.org/docs/AddressSanitizer.html)
> complains of a one-byte buffer underflow in parse_name_and_email() while
> running the test suite. And indeed, if one of the lines in the mailmap
> begins with '<', we dereference the address just before the beginning of
> the buffer when looking for whitespace to remove, before checking that
> we aren't going too far.
> 
> So reverse the order of the tests to make sure that we don't read
> outside the buffer.

Thanks, I think your fix is correct.

Show 11 quoted lines

> diff --git a/mailmap.c b/mailmap.c
> index 47aa419..ea4b471 100644
> --- a/mailmap.c
> +++ b/mailmap.c
> @@ -118,7 +118,7 @@ static char *parse_name_and_email(char *buffer, char **name,
>  	while (isspace(*nstart) && nstart < left)
>  		++nstart;
>  	nend = left-1;
> -	while (isspace(*nend) && nend > nstart)
> +	while (nend > nstart && isspace(*nend))
>  		--nend;

The fix confused me for a moment, because the problem is not actually in the loop condition itself; working backwards from "nend > nstart", we will at worst dereference nstart unnecessarily. The real problem is in the "nend = left-1" above, which sets the loop precondition outside the string to be examined.

So you could also check for "left == nstart" before the loop even begins. I think your fix (to just make the loop more robust to that precondition) is better, though, as the rest of the code does the right thing with such a value of nend.

It looks like t4203 triggers this problem. Curious that valgrind does not find it. I guess since it does not have compiler support, it cannot find out-of-bound errors on stack buffers. Does the rest of the test suite turn up clean with AddressSanitizer?

-Peff