{"thread":{"id":"22577","subject":"OS X Unicode Normalization Hits Again","startedAt":"2010-02-09T03:38:45Z","lastAt":"2010-02-09T07:05:19Z","messageCount":8,"participants":["Brian Gernhardt","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"133986","messageId":"C417EBAD-9254-448C-9FD1-2D2FABB8CF32@gernhardtsoftware.com","threadId":"22577","inReplyTo":null,"subject":"OS X Unicode Normalization Hits Again","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2010-02-09T03:38:45Z","receivedAt":"2010-02-09T03:38:45Z","isPatch":false,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"git fails test t3902-quoted for me starting with 8424981: Fix invalid read in quote_c_style_counted\n\nThe problem isn't with the code, or the test.  It's that OS X is \"normalizing\" the file paths again.\n\ndiff --git a/t/t3902-quoted.sh b/t/t3902-quoted.sh\nindex 5868052..14da45f 100755\n--- a/t/t3902-quoted.sh\n+++ b/t/t3902-quoted.sh\n@@ -33,6 +33,7 @@ for_each_name () {\n \n test_expect_success setup '\n \n+       mkdir \"caractère spécial\" &&\n        for_each_name \"echo initial >\\\"\\$name\\\"\"\n        git add . &&\n        git commit -q -m Initial &&\n\nGit makes the directory \"caract\\303\\250re sp\\303\\251cial\" but OS X stores \"caracte\\314\\200re spe\\314\\201cial\".\n\nShould I just alter the test to use the same form as OS X, or is there a better method?\n\n~~ Brian\n"},{"id":"133987","messageId":"7vfx5bt6nn.fsf@alter.siamese.dyndns.org","threadId":"22577","inReplyTo":"C417EBAD-9254-448C-9FD1-2D2FABB8CF32@gernhardtsoftware.com","subject":"Re: OS X Unicode Normalization Hits Again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T03:44:12Z","receivedAt":"2010-02-09T03:44:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"How about using $FN as the directory name instead?\n"},{"id":"133993","messageId":"1265688445-46137-1-git-send-email-brian@gernhardtsoftware.com","threadId":"22577","inReplyTo":"7vfx5bt6nn.fsf@alter.siamese.dyndns.org","subject":"[PATCH] t9302: Protect against OS X normalization","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2010-02-09T04:07:25Z","receivedAt":"2010-02-09T04:07:25Z","isPatch":true,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"8424981: \"Fix invalid read in quote_c_style_counted\" introduced a test\nthat used \"caractère spécial\" as a directory name.\n\nGit creates it as \"caract\\303\\250re sp\\303\\251cial\"\nOS X stores it as \"caracte\\314\\200re spe\\314\\201cial\"\n\nTo work around this problem, use the already introduced $FN as the\ndirectory name.\n\nSigned-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>\n---\n\n Junio C Hamano wrote:\n > How about using $FN as the directory name instead?\n\n I knew there was a clever answer I was missing.\n\n t/t3902-quoted.sh |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t3902-quoted.sh b/t/t3902-quoted.sh\nindex 14da45f..29103f6 100755\n--- a/t/t3902-quoted.sh\n+++ b/t/t3902-quoted.sh\n@@ -25,7 +25,7 @@ for_each_name () {\n \tfor name in \\\n \t    Name \"Name and a${LF}LF\" \"Name and an${HT}HT\" \"Name${DQ}\" \\\n \t    \"$FN$HT$GN\" \"$FN$LF$GN\" \"$FN $GN\" \"$FN$GN\" \"$FN$DQ$GN\" \\\n-\t    \"With SP in it\" \"caractère spécial/file\"\n+\t    \"With SP in it\" \"$FN/file\"\n \tdo\n \t\teval \"$1\"\n \tdone\n@@ -33,7 +33,7 @@ for_each_name () {\n \n test_expect_success setup '\n \n-\tmkdir \"caractère spécial\" &&\n+\tmkdir \"$FN\" &&\n \tfor_each_name \"echo initial >\\\"\\$name\\\"\"\n \tgit add . &&\n \tgit commit -q -m Initial &&\n@@ -51,11 +51,11 @@ Name\n \"Name and an\\tHT\"\n \"Name\\\"\"\n With SP in it\n-\"caract\\303\\250re sp\\303\\251cial/file\"\n \"\\346\\277\\261\\351\\207\\216\\t\\347\\264\\224\"\n \"\\346\\277\\261\\351\\207\\216\\n\\347\\264\\224\"\n \"\\346\\277\\261\\351\\207\\216 \\347\\264\\224\"\n \"\\346\\277\\261\\351\\207\\216\\\"\\347\\264\\224\"\n+\"\\346\\277\\261\\351\\207\\216/file\"\n \"\\346\\277\\261\\351\\207\\216\\347\\264\\224\"\n EOF\n \n@@ -65,11 +65,11 @@ Name\n \"Name and an\\tHT\"\n \"Name\\\"\"\n With SP in it\n-caractère spécial/file\n \"濱野\\t純\"\n \"濱野\\n純\"\n 濱野 純\n \"濱野\\\"純\"\n+濱野/file\n 濱野純\n EOF\n \n-- \n1.7.0.rc1.49.gd2d66\n"},{"id":"134005","messageId":"20100209060845.GD14736@coredump.intra.peff.net","threadId":"22577","inReplyTo":"1265688445-46137-1-git-send-email-brian@gernhardtsoftware.com","subject":"Re: [PATCH] t9302: Protect against OS X normalization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-09T06:08:45Z","receivedAt":"2010-02-09T06:08:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2010 at 11:07:25PM -0500, Brian Gernhardt wrote:\n\n> 8424981: \"Fix invalid read in quote_c_style_counted\" introduced a test\n> that used \"caractère spécial\" as a directory name.\n> \n> Git creates it as \"caract\\303\\250re sp\\303\\251cial\"\n> OS X stores it as \"caracte\\314\\200re spe\\314\\201cial\"\n> \n> To work around this problem, use the already introduced $FN as the\n> directory name.\n> \n> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>\n> ---\n> \n>  Junio C Hamano wrote:\n>  > How about using $FN as the directory name instead?\n> \n>  I knew there was a clever answer I was missing.\n\nI am not 100% sure this will still trigger the failure that 8424981 was\nmeant to fix. From my recollection of the bug, it not only needed an\nunterminated string (which we get by having a directory) but the string\nlength and presence of multiple spread-out characters may have been\nrelevant.\n\nOf course, that specific bug is fixed, so maybe it is not worth worrying\nabout too much.\n\n-Peff\n"},{"id":"134006","messageId":"20100209061415.GE14736@coredump.intra.peff.net","threadId":"22577","inReplyTo":"20100209060845.GD14736@coredump.intra.peff.net","subject":"Re: [PATCH] t9302: Protect against OS X normalization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-09T06:14:15Z","receivedAt":"2010-02-09T06:14:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 09, 2010 at 01:08:45AM -0500, Jeff King wrote:\n\n> >  > How about using $FN as the directory name instead?\n> > \n> >  I knew there was a clever answer I was missing.\n> \n> I am not 100% sure this will still trigger the failure that 8424981 was\n> meant to fix. From my recollection of the bug, it not only needed an\n\nActually, I take it back. I just tested it and it does break just fine.\n:)\n\nSo the fix looks good to me.\n\n-Peff\n"},{"id":"134007","messageId":"7vocjyq4qq.fsf@alter.siamese.dyndns.org","threadId":"22577","inReplyTo":"20100209060845.GD14736@coredump.intra.peff.net","subject":"Re: [PATCH] t9302: Protect against OS X normalization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T06:53:49Z","receivedAt":"2010-02-09T06:53:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I am not 100% sure this will still trigger the failure that 8424981 was\n> meant to fix. From my recollection of the bug, it not only needed an\n> unterminated string (which we get by having a directory) but the string\n> length and presence of multiple spread-out characters may have been\n> relevant.\n\nI think the test was triggering by chance.  We give the directory name as\npfx to write_name_quoted() which in turn tells quote_c_style_counted() to\nconsume only pfxlen, but the random junk after pfxlen was being looked at\nbecause we didn't decrement maxlen.\n\nThe call chain that led to this part looks like this:\n\n tree.c:read_tree_recursive() ll.127-131\n - allocate newbase[], fill the leading directory path\n - call read_tree_recursive with newbase as base\n   tree.c:read_tree_recursive() l.114\n   - call callback function\n     builtin-ls-tree.c:show_tree() l.114\n     - call write_name_quotedpfx() with the given base\n       unfixed code simply oversteps the end of newbase[] given from the\n       toplevel read_tree_recursive!\n\nI think there is no reliable reproduction recipe for this, as we don't\ncontrol what garbage is in the tail of malloc'ed memory; valgrind would\nhave found it, though.\n\nLet's revert the test part of the patch.\n"},{"id":"134008","messageId":"20100209065845.GA6503@sigill.intra.peff.net","threadId":"22577","inReplyTo":"7vocjyq4qq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t9302: Protect against OS X normalization","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-09T06:58:45Z","receivedAt":"2010-02-09T06:58:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2010 at 10:53:49PM -0800, Junio C Hamano wrote:\n\n> I think there is no reliable reproduction recipe for this, as we don't\n> control what garbage is in the tail of malloc'ed memory; valgrind would\n> have found it, though.\n>\n> Let's revert the test part of the patch.\n\nIMHO, valgrind finding it is reason enough to keep it. We do run the\ntest suite against valgrind from time to time, and clearly this code\npath was not being exercised prior to this (or my previous valgrind runs\nwould have caught it).\n\nI think Brian's patch is the best thing to do.\n\n-Peff\n"},{"id":"134009","messageId":"7vr5ouopn4.fsf@alter.siamese.dyndns.org","threadId":"22577","inReplyTo":"20100209065845.GA6503@sigill.intra.peff.net","subject":"Re: [PATCH] t9302: Protect against OS X normalization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-09T07:05:19Z","receivedAt":"2010-02-09T07:05:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Feb 08, 2010 at 10:53:49PM -0800, Junio C Hamano wrote:\n>\n>> I think there is no reliable reproduction recipe for this, as we don't\n>> control what garbage is in the tail of malloc'ed memory; valgrind would\n>> have found it, though.\n>>\n>> Let's revert the test part of the patch.\n>\n> IMHO, valgrind finding it is reason enough to keep it. We do run the\n> test suite against valgrind from time to time, and clearly this code\n> path was not being exercised prior to this (or my previous valgrind runs\n> would have caught it).\n>\n> I think Brian's patch is the best thing to do.\n\nHaving Brian's patch would not hurt, and it definitely is an improvement\nthan using a name OSX cannot cope with, so I'll apply it nevertheless.\n\nFWIW, neither the updated test nor your French test does not trigger for\nme after reverting the \"quote.c\" part from 8424981 without valgrind.\n\nThanks for helping me think this through.\n"}]}