Re: Problem listing GIT repository with accents

[Jeff King <peff@peff.net>]

On Mon, Feb 01, 2010 at 01:48:01PM +0100, Johannes Sixt wrote:

> Jeff King schrieb:
> > @@ -209,11 +209,14 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
> >  	size_t len, count = 0;
> >  	const char *p = name;
> >  
> > +	if (maxlen < 0)
> > +		maxlen = strlen(name);
> > +
> >  	for (;;) {
> >  		int ch;
> >  
> >  		len = next_quote_pos(p, maxlen);
> > -		if (len == maxlen || !p[len])
> > +		if (len == maxlen)
> >  			break;
> >  
> >  		if (!no_dq && p == name)
> > @@ -223,6 +226,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
> >  		EMIT('\\');
> >  		p += len;
> >  		ch = (unsigned char)*p++;
> > +		maxlen -= len + 1;
> 
> Couldn't you just write
> 
> 		if (0 <= maxlen)
> 			maxlen -= len + 1;
> 
> to avoid the strlen(), because the rest of the loop is actually OK when
> maxlen == -1, isn't it?

If you want to keep the "!p[len]" condition, then yes. If we want to
properly quote internal NULs (which again, I am not sure is of practical
use), then you would also have to make that condition "(maxlen >= 0 &&
!p[len])".  Which is really not that bad, but I was trying to make it
easier to read. I am OK with any of the three combinations of fixes.

And the fact that I am using the word "combination" probably means it
should be a separate patch anyway.

So here is the minimal patch to go on 'maint', for Junio's convenience.

-- >8 --
Subject: [PATCH] fix invalid read in quote_c_style_counted

This function did not work on strings that were not
NUL-terminated. It reads through a length-bounded string,
searching for characters in need of quoting. After we find
one, we output the quoted character, then advance our
pointer to find the next one. However, we never decremented
the length, meaning we ended up looking at whatever random
junk was stored after the string.

This bug was not found by the existing tests because most
code paths feed a NUL-terminated string. The notable
exception is a directory name being fed by ls-tree.

Signed-off-by: Jeff King <peff@peff.net>
---
 quote.c           |    2 ++
 t/t3902-quoted.sh |   19 ++++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/quote.c b/quote.c
index acb6bf9..723bb4f 100644
--- a/quote.c
+++ b/quote.c
@@ -223,6 +223,8 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,
 		EMIT('\\');
 		p += len;
 		ch = (unsigned char)*p++;
+		if (maxlen >= 0)
+			maxlen -= len + 1;
 		if (sq_lookup[ch] >= ' ') {
 			EMIT(sq_lookup[ch]);
 		} else {
diff --git a/t/t3902-quoted.sh b/t/t3902-quoted.sh
index 5868052..14da45f 100755
--- a/t/t3902-quoted.sh
+++ b/t/t3902-quoted.sh
@@ -25,7 +25,7 @@ for_each_name () {
 	for name in \
 	    Name "Name and a${LF}LF" "Name and an${HT}HT" "Name${DQ}" \
 	    "$FN$HT$GN" "$FN$LF$GN" "$FN $GN" "$FN$GN" "$FN$DQ$GN" \
-	    "With SP in it"
+	    "With SP in it" "caractère spécial/file"
 	do
 		eval "$1"
 	done
@@ -33,6 +33,7 @@ for_each_name () {
 
 test_expect_success setup '
 
+	mkdir "caractère spécial" &&
 	for_each_name "echo initial >\"\$name\""
 	git add . &&
 	git commit -q -m Initial &&
@@ -50,6 +51,7 @@ Name
 "Name and an\tHT"
 "Name\""
 With SP in it
+"caract\303\250re sp\303\251cial/file"
 "\346\277\261\351\207\216\t\347\264\224"
 "\346\277\261\351\207\216\n\347\264\224"
 "\346\277\261\351\207\216 \347\264\224"
@@ -63,6 +65,7 @@ Name
 "Name and an\tHT"
 "Name\""
 With SP in it
+caractère spécial/file
 "濱野\t純"
 "濱野\n純"
 濱野 純
@@ -97,6 +100,13 @@ test_expect_success 'check fully quoted output from diff-tree' '
 
 '
 
+test_expect_success 'check fully quoted output from ls-tree' '
+
+	git ls-tree --name-only -r HEAD >current &&
+	test_cmp expect.quoted current
+
+'
+
 test_expect_success 'setting core.quotepath' '
 
 	git config --bool core.quotepath false
@@ -130,4 +140,11 @@ test_expect_success 'check fully quoted output from diff-tree' '
 
 '
 
+test_expect_success 'check fully quoted output from ls-tree' '
+
+	git ls-tree --name-only -r HEAD >current &&
+	test_cmp expect.raw current
+
+'
+
 test_done
-- 
1.7.0.rc1.16.g21332.dirty