{"thread":{"id":"38230","subject":"[PATCH] is_hfs_dotgit: loosen over-eager match of \\u{..47}","startedAt":"2014-12-23T08:45:36Z","lastAt":"2014-12-23T21:12:16Z","messageCount":10,"participants":["Jeff King","Torsten Bögershausen","Junio C Hamano","Jonathan Nieder","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"253992","messageId":"20141223084536.GA25190@peff.net","threadId":"38230","inReplyTo":null,"subject":"[PATCH] is_hfs_dotgit: loosen over-eager match of \\u{..47}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-23T08:45:36Z","receivedAt":"2014-12-23T08:45:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our is_hfs_dotgit function relies on the hackily-implemented\nnext_hfs_char to give us the next character that an HFS+\nfilename comparison would look at. It's hacky because it\ndoesn't implement the full case-folding table of HFS+; it\ngives us just enough to see if the path matches \".git\".\n\nAt the end of next_hfs_char, we use tolower() to convert our\n32-bit code point to lowercase. Our tolower() implementation\nonly takes an 8-bit char, though; it throws away the upper\n24 bits. This means we can't have any false negatives for\nis_hfs_dotgit. We only care about matching 7-bit ASCII\ncharacters in \".git\", and we will correctly process 'G' or\n'g'.\n\nHowever, we _can_ have false positives. Because we throw\naway the upper bits, code point \\u{0147} (for example) will\nlook like 'G' and get downcased to 'g'. It's not known\nwhether a sequence of code points whose truncation ends up\nas \".git\" is meaningful in any language, but it does not\nhurt to be more accurate here. We can just pass out the full\n32-bit code point, and compare it manually to the upper and\nlowercase characters we care about.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI saw Linus ask about this on G+. I had done the \"no false\nnegative\" analysis when writing the patch, but didn't\nconsider the false positive.\n\nAnother way of accomplishing the same thing is for next_hfs_char to\ncontinue folding case, but _only_ do so for 8-bit code points. Like:\n\n  return (out & 0xffffff00) ? out : tolower(out);\n\nI think the what's below is more obvious (and is actually\nhow I originally wrote it; I switched to using tolower()\nduring development to try to make it more readable).\n\n t/t1450-fsck.sh | 16 ++++++++++++++++\n utf8.c          | 17 ++++++++++++-----\n 2 files changed, 28 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 1f04b8a..3f5883d 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -349,6 +349,22 @@ dot-backslash-case .\\\\\\\\.GIT\\\\\\\\foobar\n dotgit-case-backslash .git\\\\\\\\foobar\n EOF\n \n+test_expect_success 'fsck allows .Ňit' '\n+\t(\n+\t\tgit init not-dotgit &&\n+\t\tcd not-dotgit &&\n+\t\techo content >file &&\n+\t\tgit add file &&\n+\t\tgit commit -m base &&\n+\t\tblob=$(git rev-parse :file) &&\n+\t\tprintf \"100644 blob $blob\\t.\\\\305\\\\207it\" >tree &&\n+\t\ttree=$(git mktree <tree) &&\n+\t\tgit fsck 2>err &&\n+\t\tcat err &&\n+\t\t! test -s err\n+\t)\n+'\n+\n # create a static test repo which is broken by omitting\n # one particular object ($1, which is looked up via rev-parse\n # in the new repository).\ndiff --git a/utf8.c b/utf8.c\nindex 9a3f4ad..34a779e 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -606,7 +606,7 @@ static ucs_char_t next_hfs_char(const char **in)\n \t\t * but this is enough to catch anything that will convert\n \t\t * to \".git\"\n \t\t */\n-\t\treturn tolower(out);\n+\t\treturn out;\n \t}\n }\n \n@@ -614,10 +614,17 @@ int is_hfs_dotgit(const char *path)\n {\n \tucs_char_t c;\n \n-\tif (next_hfs_char(&path) != '.' ||\n-\t    next_hfs_char(&path) != 'g' ||\n-\t    next_hfs_char(&path) != 'i' ||\n-\t    next_hfs_char(&path) != 't')\n+\tc = next_hfs_char(&path);\n+\tif (c != '.')\n+\t\treturn 0;\n+\tc = next_hfs_char(&path);\n+\tif (c != 'g' && c != 'G')\n+\t\treturn 0;\n+\tc = next_hfs_char(&path);\n+\tif (c != 'i' && c != 'I')\n+\t\treturn 0;\n+\tc = next_hfs_char(&path);\n+\tif (c != 't' && c != 'T')\n \t\treturn 0;\n \tc = next_hfs_char(&path);\n \tif (c && !is_dir_sep(c))\n-- \n2.2.1.376.gec59d43\n"},{"id":"254009","messageId":"54998949.9090908@web.de","threadId":"38230","inReplyTo":"20141223084536.GA25190@peff.net","subject":"Re: [PATCH] is_hfs_dotgit: loosen over-eager match of \\u{..47}","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-12-23T15:24:57Z","receivedAt":"2014-12-23T15:24:57Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-12-23 09.45, Jeff King wrote:\n> Our is_hfs_dotgit function relies on the hackily-implemented\n> next_hfs_char to give us the next character that an HFS+\n> filename comparison would look at. It's hacky because it\n> doesn't implement the full case-folding table of HFS+; it\n> gives us just enough to see if the path matches \".git\".\n> \n> At the end of next_hfs_char, we use tolower() to convert our\n> 32-bit code point to lowercase. Our tolower() implementation\n> only takes an 8-bit char, though; it throws away the upper\n> 24 bits. This means we can't have any false negatives for\n> is_hfs_dotgit. We only care about matching 7-bit ASCII\n> characters in \".git\", and we will correctly process 'G' or\n> 'g'.\n> \n> However, we _can_ have false positives. Because we throw\n> away the upper bits, code point \\u{0147} (for example) will\n> look like 'G' and get downcased to 'g'. It's not known\n> whether a sequence of code points whose truncation ends up\n> as \".git\" is meaningful in any language, but it does not\n> hurt to be more accurate here. We can just pass out the full\n> 32-bit code point, and compare it manually to the upper and\n> lowercase characters we care about.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I saw Linus ask about this on G+. I had done the \"no false\n> negative\" analysis when writing the patch, but didn't\n> consider the false positive.\n> \n> Another way of accomplishing the same thing is for next_hfs_char to\n> continue folding case, but _only_ do so for 8-bit code points. Like:\n> \nDon't we have the same possible problem under NTFS?\nUnder Linux + VFAT ?\nUnder all OS + VFAT ?\n\n\nAnd would it make sense to turn this\n>   return (out & 0xffffff00) ? out : tolower(out);\ninto this:\nstatic ucs_char_t unicode_tolower(ucs_char_t ch) {\n   return (ch & 0xffffff00) ? ch : tolower(ch);\n}\n\n\nAnd what happens if I export NTFS to Mac OS X?\n(Other combinations possible)\nShouldn't fsck under all OS warn for NTFS and hfs possible attacks ?\n \n"},{"id":"254031","messageId":"xmqqoaqu2pu7.fsf@gitster.dls.corp.google.com","threadId":"38230","inReplyTo":"54998949.9090908@web.de","subject":"Re: [PATCH] is_hfs_dotgit: loosen over-eager match of \\u{..47}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-23T18:17:20Z","receivedAt":"2014-12-23T18:17:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> Don't we have the same possible problem under NTFS?\n> Under Linux + VFAT ?\n> Under all OS + VFAT ?\n\nI do not think so, as NTFS codepath does not use the (previouly too\nwide but now fixed) is_hfs_dotgit() in the first place, and\nis_ntfs_dotgit() does not read one unicode codepoint at a time if I\nremember correctly.\n\n> And what happens if I export NTFS to Mac OS X?\n> (Other combinations possible)\n> Shouldn't fsck under all OS warn for NTFS and hfs possible attacks ?\n\nDoesn't it?  Have you read the whole series you are commenting on?\n"},{"id":"254032","messageId":"20141223181812.GA27078@peff.net","threadId":"38230","inReplyTo":"54998949.9090908@web.de","subject":"Re: [PATCH] is_hfs_dotgit: loosen over-eager match of \\u{..47}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-23T18:18:12Z","receivedAt":"2014-12-23T18:18:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 23, 2014 at 04:24:57PM +0100, Torsten Bögershausen wrote:\n\n> Don't we have the same possible problem under NTFS?\n> Under Linux + VFAT ?\n> Under all OS + VFAT ?\n\nI'm not sure what you mean.\n\nThis code path is _only_ about checking for HFS+-specific problems. We\ncheck general case-insensitivity in another code path. And we check\nNTFS-specific problems in another code path.\n\n(Technically we could make a \"check all\" function that runs over the\ndata only once, which would be more efficient. But doing it this way\nmakes the code much easier to follow).\n\n> And would it make sense to turn this\n> >   return (out & 0xffffff00) ? out : tolower(out);\n> into this:\n> static ucs_char_t unicode_tolower(ucs_char_t ch) {\n>    return (ch & 0xffffff00) ? ch : tolower(ch);\n> }\n\nPerhaps, but you would need a big warning at the top of that function\nthat is _only_ downcasing ASCII characters. I.e., it is fine for our use\nhere, but you would not want other unicode-aware code to call it. The\nnext_hfs_char already has such a warning at the top.\n\n> And what happens if I export NTFS to Mac OS X?\n> (Other combinations possible)\n> Shouldn't fsck under all OS warn for NTFS and hfs possible attacks ?\n\nFsck already warns for all system types, no matter what platform you're\non. And we do case-insensitive blocking of \".git\" entering the index for\nall platforms.\n\nWe don't turn on filesystem-specific blocking of paths entering the\nindex on all platforms. You get HFS+ blocking by default on OS X, and\nNTFS on Windows. If you are using HFS+ on Linux, you should set\ncore.protectHFS.\n\nPossibly we should just turn on all checks everywhere. That would be a\nsafer default, at the cost to Linux folks of:\n\n  1. Some slight inefficiency in each read-tree, as we have to do extra\n     processing on each name.\n\n  2. Some names would be disallowed that are otherwise OK. For the most\n     part these are not names that would be used in practice, though\n     (e.g., losing the ability to create \".git\\u200c\" is not a big loss\n     to anyone). I think Git for Windows has started blocking other\n     stuff like \"CON\" that is not specifically related to .git, though,\n     and that is more plausible for somebody to use in real life.\n\n-Peff\n"},{"id":"254046","messageId":"20141223201416.GA29365@google.com","threadId":"38230","inReplyTo":"20141223084536.GA25190@peff.net","subject":"Re: [PATCH] is_hfs_dotgit: loosen over-eager match of \\u{..47}","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-23T20:14:16Z","receivedAt":"2014-12-23T20:14:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t1450-fsck.sh | 16 ++++++++++++++++\n>  utf8.c          | 17 ++++++++++++-----\n>  2 files changed, 28 insertions(+), 5 deletions(-)\n\nMph.  Sorry I missed this before.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\n[...]\n> +++ b/t/t1450-fsck.sh\n[...]\n> +\t\tgit fsck 2>err &&\n> +\t\tcat err &&\n> +\t\t! test -s err\n\nNit: if this said\n\n\t\ttest_line_count = 0 err\n\nthen the error message would be more obvious when it fails with\n--verbose.\n\nThanks,\nJonathan\n"},{"id":"254047","messageId":"5499D10E.5050704@kdbg.org","threadId":"38230","inReplyTo":"20141223084536.GA25190@peff.net","subject":"Re: [PATCH] is_hfs_dotgit: loosen over-eager match of \\u{..47}","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-12-23T20:31:10Z","receivedAt":"2014-12-23T20:31:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 23.12.2014 um 09:45 schrieb Jeff King:\n> @@ -606,7 +606,7 @@ static ucs_char_t next_hfs_char(const char **in)\n>  \t\t * but this is enough to catch anything that will convert\n>  \t\t * to \".git\"\n>  \t\t */\n> -\t\treturn tolower(out);\n> +\t\treturn out;\n\nDid you consider changing the comment that we see in the pre-context here?\n\n-- Hannes\n"},{"id":"254048","messageId":"xmqqr3vq13mo.fsf@gitster.dls.corp.google.com","threadId":"38230","inReplyTo":"20141223201416.GA29365@google.com","subject":"Re: [PATCH] is_hfs_dotgit: loosen over-eager match of \\u{..47}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-23T21:02:23Z","receivedAt":"2014-12-23T21:02:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> +++ b/t/t1450-fsck.sh\n> [...]\n>> +\t\tgit fsck 2>err &&\n>> +\t\tcat err &&\n>> +\t\t! test -s err\n>\n> Nit: if this said\n>\n> \t\ttest_line_count = 0 err\n>\n> then the error message would be more obvious when it fails with\n> --verbose.\n\nThat's a good suggestion, I think.  This is meant to apply on top of\nd08c13b, and we already had test_line_count back then.\n\nSo far I collected these follow-ups to squash into Peff's patch.\n\n\n\n t/t1450-fsck.sh |  3 +--\n utf8.c          | 15 ++++++++-------\n 2 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 5e42385..0279b2b 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -284,8 +284,7 @@ test_expect_success 'fsck allows .Ňit' '\n \t\tprintf \"100644 blob $blob\\t.\\\\305\\\\207it\" >tree &&\n \t\ttree=$(git mktree <tree) &&\n \t\tgit fsck 2>err &&\n-\t\tcat err &&\n-\t\t! test -s err\n+\t\ttest_line_count = 0 err\n \t)\n '\n \ndiff --git a/utf8.c b/utf8.c\nindex 18a8f42..9c9fa3a 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -630,8 +630,8 @@ int mbs_chrlen(const char **text, size_t *remainder_p, const char *encoding)\n }\n \n /*\n- * Pick the next char from the stream, folding as an HFS+ filename comparison\n- * would. Note that this is _not_ complete by any means. It's just enough\n+ * Pick the next char from the stream, ignoring codepoints an HFS+ would.\n+ * Note that this is _not_ complete by any means. It's just enough\n  * to make is_hfs_dotgit() work, and should not be used otherwise.\n  */\n static ucs_char_t next_hfs_char(const char **in)\n@@ -668,11 +668,6 @@ static ucs_char_t next_hfs_char(const char **in)\n \t\t\tcontinue;\n \t\t}\n \n-\t\t/*\n-\t\t * there's a great deal of other case-folding that occurs,\n-\t\t * but this is enough to catch anything that will convert\n-\t\t * to \".git\"\n-\t\t */\n \t\treturn out;\n \t}\n }\n@@ -685,6 +680,12 @@ int is_hfs_dotgit(const char *path)\n \tif (c != '.')\n \t\treturn 0;\n \tc = next_hfs_char(&path);\n+\n+\t/*\n+\t * there's a great deal of other case-folding that occurs\n+\t * in HFS+, but this is enough to catch anything that will\n+\t * convert to \".git\"\n+\t */\n \tif (c != 'g' && c != 'G')\n \t\treturn 0;\n \tc = next_hfs_char(&path);\n"},{"id":"254049","messageId":"20141223210928.GA13304@peff.net","threadId":"38230","inReplyTo":"20141223201416.GA29365@google.com","subject":"Re: [PATCH] is_hfs_dotgit: loosen over-eager match of \\u{..47}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-23T21:09:28Z","receivedAt":"2014-12-23T21:09:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 23, 2014 at 12:14:16PM -0800, Jonathan Nieder wrote:\n\n> > +++ b/t/t1450-fsck.sh\n> [...]\n> > +\t\tgit fsck 2>err &&\n> > +\t\tcat err &&\n> > +\t\t! test -s err\n> \n> Nit: if this said\n> \n> \t\ttest_line_count = 0 err\n> \n> then the error message would be more obvious when it fails with\n> --verbose.\n\nThanks, I actually considered adding a \"test_file_is_empty\" for that\nreason, but empty line-count does the same thing.\n\n-Peff\n"},{"id":"254050","messageId":"20141223211152.GB13304@peff.net","threadId":"38230","inReplyTo":"5499D10E.5050704@kdbg.org","subject":"Re: [PATCH] is_hfs_dotgit: loosen over-eager match of \\u{..47}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-23T21:11:52Z","receivedAt":"2014-12-23T21:11:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 23, 2014 at 09:31:10PM +0100, Johannes Sixt wrote:\n\n> Am 23.12.2014 um 09:45 schrieb Jeff King:\n> > @@ -606,7 +606,7 @@ static ucs_char_t next_hfs_char(const char **in)\n> >  \t\t * but this is enough to catch anything that will convert\n> >  \t\t * to \".git\"\n> >  \t\t */\n> > -\t\treturn tolower(out);\n> > +\t\treturn out;\n> \n> Did you consider changing the comment that we see in the pre-context here?\n\nI did consider it, but the comment that is there was actually written\nfor the _original_ version, before I added tolower in the first place\n(it also applied equally to the tolower() version, so I left it).\n\nSo it was clear either way, at least in my brain. :)\n\nOf course that does not say anything about people's brains who did not\nwrite the patch.  The changes Junio suggested elsewhere in the thread do\nmake it more clear.\n\n-Peff\n"},{"id":"254051","messageId":"20141223211216.GC13304@peff.net","threadId":"38230","inReplyTo":"xmqqr3vq13mo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] is_hfs_dotgit: loosen over-eager match of \\u{..47}","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-23T21:12:16Z","receivedAt":"2014-12-23T21:12:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 23, 2014 at 01:02:23PM -0800, Junio C Hamano wrote:\n\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n> \n> >> +++ b/t/t1450-fsck.sh\n> > [...]\n> >> +\t\tgit fsck 2>err &&\n> >> +\t\tcat err &&\n> >> +\t\t! test -s err\n> >\n> > Nit: if this said\n> >\n> > \t\ttest_line_count = 0 err\n> >\n> > then the error message would be more obvious when it fails with\n> > --verbose.\n> \n> That's a good suggestion, I think.  This is meant to apply on top of\n> d08c13b, and we already had test_line_count back then.\n> \n> So far I collected these follow-ups to squash into Peff's patch.\n\nThey all look good to me. Thanks.\n\n-Peff\n"}]}