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

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

[Romain Francoise]

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.

Signed-off-by: Romain Francoise <romain@orebokech.com>
---
 mailmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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;
 
 	*name = (nstart < nend ? nstart : NULL);
-- 
1.7.10.4

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

[Jeff King]

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

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

[Romain Francoise]

Jeff King <peff@peff.net> writes:

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

Yep.

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

I tested your 'master' and your 'pu' with expensive tests enabled and both
are clean after fixing t4203.

Thanks!