threads / discuss / 22577

OS X Unicode Normalization Hits Again

Subject: OS X Unicode Normalization Hits Again

## tl;dr

8 messages between Feb 9, 2010 and Feb 9, 2010.

replies: 7people: 3as markdown or json

Brian Gernhardt· Feb 9, 2010, 03:38 UTC · lore
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
Brian Gernhardt· Feb 9, 2010, 04:07 UTC · re: Junio C Hamano · lore

[PATCH] t9302: Protect against OS X normalization

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
Jeff King· Feb 9, 2010, 06:08 UTC · re: Brian Gernhardt · lore

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

On Mon, Feb 08, 2010 at 11:07:25PM -0500, Brian Gernhardt wrote:
Show 16 quoted lines
> 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
Jeff King· Feb 9, 2010, 06:14 UTC · re: Jeff King · lore

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

On Tue, Feb 09, 2010 at 01:08:45AM -0500, Jeff King wrote:
Show 6 quoted lines
> >  > 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
Junio C Hamano· Feb 9, 2010, 06:53 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> 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.
Jeff King· Feb 9, 2010, 06:58 UTC · re: Junio C Hamano · lore

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

On Mon, Feb 08, 2010 at 10:53:49PM -0800, Junio C Hamano wrote:
Show 5 quoted lines
> 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
Junio C Hamano· Feb 9, 2010, 07:05 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> 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.

← back to recent threads