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

[Jeff King <peff@peff.net>]

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

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

> 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