{"thread":{"id":"33100","subject":"[PATCH] setup.c: Fix prefix_pathspec from looping pass end of string","startedAt":"2013-03-07T16:36:03Z","lastAt":"2013-03-09T23:46:00Z","messageCount":9,"participants":["Andrew Wong","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"210771","messageId":"1362674163-24682-1-git-send-email-andrew.kw.w@gmail.com","threadId":"33100","inReplyTo":null,"subject":"[PATCH] setup.c: Fix prefix_pathspec from looping pass end of string","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2013-03-07T16:36:03Z","receivedAt":"2013-03-07T16:36:03Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"The previous code was assuming length ends at either `)` or `,`, and was\nnot handling the case where strcspn returns length due to end of string.\nSo specifying \":(top\" as pathspec will cause the loop to go pass the end\nof string.\n\nSigned-off-by: Andrew Wong \n---\n setup.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 1dee47e..f4c4e73 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -207,9 +207,11 @@ static const char *prefix_pathspec(const char *prefix, int prefixlen, const char\n \t\t *copyfrom && *copyfrom != ')';\n \t\t copyfrom = nextat) {\n \t\t\tsize_t len = strcspn(copyfrom, \",)\");\n-\t\t\tif (copyfrom[len] == ')')\n+\t\t\tif (copyfrom[len] == '\\0')\n \t\t\t\tnextat = copyfrom + len;\n-\t\t\telse\n+\t\t\telse if (copyfrom[len] == ')')\n+\t\t\t\tnextat = copyfrom + len;\n+\t\t\telse if (copyfrom[len] == ',')\n \t\t\t\tnextat = copyfrom + len + 1;\n \t\t\tif (!len)\n \t\t\t\tcontinue;\n-- \n1.8.2.rc0.22.gb3600c3\n"},{"id":"210789","messageId":"7vobeulw4d.fsf@alter.siamese.dyndns.org","threadId":"33100","inReplyTo":"1362674163-24682-1-git-send-email-andrew.kw.w@gmail.com","subject":"Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-07T21:48:18Z","receivedAt":"2013-03-07T21:48:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Wong writes:\n\n> The previous code was assuming length ends at either `)` or `,`, and was\n> not handling the case where strcspn returns length due to end of string.\n> So specifying \":(top\" as pathspec will cause the loop to go pass the end\n> of string.\n\nThanks.\n\nThe parser that goes past the end of the string may be a bug worth\nfixing, but is this patch sufficient to diagnose such an input as an\nerror?\n\n\n\n\n> Signed-off-by: Andrew Wong \n> ---\n> setup.c | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index 1dee47e..f4c4e73 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -207,9 +207,11 @@ static const char *prefix_pathspec(const char *prefix, int prefixlen, const char\n> \t\t *copyfrom && *copyfrom != ')';\n> \t\t copyfrom = nextat) {\n> \t\t\tsize_t len = strcspn(copyfrom, \",)\");\n> -\t\t\tif (copyfrom[len] == ')')\n> +\t\t\tif (copyfrom[len] == '\\0')\n> \t\t\t\tnextat = copyfrom + len;\n> -\t\t\telse\n> +\t\t\telse if (copyfrom[len] == ')')\n> +\t\t\t\tnextat = copyfrom + len;\n> +\t\t\telse if (copyfrom[len] == ',')\n> \t\t\t\tnextat = copyfrom + len + 1;\n> \t\t\tif (!len)\n> \t\t\t\tcontinue;\n"},{"id":"210794","messageId":"CADgNjakrBCD2jMNUz95E-7FkyKmNgcQeuz8grDWczb-hM6yHhg@mail.gmail.com","threadId":"33100","inReplyTo":"7vobeulw4d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2013-03-07T22:25:46Z","receivedAt":"2013-03-07T22:25:46Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"On 3/7/13, Junio C Hamano wrote:\n> The parser that goes past the end of the string may be a bug worth\n> fixing, but is this patch sufficient to diagnose such an input as an\n> error?\n\nYea, the patch should fix the passing end of string too. The parser\nwas going past end of string because the nextat is set to \"copyfrom +\nlen + 1\" for the '\\0' case too. Then \"+ 1\" causes the parser to go\npass end of string. If we handle the '\\0' case separately, then the\nparser ends properly, and shouldn't be able to go pass the end of\nstring.\n\nHm, should I be paranoid and put an \"else\" clause to call die() as\nwell? In case there's a scenario where none of the 3 cases is true...\n\nAndrew\n"},{"id":"210803","messageId":"7vvc92kbho.fsf@alter.siamese.dyndns.org","threadId":"33100","inReplyTo":"CADgNjakrBCD2jMNUz95E-7FkyKmNgcQeuz8grDWczb-hM6yHhg@mail.gmail.com","subject":"Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-07T23:59:15Z","receivedAt":"2013-03-07T23:59:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Wong writes:\n\n> On 3/7/13, Junio C Hamano wrote:\n>> The parser that goes past the end of the string may be a bug worth\n>> fixing, but is this patch sufficient to diagnose such an input as an\n>> error?\n>\n> Yea, the patch should fix the passing end of string too. The parser\n> was going past end of string because the nextat is set to \"copyfrom +\n> len + 1\" for the '\\0' case too. Then \"+ 1\" causes the parser to go\n> pass end of string. If we handle the '\\0' case separately, then the\n> parser ends properly, and shouldn't be able to go pass the end of\n> string.\n\nThis did not error out for me, though.\n\n $ cd t && git ls-files \":(top\"\n"},{"id":"210805","messageId":"CADgNja=8f+_ORb_WStRz2grr0pYmJ2gZTnCHbOGUb3ogPPd_LQ@mail.gmail.com","threadId":"33100","inReplyTo":"7vvc92kbho.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2013-03-08T00:25:36Z","receivedAt":"2013-03-08T00:25:36Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"On 3/7/13, Junio C Hamano wrote:\n> This did not error out for me, though.\n>\n> $ cd t && git ls-files \":(top\"\n\nNo error message at all? Hm, maybe in your case, the byte after the\nend of string happens to be '\\0' and the loop ended by chance?\n\ngit doesn't crash for me, but it generates this error:\n $ git ls-files \":(top\"\n fatal: Invalid pathspec magic 'LS_COLORS=' in ':(top'\n\nThe loop runs for a second time after parsing \"top\", and copyfrom now\npoints to the byte after \":(top\", which is coming from argv. And in my\ndistribution/platform, it looks like the envp, the third param of\nmain(), is packed right after the argv strings, because:\n $ env | head -n 1\n LS_COLORS=\n"},{"id":"210806","messageId":"7vk3pik6aq.fsf@alter.siamese.dyndns.org","threadId":"33100","inReplyTo":"CADgNja=8f+_ORb_WStRz2grr0pYmJ2gZTnCHbOGUb3ogPPd_LQ@mail.gmail.com","subject":"Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-08T01:51:25Z","receivedAt":"2013-03-08T01:51:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrew Wong writes:\n\n> On 3/7/13, Junio C Hamano wrote:\n>> This did not error out for me, though.\n>>\n>> $ cd t && git ls-files \":(top\"\n>\n> No error message at all? Hm, maybe in your case, the byte after the\n> end of string happens to be '\\0' and the loop ended by chance?\n>\n> git doesn't crash for me, but it generates this error:\n> $ git ls-files \":(top\"\n> fatal: Invalid pathspec magic 'LS_COLORS=' in ':(top'\n\nWhat I meant was that I do not get any error _after_ applying your\npatch.\n\nIt is broken to behave as if \"LS_COLORS=...\" (which is totally\nunrelated string that happens to be laid out next in the memory) is\na part of the pathspec magic specification your \":(top\" started.\nYour patch makes the code stop doing that.\n\nBut it is equally broken to behave as if there is nothing wrong in\nthe incomplete magic \":(top\" that is not closed, isn't it?\n"},{"id":"210807","messageId":"513945CA.6070302@gmail.com","threadId":"33100","inReplyTo":"7vk3pik6aq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2013-03-08T01:58:34Z","receivedAt":"2013-03-08T01:58:34Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"On 03/07/13 20:51, Junio C Hamano wrote:\n> But it is equally broken to behave as if there is nothing wrong in\n> the incomplete magic \":(top\" that is not closed, isn't it?\nAh, yea, I did notice that, but then I saw a few lines below:\n if (*copyfrom == ')')\n copyfrom++;\nwhich is explicitly making the \")\" optional. So I thought maybe that was\nthe original intention, and left it at that. Though the doc says to end\nwith \")\", so I guess it should error out after all? If that's the case,\nI can try to come up with a patch to error it out (through die() ?).\n"},{"id":"210942","messageId":"1362872760-25803-1-git-send-email-andrew.kw.w@gmail.com","threadId":"33100","inReplyTo":"7vk3pik6aq.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/2] setup.c: Fix prefix_pathspec from looping pass end of string","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2013-03-09T23:45:59Z","receivedAt":"2013-03-09T23:45:59Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"The previous code was assuming length ends at either \")\" or \",\", and was\nnot handling the case where strcspn returns length due to end of string.\nSo specifying \":(top\" as pathspec will cause the loop to go pass the end\nof string.\n\nSigned-off-by: Andrew Wong \n---\n setup.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 1dee47e..f4c4e73 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -207,9 +207,11 @@ static const char *prefix_pathspec(const char *prefix, int prefixlen, const char\n \t\t *copyfrom && *copyfrom != ')';\n \t\t copyfrom = nextat) {\n \t\t\tsize_t len = strcspn(copyfrom, \",)\");\n-\t\t\tif (copyfrom[len] == ')')\n+\t\t\tif (copyfrom[len] == '\\0')\n \t\t\t\tnextat = copyfrom + len;\n-\t\t\telse\n+\t\t\telse if (copyfrom[len] == ')')\n+\t\t\t\tnextat = copyfrom + len;\n+\t\t\telse if (copyfrom[len] == ',')\n \t\t\t\tnextat = copyfrom + len + 1;\n \t\t\tif (!len)\n \t\t\t\tcontinue;\n-- \n1.7.12.4\n"},{"id":"210943","messageId":"1362872760-25803-2-git-send-email-andrew.kw.w@gmail.com","threadId":"33100","inReplyTo":"1362872760-25803-1-git-send-email-andrew.kw.w@gmail.com","subject":"[PATCH 2/2] setup.c: Check that the pathspec magic ends with \")\"","fromName":"Andrew Wong","fromEmail":"andrew.kw.w@gmail.com","sentAt":"2013-03-09T23:46:00Z","receivedAt":"2013-03-09T23:46:00Z","isPatch":true,"sender":{"key":"andrew.kw.w@gmail.com","avatar":"https://avatars.githubusercontent.com/u/489311?v=4"},"body":"The previous code allowed the \")\" to be optional.\n\nSigned-off-by: Andrew Wong \n---\n setup.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex f4c4e73..5ed2b93 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -225,8 +225,9 @@ static const char *prefix_pathspec(const char *prefix, int prefixlen, const char\n \t\t\t\tdie(\"Invalid pathspec magic '%.*s' in '%s'\",\n \t\t\t\t (int) len, copyfrom, elt);\n \t\t}\n-\t\tif (*copyfrom == ')')\n-\t\t\tcopyfrom++;\n+\t\tif (*copyfrom != ')')\n+\t\t\tdie(\"Missing ')' at the end of pathspec magic in '%s'\", elt);\n+\t\tcopyfrom++;\n \t} else {\n \t\t/* shorthand */\n \t\tfor (copyfrom = elt + 1;\n-- \n1.7.12.4\n"}]}