OS X Unicode Normalization Hits Again

OS X Unicode Normalization Hits Again

[Brian Gernhardt]

git fails test t3902-quoted for me starting with 8424981: Fix invalid read in quote_c_style_counted

The problem isn't with the code, or the test.  It's that OS X is "normalizing" the file paths again.

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
@@ -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 &&

Git makes the directory "caract\303\250re sp\303\251cial" but OS X stores "caracte\314\200re spe\314\201cial".

Should I just alter the test to use the same form as OS X, or is there a better method?

~~ Brian

Re: OS X Unicode Normalization Hits Again

[Junio C Hamano]

How about using $FN as the directory name instead?

[PATCH] t9302: Protect against OS X normalization

[Brian Gernhardt]

8424981: "Fix invalid read in quote_c_style_counted" introduced a test
that used "caractère spécial" as a directory name.

Git creates it as "caract\303\250re sp\303\251cial"
OS X stores it as "caracte\314\200re spe\314\201cial"

To work around this problem, use the already introduced $FN as the
directory name.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

 Junio C Hamano wrote:
 > How about using $FN as the directory name instead?

 I knew there was a clever answer I was missing.

 t/t3902-quoted.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3902-quoted.sh b/t/t3902-quoted.sh
index 14da45f..29103f6 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" "caractère spécial/file"
+	    "With SP in it" "$FN/file"
 	do
 		eval "$1"
 	done
@@ -33,7 +33,7 @@ for_each_name () {
 
 test_expect_success setup '
 
-	mkdir "caractère spécial" &&
+	mkdir "$FN" &&
 	for_each_name "echo initial >\"\$name\""
 	git add . &&
 	git commit -q -m Initial &&
@@ -51,11 +51,11 @@ 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"
 "\346\277\261\351\207\216\"\347\264\224"
+"\346\277\261\351\207\216/file"
 "\346\277\261\351\207\216\347\264\224"
 EOF
 
@@ -65,11 +65,11 @@ Name
 "Name and an\tHT"
 "Name\""
 With SP in it
-caractère spécial/file
 "濱野\t純"
 "濱野\n純"
 濱野 純
 "濱野\"純"
+濱野/file
 濱野純
 EOF
 
-- 
1.7.0.rc1.49.gd2d66

Re: [PATCH] t9302: Protect against OS X normalization

[Jeff King]

On Mon, Feb 08, 2010 at 11:07:25PM -0500, Brian Gernhardt wrote:

> 8424981: "Fix invalid read in quote_c_style_counted" introduced a test
> that used "caractère spécial" as a directory name.
> 
> Git creates it as "caract\303\250re sp\303\251cial"
> OS X stores it as "caracte\314\200re spe\314\201cial"
> 
> To work around this problem, use the already introduced $FN as the
> directory name.
> 
> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> ---
> 
>  Junio C Hamano wrote:
>  > How about using $FN as the directory name instead?
> 
>  I knew there was a clever answer I was missing.

I am not 100% sure this will still trigger the failure that 8424981 was
meant to fix. From my recollection of the bug, it not only needed an
unterminated string (which we get by having a directory) but the string
length and presence of multiple spread-out characters may have been
relevant.

Of course, that specific bug is fixed, so maybe it is not worth worrying
about too much.

-Peff

Re: [PATCH] t9302: Protect against OS X normalization

[Jeff King]

On Tue, Feb 09, 2010 at 01:08:45AM -0500, Jeff King wrote:

> >  > How about using $FN as the directory name instead?
> > 
> >  I knew there was a clever answer I was missing.
> 
> I am not 100% sure this will still trigger the failure that 8424981 was
> meant to fix. From my recollection of the bug, it not only needed an

Actually, I take it back. I just tested it and it does break just fine.
:)

So the fix looks good to me.

-Peff

Re: [PATCH] t9302: Protect against OS X normalization

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I am not 100% sure this will still trigger the failure that 8424981 was
> meant to fix. From my recollection of the bug, it not only needed an
> unterminated string (which we get by having a directory) but the string
> length and presence of multiple spread-out characters may have been
> relevant.

I think the test was triggering by chance.  We give the directory name as
pfx to write_name_quoted() which in turn tells quote_c_style_counted() to
consume only pfxlen, but the random junk after pfxlen was being looked at
because we didn't decrement maxlen.

The call chain that led to this part looks like this:

 tree.c:read_tree_recursive() ll.127-131
 - allocate newbase[], fill the leading directory path
 - call read_tree_recursive with newbase as base
   tree.c:read_tree_recursive() l.114
   - call callback function
     builtin-ls-tree.c:show_tree() l.114
     - call write_name_quotedpfx() with the given base
       unfixed code simply oversteps the end of newbase[] given from the
       toplevel read_tree_recursive!

I think there is no reliable reproduction recipe for this, as we don't
control what garbage is in the tail of malloc'ed memory; valgrind would
have found it, though.

Let's revert the test part of the patch.

Re: [PATCH] t9302: Protect against OS X normalization

[Jeff King]

On Mon, Feb 08, 2010 at 10:53:49PM -0800, Junio C Hamano wrote:

> I think there is no reliable reproduction recipe for this, as we don't
> control what garbage is in the tail of malloc'ed memory; valgrind would
> have found it, though.
>
> Let's revert the test part of the patch.

IMHO, valgrind finding it is reason enough to keep it. We do run the
test suite against valgrind from time to time, and clearly this code
path was not being exercised prior to this (or my previous valgrind runs
would have caught it).

I think Brian's patch is the best thing to do.

-Peff

Re: [PATCH] t9302: Protect against OS X normalization

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Feb 08, 2010 at 10:53:49PM -0800, Junio C Hamano wrote:
>
>> I think there is no reliable reproduction recipe for this, as we don't
>> control what garbage is in the tail of malloc'ed memory; valgrind would
>> have found it, though.
>>
>> Let's revert the test part of the patch.
>
> IMHO, valgrind finding it is reason enough to keep it. We do run the
> test suite against valgrind from time to time, and clearly this code
> path was not being exercised prior to this (or my previous valgrind runs
> would have caught it).
>
> I think Brian's patch is the best thing to do.

Having Brian's patch would not hurt, and it definitely is an improvement
than using a name OSX cannot cope with, so I'll apply it nevertheless.

FWIW, neither the updated test nor your French test does not trigger for
me after reverting the "quote.c" part from 8424981 without valgrind.

Thanks for helping me think this through.