On Mon, Feb 01, 2010 at 06:32:13AM -0500, Jeff King wrote:
> The patch below fixes it for me. This is the first time I've ever looked
> at this code, though, so an extra set of eyes is appreciated. I'm also
> not sure of the "!p[len]" termination that the loop uses (quoted in the
> context below). The string is explicitly not NUL-terminated, so why
> would that matter? I think that may have been covering up the bug in
> some cases.
OK, I see now. Callers can pass "-1" for the length to indicate that the
name is NUL-terminated. So my initial patch does work, but it ends up
decrementing the "-1", which happens to work because next_quote_pos just
checks that it is negative.
Here is an updated patch. The two changes are:
1. A test in t3902.
2. It converts the "-1" into strlen(name) before doing any work, which
means we can lose the test for a NUL-terminator. I think this makes
it much easier to see what is going on. And arguably we are fixing
a "bug" where embedded NULs in a length-bounded string erroneously
marked the end-of-string. In practice, I don't think it matters as
we are quoting paths, and NUL is not a valid character there.
It does come at the slight expense of running an extra strlen()
instead of discovering the length as we progress through the loop.
It's possible to fix that, but it makes the code a bit harder to
read.
Junio, I think this bug goes back to v1.5.4, which makes the fix
appropriate for 'maint'.
-- >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.
On top of this, callers can pass in "-1" as the length to
indicate a NUL-terminated string, and we would also end our
loop upon seeing a NUL. To keep the code simple, let's just
convert "-1" cases into "strlen(name)".
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@xxxxxxxx>
---
quote.c | 6 +++++-
t/t3902-quoted.sh | 19 ++++++++++++++++++-
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/quote.c b/quote.c
index acb6bf9..e35ae50 100644
--- a/quote.c
+++ b/quote.c
@@ -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;
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html