threads / patch / 31959

patchmailmap: avoid out-of-bounds memory access

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

## tl;dr

3 messages between Oct 27, 2012 and Oct 28, 2012. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Romain Francoise· Oct 27, 2012, 22:49 UTC · lore

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(-)

Show changes to mailmap.c +1 −1

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

Jeff King· Oct 28, 2012, 11:02 UTC · re: Romain Francoise · lore

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

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

Romain Francoise· Oct 28, 2012, 13:21 UTC · re: Jeff King · lore

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

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!