{"thread":{"id":"22468","subject":"Problem listing GIT repository with accents","startedAt":"2010-02-01T10:48:29Z","lastAt":"2010-02-01T17:40:45Z","messageCount":8,"participants":["Ellié Computing Open Source Program","Jeff King","Johannes Sixt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"133238","messageId":"7E88665723814E46BCBA1A39E84C27A5@elrond","threadId":"22468","inReplyTo":null,"subject":"Problem listing GIT repository with accents","fromName":"Ellié Computing Open Source Program","fromEmail":"opensource@elliecomputing.com","sentAt":"2010-02-01T10:48:29Z","receivedAt":"2010-02-01T10:48:29Z","isPatch":false,"sender":{"key":"opensource@elliecomputing.com","avatar":null},"body":"Dear all,\n\nI'm writing a tool (ECMerge) which use the git command line to list folders \ncontent (as of specific commits, tags and so on). This way our users can \nbrowse a GIT repository in our GUI.\n\nWe are in front of the following problem: when a GIT repository contains a \nfolder with accents, the files names for its content are often prepended \nwith random characters. Here is a transcript of a list of operation \nexhibiting the problem:\n\n C:\\temp\\scc-tests\\git>git ls-tree HEAD .\n100644 blob 443d8625f771c421efd86c129483f9a139a4e85f    \"p\\351p\\351.txt\"\nC:\\temp\\scc-tests\\git>mkdir \"caractère spécial\"\nC:\\temp\\scc-tests\\git>echo plouf > \"caractère spécial\\plouf.txt\"\nC:\\temp\\scc-tests\\git>git add \"caractère spécial\"\nC:\\temp\\scc-tests\\git>git commit -m plouf\n[master b94d9cb] plouf\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 \"caract\\350re sp\\351cial/plouf.txt\"\n\nC:\\temp\\scc-tests\\git>git ls-tree HEAD .\n040000 tree d2b614bbfb4c5f39a32eb1309654262df113f605    \"caract\\350re \nsp\\351cial\"\n100644 blob 443d8625f771c421efd86c129483f9a139a4e85f    \"p\\351p\\351.txt\"\n\nC:\\temp\\scc-tests\\git>git ls-tree -r HEAD .\n100644 blob bf10a8b39e72c754ee1872fcdb13662cba6a8880    \"caract\\350re \nsp\\351cial/\\272plouf.txt\"\n100644 blob 443d8625f771c421efd86c129483f9a139a4e85f    \"p\\351p\\351.txt\"\n\nC:\\temp\\scc-tests\\git>git ls-tree -r HEAD \"caractère spécial\"\n100644 blob bf10a8b39e72c754ee1872fcdb13662cba6a8880    \"caract\\350re \nsp\\351cial/\\272plouf.txt\"\n\nNote the spurious \\272 which comes in the listing :(\nTrying again the same commands may give other spurious characters (each time \nwe tried we get different _bad_ responses)\n\n\nHope you can do somehing for that,\nBest regards\nArmel Asselin - from Ellié Computing\n"},{"id":"133240","messageId":"20100201113213.GA22663@coredump.intra.peff.net","threadId":"22468","inReplyTo":"7E88665723814E46BCBA1A39E84C27A5@elrond","subject":"Re: Problem listing GIT repository with accents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-01T11:32:13Z","receivedAt":"2010-02-01T11:32:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 01, 2010 at 11:48:29AM +0100, Ellié Computing Open Source Program wrote:\n\n> C:\\temp\\scc-tests\\git>git ls-tree -r HEAD \"caractère spécial\"\n> 100644 blob bf10a8b39e72c754ee1872fcdb13662cba6a8880    \"caract\\350re\n> sp\\351cial/\\272plouf.txt\"\n> \n> Note the spurious \\272 which comes in the listing :(\n> Trying again the same commands may give other spurious characters\n> (each time we tried we get different _bad_ responses)\n\nLooks like a bug. I was able to replicate it here, and valgrind notices\nit, too:\n\n  ==22720== Invalid read of size 1\n  ==22720==    at 0x80E77FF: next_quote_pos (quote.c:174)\n  ==22720==    by 0x80E783A: quote_c_style_counted (quote.c:215)\n  ==22720==    by 0x80E7D14: write_name_quotedpfx (quote.c:286)\n  ==22720==    by 0x80808F3: show_tree (builtin-ls-tree.c:114)\n  ==22720==    by 0x811000D: read_tree_recursive (tree.c:114)\n  ==22720==    by 0x81100E7: read_tree_recursive (tree.c:131)\n  ==22720==    by 0x8080CC2: cmd_ls_tree (builtin-ls-tree.c:173)\n  ==22720==    by 0x804B7FA: run_builtin (git.c:257)\n  ==22720==    by 0x804B958: handle_internal_command (git.c:412)\n  ==22720==    by 0x804BA2F: run_argv (git.c:454)\n  ==22720==    by 0x804BB97: main (git.c:525)\n  ==22720==  Address 0x43405b4 is 0 bytes after a block of size 20 alloc'd\n  ==22720==    at 0x4024C4C: malloc (vg_replace_malloc.c:195)\n  ==22720==    by 0x8115739: xmalloc (wrapper.c:20)\n  ==22720==    by 0x811005E: read_tree_recursive (tree.c:127)\n  ==22720==    by 0x8080CC2: cmd_ls_tree (builtin-ls-tree.c:173)\n  ==22720==    by 0x804B7FA: run_builtin (git.c:257)\n  ==22720==    by 0x804B958: handle_internal_command (git.c:412)\n  ==22720==    by 0x804BA2F: run_argv (git.c:454)\n  ==22720==    by 0x804BB97: main (git.c:525)\n\nThe patch below fixes it for me. This is the first time I've ever looked\nat this code, though, so an extra set of eyes is appreciated. I'm also\nnot sure of the \"!p[len]\" termination that the loop uses (quoted in the\ncontext below). The string is explicitly not NUL-terminated, so why\nwould that matter? I think that may have been covering up the bug in\nsome cases.\n\n-- >8 --\nSubject: [PATCH] fix invalid read in quote_c_style_counted\n\nWe progress through a length-bounded string, looking for\ncharacters in need of quoting. After each character is\nfound, we output everything up until that character\nliterally, then the quoted character. We then advance our\nstring and look again.  However, we never actually\ndecremented the length, meaning we ended up looking at\nwhatever random junk was stored after the string.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n quote.c |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/quote.c b/quote.c\nindex acb6bf9..392006d 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -216,20 +216,21 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,\n \t\tif (len == maxlen || !p[len])\n \t\t\tbreak;\n \n \t\tif (!no_dq && p == name)\n \t\t\tEMIT('\"');\n \n \t\tEMITBUF(p, len);\n \t\tEMIT('\\\\');\n \t\tp += len;\n \t\tch = (unsigned char)*p++;\n+\t\tmaxlen -= len + 1;\n \t\tif (sq_lookup[ch] >= ' ') {\n \t\t\tEMIT(sq_lookup[ch]);\n \t\t} else {\n \t\t\tEMIT(((ch >> 6) & 03) + '0');\n \t\t\tEMIT(((ch >> 3) & 07) + '0');\n \t\t\tEMIT(((ch >> 0) & 07) + '0');\n \t\t}\n \t}\n \n \tEMITBUF(p, len);\n-- \n1.7.0.rc1.16.g21332.dirty\n"},{"id":"133243","messageId":"20100201121933.GA9995@coredump.intra.peff.net","threadId":"22468","inReplyTo":"20100201113213.GA22663@coredump.intra.peff.net","subject":"Re: Problem listing GIT repository with accents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-01T12:19:33Z","receivedAt":"2010-02-01T12:19:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 01, 2010 at 06:32:13AM -0500, Jeff King wrote:\n\n> The patch below fixes it for me. This is the first time I've ever looked\n> at this code, though, so an extra set of eyes is appreciated. I'm also\n> not sure of the \"!p[len]\" termination that the loop uses (quoted in the\n> context below). The string is explicitly not NUL-terminated, so why\n> would that matter? I think that may have been covering up the bug in\n> some cases.\n\nOK, I see now. Callers can pass \"-1\" for the length to indicate that the\nname is NUL-terminated. So my initial patch does work, but it ends up\ndecrementing the \"-1\", which happens to work because next_quote_pos just\nchecks that it is negative.\n\nHere is an updated patch. The two changes are:\n\n  1. A test in t3902.\n\n  2. It converts the \"-1\" into strlen(name) before doing any work, which\n     means we can lose the test for a NUL-terminator. I think this makes\n     it much easier to see what is going on. And arguably we are fixing\n     a \"bug\" where embedded NULs in a length-bounded string erroneously\n     marked the end-of-string. In practice, I don't think it matters as\n     we are quoting paths, and NUL is not a valid character there.\n\n     It does come at the slight expense of running an extra strlen()\n     instead of discovering the length as we progress through the loop.\n     It's possible to fix that, but it makes the code a bit harder to\n     read.\n\nJunio, I think this bug goes back to v1.5.4, which makes the fix\nappropriate for 'maint'.\n\n-- >8 --\nSubject: [PATCH] fix invalid read in quote_c_style_counted\n\nThis function did not work on strings that were not\nNUL-terminated. It reads through a length-bounded string,\nsearching for characters in need of quoting. After we find\none, we output the quoted character, then advance our\npointer to find the next one. However, we never decremented\nthe length, meaning we ended up looking at whatever random\njunk was stored after the string.\n\nOn top of this, callers can pass in \"-1\" as the length to\nindicate a NUL-terminated string, and we would also end our\nloop upon seeing a NUL. To keep the code simple, let's just\nconvert \"-1\" cases into \"strlen(name)\".\n\nThis bug was not found by the existing tests because most\ncode paths feed a NUL-terminated string. The notable\nexception is a directory name being fed by ls-tree.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n quote.c           |    6 +++++-\n t/t3902-quoted.sh |   19 ++++++++++++++++++-\n 2 files changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/quote.c b/quote.c\nindex acb6bf9..e35ae50 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -209,11 +209,14 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,\n \tsize_t len, count = 0;\n \tconst char *p = name;\n \n+\tif (maxlen < 0)\n+\t\tmaxlen = strlen(name);\n+\n \tfor (;;) {\n \t\tint ch;\n \n \t\tlen = next_quote_pos(p, maxlen);\n-\t\tif (len == maxlen || !p[len])\n+\t\tif (len == maxlen)\n \t\t\tbreak;\n \n \t\tif (!no_dq && p == name)\n@@ -223,6 +226,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,\n \t\tEMIT('\\\\');\n \t\tp += len;\n \t\tch = (unsigned char)*p++;\n+\t\tmaxlen -= len + 1;\n \t\tif (sq_lookup[ch] >= ' ') {\n \t\t\tEMIT(sq_lookup[ch]);\n \t\t} else {\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@@ -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\"\n+\t    \"With SP in it\" \"caractère spécial/file\"\n \tdo\n \t\teval \"$1\"\n \tdone\n@@ -33,6 +33,7 @@ for_each_name () {\n \n test_expect_success setup '\n \n+\tmkdir \"caractère spécial\" &&\n \tfor_each_name \"echo initial >\\\"\\$name\\\"\"\n \tgit add . &&\n \tgit commit -q -m Initial &&\n@@ -50,6 +51,7 @@ 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@@ -63,6 +65,7 @@ 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@@ -97,6 +100,13 @@ test_expect_success 'check fully quoted output from diff-tree' '\n \n '\n \n+test_expect_success 'check fully quoted output from ls-tree' '\n+\n+\tgit ls-tree --name-only -r HEAD >current &&\n+\ttest_cmp expect.quoted current\n+\n+'\n+\n test_expect_success 'setting core.quotepath' '\n \n \tgit config --bool core.quotepath false\n@@ -130,4 +140,11 @@ test_expect_success 'check fully quoted output from diff-tree' '\n \n '\n \n+test_expect_success 'check fully quoted output from ls-tree' '\n+\n+\tgit ls-tree --name-only -r HEAD >current &&\n+\ttest_cmp expect.raw current\n+\n+'\n+\n test_done\n-- \n1.7.0.rc1.16.g21332.dirty\n"},{"id":"133246","messageId":"4B66CD81.3010005@viscovery.net","threadId":"22468","inReplyTo":"20100201121933.GA9995@coredump.intra.peff.net","subject":"Re: Problem listing GIT repository with accents","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-02-01T12:48:01Z","receivedAt":"2010-02-01T12:48:01Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Jeff King schrieb:\n> @@ -209,11 +209,14 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,\n>  \tsize_t len, count = 0;\n>  \tconst char *p = name;\n>  \n> +\tif (maxlen < 0)\n> +\t\tmaxlen = strlen(name);\n> +\n>  \tfor (;;) {\n>  \t\tint ch;\n>  \n>  \t\tlen = next_quote_pos(p, maxlen);\n> -\t\tif (len == maxlen || !p[len])\n> +\t\tif (len == maxlen)\n>  \t\t\tbreak;\n>  \n>  \t\tif (!no_dq && p == name)\n> @@ -223,6 +226,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,\n>  \t\tEMIT('\\\\');\n>  \t\tp += len;\n>  \t\tch = (unsigned char)*p++;\n> +\t\tmaxlen -= len + 1;\n\nCouldn't you just write\n\n\t\tif (0 <= maxlen)\n\t\t\tmaxlen -= len + 1;\n\nto avoid the strlen(), because the rest of the loop is actually OK when\nmaxlen == -1, isn't it?\n\n-- Hannes\n"},{"id":"133248","messageId":"20100201133903.GA923@coredump.intra.peff.net","threadId":"22468","inReplyTo":"4B66CD81.3010005@viscovery.net","subject":"Re: Problem listing GIT repository with accents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-01T13:39:03Z","receivedAt":"2010-02-01T13:39:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 01, 2010 at 01:48:01PM +0100, Johannes Sixt wrote:\n\n> Jeff King schrieb:\n> > @@ -209,11 +209,14 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,\n> >  \tsize_t len, count = 0;\n> >  \tconst char *p = name;\n> >  \n> > +\tif (maxlen < 0)\n> > +\t\tmaxlen = strlen(name);\n> > +\n> >  \tfor (;;) {\n> >  \t\tint ch;\n> >  \n> >  \t\tlen = next_quote_pos(p, maxlen);\n> > -\t\tif (len == maxlen || !p[len])\n> > +\t\tif (len == maxlen)\n> >  \t\t\tbreak;\n> >  \n> >  \t\tif (!no_dq && p == name)\n> > @@ -223,6 +226,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,\n> >  \t\tEMIT('\\\\');\n> >  \t\tp += len;\n> >  \t\tch = (unsigned char)*p++;\n> > +\t\tmaxlen -= len + 1;\n> \n> Couldn't you just write\n> \n> \t\tif (0 <= maxlen)\n> \t\t\tmaxlen -= len + 1;\n> \n> to avoid the strlen(), because the rest of the loop is actually OK when\n> maxlen == -1, isn't it?\n\nIf you want to keep the \"!p[len]\" condition, then yes. If we want to\nproperly quote internal NULs (which again, I am not sure is of practical\nuse), then you would also have to make that condition \"(maxlen >= 0 &&\n!p[len])\".  Which is really not that bad, but I was trying to make it\neasier to read. I am OK with any of the three combinations of fixes.\n\nAnd the fact that I am using the word \"combination\" probably means it\nshould be a separate patch anyway.\n\nSo here is the minimal patch to go on 'maint', for Junio's convenience.\n\n-- >8 --\nSubject: [PATCH] fix invalid read in quote_c_style_counted\n\nThis function did not work on strings that were not\nNUL-terminated. It reads through a length-bounded string,\nsearching for characters in need of quoting. After we find\none, we output the quoted character, then advance our\npointer to find the next one. However, we never decremented\nthe length, meaning we ended up looking at whatever random\njunk was stored after the string.\n\nThis bug was not found by the existing tests because most\ncode paths feed a NUL-terminated string. The notable\nexception is a directory name being fed by ls-tree.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n quote.c           |    2 ++\n t/t3902-quoted.sh |   19 ++++++++++++++++++-\n 2 files changed, 20 insertions(+), 1 deletions(-)\n\ndiff --git a/quote.c b/quote.c\nindex acb6bf9..723bb4f 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -223,6 +223,8 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,\n \t\tEMIT('\\\\');\n \t\tp += len;\n \t\tch = (unsigned char)*p++;\n+\t\tif (maxlen >= 0)\n+\t\t\tmaxlen -= len + 1;\n \t\tif (sq_lookup[ch] >= ' ') {\n \t\t\tEMIT(sq_lookup[ch]);\n \t\t} else {\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@@ -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\"\n+\t    \"With SP in it\" \"caractère spécial/file\"\n \tdo\n \t\teval \"$1\"\n \tdone\n@@ -33,6 +33,7 @@ for_each_name () {\n \n test_expect_success setup '\n \n+\tmkdir \"caractère spécial\" &&\n \tfor_each_name \"echo initial >\\\"\\$name\\\"\"\n \tgit add . &&\n \tgit commit -q -m Initial &&\n@@ -50,6 +51,7 @@ 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@@ -63,6 +65,7 @@ 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@@ -97,6 +100,13 @@ test_expect_success 'check fully quoted output from diff-tree' '\n \n '\n \n+test_expect_success 'check fully quoted output from ls-tree' '\n+\n+\tgit ls-tree --name-only -r HEAD >current &&\n+\ttest_cmp expect.quoted current\n+\n+'\n+\n test_expect_success 'setting core.quotepath' '\n \n \tgit config --bool core.quotepath false\n@@ -130,4 +140,11 @@ test_expect_success 'check fully quoted output from diff-tree' '\n \n '\n \n+test_expect_success 'check fully quoted output from ls-tree' '\n+\n+\tgit ls-tree --name-only -r HEAD >current &&\n+\ttest_cmp expect.raw current\n+\n+'\n+\n test_done\n-- \n1.7.0.rc1.16.g21332.dirty\n"},{"id":"133249","messageId":"20100201134454.GA796@coredump.intra.peff.net","threadId":"22468","inReplyTo":"20100201133903.GA923@coredump.intra.peff.net","subject":"Re: Problem listing GIT repository with accents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-01T13:44:54Z","receivedAt":"2010-02-01T13:44:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 01, 2010 at 08:39:03AM -0500, Jeff King wrote:\n\n> If you want to keep the \"!p[len]\" condition, then yes. If we want to\n> properly quote internal NULs (which again, I am not sure is of practical\n> use), then you would also have to make that condition \"(maxlen >= 0 &&\n> !p[len])\".  Which is really not that bad, but I was trying to make it\n> easier to read. I am OK with any of the three combinations of fixes.\n> \n> And the fact that I am using the word \"combination\" probably means it\n> should be a separate patch anyway.\n> \n> So here is the minimal patch to go on 'maint', for Junio's convenience.\n\nAnd here is what the \"quote embedded NULs\" patch would look like on top.\nIt's actually pretty straightforward, but the more I think of it, the\nmore I think it is probably not worthwhile. Not only are we quoting\npaths, which should not have embedded NULs, but it requires that the\ncaller always pass the length explicitly, and clearly we are not doing\nthat all or even most of the time. So while this would fix the low-level\n\"this function quotes an arbitrary string\" case, for it to be of any use\nall of the code paths leading to it would need to be audited to handle\nNUL-embedded strings.\n\n---\ndiff --git a/quote.c b/quote.c\nindex 723bb4f..fc93435 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -213,7 +213,7 @@ static size_t quote_c_style_counted(const char *name, ssize_t maxlen,\n \t\tint ch;\n \n \t\tlen = next_quote_pos(p, maxlen);\n-\t\tif (len == maxlen || !p[len])\n+\t\tif (len == maxlen || (maxlen < 0 && !p[len]))\n \t\t\tbreak;\n \n \t\tif (!no_dq && p == name)\n"},{"id":"133267","messageId":"7v3a1k50sw.fsf@alter.siamese.dyndns.org","threadId":"22468","inReplyTo":"20100201134454.GA796@coredump.intra.peff.net","subject":"Re: Problem listing GIT repository with accents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-01T17:21:51Z","receivedAt":"2010-02-01T17:21:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> And here is what the \"quote embedded NULs\" patch would look like on top.\n> It's actually pretty straightforward, but the more I think of it, the\n> more I think it is probably not worthwhile. Not only are we quoting\n> paths, which should not have embedded NULs, but it requires that the\n> caller always pass the length explicitly, and clearly we are not doing\n> that all or even most of the time. So while this would fix the low-level\n> \"this function quotes an arbitrary string\" case, for it to be of any use\n> all of the code paths leading to it would need to be audited to handle\n> NUL-embedded strings.\n\nThanks; I think your analysis is very sound.\n\nThe current callers do not care (for a good reason).  They are dealing\nwith a pathname, and either they are feeding a string on which there is a\npathname followed by something else and they know where that something\nelse begins (they give maxlen because they don't want to or cannot NUL\nterminate the string in place while calling this function), or they know\nthey want to quote to the end of the string but they haven't counted how\nlong it is (they say \"I don't care---just quote to the end\").  In either\nway, they don't expect giving too long a maxlen will go beyond the end of\nthe string past the terminating NUL.\n\nUnless we document \"this function is to C-quote a (portion of a) string,\neither to the end or up to the given length\", however, future callers may\nincorrectly assume that with length the function can be fed anything and\nwould C-quote that piece of memory.  The argument name \"const char *name\"\nalready suggests that is not an arbitrary binary rubbish, changing that to\n\"str\" would probably make that a bit stronger documentation, or we could\nexplicitly say \"this is a (early part of a) NUL-terminated string\" in a\ncomment.\n\nBut your one-liner patch would actually be a smaller change than any of\nthem and makes the whole problem disappear; wouldn't it be a far better\nsolution?\n"},{"id":"133270","messageId":"20100201174045.GA5756@coredump.intra.peff.net","threadId":"22468","inReplyTo":"7v3a1k50sw.fsf@alter.siamese.dyndns.org","subject":"Re: Problem listing GIT repository with accents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-02-01T17:40:45Z","receivedAt":"2010-02-01T17:40:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 01, 2010 at 09:21:51AM -0800, Junio C Hamano wrote:\n\n> Unless we document \"this function is to C-quote a (portion of a) string,\n> either to the end or up to the given length\", however, future callers may\n> incorrectly assume that with length the function can be fed anything and\n> would C-quote that piece of memory.  The argument name \"const char *name\"\n> already suggests that is not an arbitrary binary rubbish, changing that to\n> \"str\" would probably make that a bit stronger documentation, or we could\n> explicitly say \"this is a (early part of a) NUL-terminated string\" in a\n> comment.\n> \n> But your one-liner patch would actually be a smaller change than any of\n> them and makes the whole problem disappear; wouldn't it be a far better\n> solution?\n\nSure, if you are going to bother to document it to future-proof against\nnew callers, you might as well just make it more flexible with my\none-liner. I don't think it will impact the behavior of any existing\ncallers either way.\n\nI was just going to not bother, but perhaps while we have spent some\nbrain cycles on it, it is better to just fix it. Either way is fine with\nme.\n\n-Peff\n"}]}