{"thread":{"id":"40742","subject":"[PATCH] wt-status: use strncmp() for length-limited string comparison","startedAt":"2015-11-06T22:47:03Z","lastAt":"2015-11-28T17:31:31Z","messageCount":8,"participants":["René Scharfe","Jeff King","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"273033","messageId":"563D2DE7.1030005@web.de","threadId":"40742","inReplyTo":null,"subject":"[PATCH] wt-status: use strncmp() for length-limited string comparison","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-06T22:47:03Z","receivedAt":"2015-11-06T22:47:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"When a branch name is longer than four characters, memcmp() can read\npast the end of the string literal \"HEAD\". Use strncmp() instead, which\nstops at the end of a string. This fixes the following test failures\nwith AddressSanitizer:\n\nt3203-branch-output.sh (Wstat: 256 Tests: 18 Failed: 4)\n Failed tests: 12, 15-17\n Non-zero exit status: 1\nt3412-rebase-root.sh (Wstat: 256 Tests: 31 Failed: 3)\n Failed tests: 28-29, 31\n Non-zero exit status: 1\nt3507-cherry-pick-conflict.sh (Wstat: 256 Tests: 31 Failed: 4)\n Failed tests: 14, 29-31\n Non-zero exit status: 1\nt3510-cherry-pick-sequence.sh (Wstat: 256 Tests: 39 Failed: 14)\n Failed tests: 17, 22-26, 28-30, 34-35, 37-39\n Non-zero exit status: 1\nt3420-rebase-autostash.sh (Wstat: 256 Tests: 28 Failed: 4)\n Failed tests: 24-27\n Non-zero exit status: 1\nt3404-rebase-interactive.sh (Wstat: 256 Tests: 91 Failed: 57)\n Failed tests: 17, 19, 21-42, 44, 46-74, 77, 81-82\n Non-zero exit status: 1\nt3900-i18n-commit.sh (Wstat: 256 Tests: 34 Failed: 1)\n Failed test: 34\n Non-zero exit status: 1\nt5407-post-rewrite-hook.sh (Wstat: 256 Tests: 14 Failed: 6)\n Failed tests: 9-14\n Non-zero exit status: 1\nt7001-mv.sh (Wstat: 256 Tests: 46 Failed: 5)\n Failed tests: 39-43\n Non-zero exit status: 1\nt7509-commit.sh (Wstat: 256 Tests: 12 Failed: 2)\n Failed tests: 11-12\n Non-zero exit status: 1\nt7512-status-help.sh (Wstat: 256 Tests: 39 Failed: 35)\n Failed tests: 5-39\n Non-zero exit status: 1\nt6030-bisect-porcelain.sh (Wstat: 256 Tests: 70 Failed: 1)\n Failed test: 13\n Non-zero exit status: 1\n\nSigned-off-by: Rene Scharfe \n---\n wt-status.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 435fc28..8dc281b 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1319,7 +1319,7 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,\n \thashcpy(cb->nsha1, nsha1);\n \tfor (end = target; *end && *end != '\\n'; end++)\n \t\t;\n-\tif (!memcmp(target, \"HEAD\", end - target)) {\n+\tif (!strncmp(target, \"HEAD\", end - target)) {\n \t\t/* HEAD is relative. Resolve it to the right reflog entry. */\n \t\tstrbuf_addstr(&cb->buf,\n \t\t\t find_unique_abbrev(nsha1, DEFAULT_ABBREV));\n-- \n2.6.3\n"},{"id":"273656","messageId":"20151124213601.GB29185@sigill.intra.peff.net","threadId":"40742","inReplyTo":"563D2DE7.1030005@web.de","subject":"Re: [PATCH] wt-status: use strncmp() for length-limited string comparison","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-24T21:36:01Z","receivedAt":"2015-11-24T21:36:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 06, 2015 at 11:47:03PM +0100, René Scharfe wrote:\n\n> When a branch name is longer than four characters, memcmp() can read\n> past the end of the string literal \"HEAD\". Use strncmp() instead, which\n> stops at the end of a string. This fixes the following test failures\n> with AddressSanitizer:\n\nHmm. I think this is mostly harmless, as a comparison like:\n\n memcmp(\"HEAD and more\", \"HEAD\", strlen(\"HEAD\"))\n\nwould yield non-zero when we compare the NUL in the second string to\nwhatever is in the first. So I assume what is going on is that memcmp is\ndoing larger compares than byte by byte, and is examining 4 or 8 bytes\nstarting at that NUL.\n\nThe outcome is equivalent, but we do touch memory that is not ours, so I\nthink this is a positive direction in that sense.\n\nBut...\n\n> diff --git a/wt-status.c b/wt-status.c\n> index 435fc28..8dc281b 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1319,7 +1319,7 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,\n> \thashcpy(cb->nsha1, nsha1);\n> \tfor (end = target; *end && *end != '\\n'; end++)\n> \t\t;\n> -\tif (!memcmp(target, \"HEAD\", end - target)) {\n> +\tif (!strncmp(target, \"HEAD\", end - target)) {\n\nThis will match prefixes like \"HEA\" in the target, won't it?\n\nI think you want something more like:\n\n if (end - target == 4 && !memcmp(target, \"HEAD\", 4))\n\nI tried to think of a way that didn't involve a magic number. The best I\ncame up with is:\n\n if (skip_prefix(target, \"HEAD\", &v) && v == end)\n\nbut that requires an extra variable, and is arguably more obfuscated.\n\n-Peff\n"},{"id":"273698","messageId":"56551A11.9030809@web.de","threadId":"40742","inReplyTo":"20151124213601.GB29185@sigill.intra.peff.net","subject":"Re: [PATCH] wt-status: use strncmp() for length-limited string comparison","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-25T02:16:49Z","receivedAt":"2015-11-25T02:16:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.11.2015 um 22:36 schrieb Jeff King:\n> On Fri, Nov 06, 2015 at 11:47:03PM +0100, René Scharfe wrote:\n> \n>> When a branch name is longer than four characters, memcmp() can read\n>> past the end of the string literal \"HEAD\". Use strncmp() instead, which\n>> stops at the end of a string. This fixes the following test failures\n>> with AddressSanitizer:\n> \n> Hmm. I think this is mostly harmless, as a comparison like:\n> \n> memcmp(\"HEAD and more\", \"HEAD\", strlen(\"HEAD\"))\n> \n> would yield non-zero when we compare the NUL in the second string to\n> whatever is in the first. So I assume what is going on is that memcmp is\n> doing larger compares than byte by byte, and is examining 4 or 8 bytes\n> starting at that NUL.\n> \n> The outcome is equivalent, but we do touch memory that is not ours, so I\n> think this is a positive direction in that sense.\n\nYes, except it should be strlen(\"HEAD and more\") in your example code;\nwith strlen(\"HEAD\") it would compare just 4 bytes and return 0.\n\n> But...\n> \n>> diff --git a/wt-status.c b/wt-status.c\n>> index 435fc28..8dc281b 100644\n>> --- a/wt-status.c\n>> +++ b/wt-status.c\n>> @@ -1319,7 +1319,7 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,\n>> \thashcpy(cb->nsha1, nsha1);\n>> \tfor (end = target; *end && *end != '\\n'; end++)\n>> \t\t;\n>> -\tif (!memcmp(target, \"HEAD\", end - target)) {\n>> +\tif (!strncmp(target, \"HEAD\", end - target)) {\n> \n> This will match prefixes like \"HEA\" in the target, won't it?\n\nOww, yes. :-/ NB: The existing code does the same.\n\n> I think you want something more like:\n> \n> if (end - target == 4 && !memcmp(target, \"HEAD\", 4))\n> \n> I tried to think of a way that didn't involve a magic number. The best I\n> came up with is:\n> \n> if (skip_prefix(target, \"HEAD\", &v) && v == end)\n> \n> but that requires an extra variable, and is arguably more obfuscated.\n\nUsing one more variable isn't that bad, as long as it gets a fitting\nname. Or we could reuse \"end\" (I'm not worrying about scanning \"HEAD\"\ntwice very much):\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 435fc28..96a731e 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1317,14 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,\n \ttarget += strlen(\" to \");\n \tstrbuf_reset(&cb->buf);\n \thashcpy(cb->nsha1, nsha1);\n-\tfor (end = target; *end && *end != '\\n'; end++)\n-\t\t;\n-\tif (!memcmp(target, \"HEAD\", end - target)) {\n+\tif (skip_prefix(target, \"HEAD\", &end) && (!*end || *end == '\\n')) {\n \t\t/* HEAD is relative. Resolve it to the right reflog entry. */\n \t\tstrbuf_addstr(&cb->buf,\n \t\t\t find_unique_abbrev(nsha1, DEFAULT_ABBREV));\n \t\treturn 1;\n \t}\n+\tfor (end = target; *end && *end != '\\n'; end++)\n+\t\t;\n \tstrbuf_add(&cb->buf, target, end - target);\n \treturn 1;\n }\n"},{"id":"273714","messageId":"20151125091503.GA1779@sigill.intra.peff.net","threadId":"40742","inReplyTo":"56551A11.9030809@web.de","subject":"Re: [PATCH] wt-status: use strncmp() for length-limited string comparison","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-25T09:15:03Z","receivedAt":"2015-11-25T09:15:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 25, 2015 at 03:16:49AM +0100, René Scharfe wrote:\n\n> > Hmm. I think this is mostly harmless, as a comparison like:\n> > \n> > memcmp(\"HEAD and more\", \"HEAD\", strlen(\"HEAD\"))\n> [...]\n> \n> Yes, except it should be strlen(\"HEAD and more\") in your example code;\n> with strlen(\"HEAD\") it would compare just 4 bytes and return 0.\n\nWhoops, yeah. Thank you for figuring out what I meant. :)\n\n> Using one more variable isn't that bad, as long as it gets a fitting\n> name. Or we could reuse \"end\" (I'm not worrying about scanning \"HEAD\"\n> twice very much):\n> \n> diff --git a/wt-status.c b/wt-status.c\n> index 435fc28..96a731e 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1317,14 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,\n> \ttarget += strlen(\" to \");\n> \tstrbuf_reset(&cb->buf);\n> \thashcpy(cb->nsha1, nsha1);\n> -\tfor (end = target; *end && *end != '\\n'; end++)\n> -\t\t;\n> -\tif (!memcmp(target, \"HEAD\", end - target)) {\n> +\tif (skip_prefix(target, \"HEAD\", &end) && (!*end || *end == '\\n')) {\n> \t\t/* HEAD is relative. Resolve it to the right reflog entry. */\n> \t\tstrbuf_addstr(&cb->buf,\n> \t\t\t find_unique_abbrev(nsha1, DEFAULT_ABBREV));\n> \t\treturn 1;\n> \t}\n\nYeah, I think parsing left-to-right like this makes things much more\nobvious. And regarding scanning HEAD twice, I think we already do that\n(we find the trailing newline first in the current code). Though I agree\nthat is absurd premature optimization.\n\n> +\tfor (end = target; *end && *end != '\\n'; end++)\n> +\t\t;\n\nThis loop (which I know you just moved, not wrote) is basically\nstrchrnul, isn't it? That might be more readable.\n\n-Peff\n"},{"id":"273721","messageId":"vpqtwoawewc.fsf@anie.imag.fr","threadId":"40742","inReplyTo":"20151125091503.GA1779@sigill.intra.peff.net","subject":"Re: [PATCH] wt-status: use strncmp() for length-limited string comparison","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-11-25T10:29:39Z","receivedAt":"2015-11-25T10:29:39Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n>> diff --git a/wt-status.c b/wt-status.c\n>> index 435fc28..96a731e 100644\n>> --- a/wt-status.c\n>> +++ b/wt-status.c\n>> @@ -1317,14 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,\n>> \ttarget += strlen(\" to \");\n>> \tstrbuf_reset(&cb->buf);\n>> \thashcpy(cb->nsha1, nsha1);\n>> -\tfor (end = target; *end && *end != '\\n'; end++)\n>> -\t\t;\n>> -\tif (!memcmp(target, \"HEAD\", end - target)) {\n>> +\tif (skip_prefix(target, \"HEAD\", &end) && (!*end || *end == '\\n')) {\n>> \t\t/* HEAD is relative. Resolve it to the right reflog entry. */\n>> \t\tstrbuf_addstr(&cb->buf,\n>> \t\t\t find_unique_abbrev(nsha1, DEFAULT_ABBREV));\n>> \t\treturn 1;\n>> \t}\n>\n> Yeah, I think parsing left-to-right like this makes things much more\n> obvious.\n\nAgreed.\n\n>> +\tfor (end = target; *end && *end != '\\n'; end++)\n>> +\t\t;\n>\n> This loop (which I know you just moved, not wrote) is basically\n> strchrnul, isn't it? That might be more readable.\n\nAgreed too.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"273736","messageId":"5655C14A.1060308@web.de","threadId":"40742","inReplyTo":"20151125091503.GA1779@sigill.intra.peff.net","subject":"[PATCH v2] wt-status: correct and simplify check for detached HEAD","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-11-25T14:10:18Z","receivedAt":"2015-11-25T14:10:18Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If a branch name is longer than four characters then memcmp() reads over\nthe end of the static string \"HEAD\". This causes the following test\nfailures with AddressSanitizer:\n\nt3203-branch-output.sh (Wstat: 256 Tests: 18 Failed: 4)\n Failed tests: 12, 15-17\n Non-zero exit status: 1\nt3412-rebase-root.sh (Wstat: 256 Tests: 31 Failed: 3)\n Failed tests: 28-29, 31\n Non-zero exit status: 1\nt3507-cherry-pick-conflict.sh (Wstat: 256 Tests: 31 Failed: 4)\n Failed tests: 14, 29-31\n Non-zero exit status: 1\nt3510-cherry-pick-sequence.sh (Wstat: 256 Tests: 39 Failed: 14)\n Failed tests: 17, 22-26, 28-30, 34-35, 37-39\n Non-zero exit status: 1\nt3420-rebase-autostash.sh (Wstat: 256 Tests: 28 Failed: 4)\n Failed tests: 24-27\n Non-zero exit status: 1\nt3404-rebase-interactive.sh (Wstat: 256 Tests: 91 Failed: 57)\n Failed tests: 17, 19, 21-42, 44, 46-74, 77, 81-82\n Non-zero exit status: 1\nt3900-i18n-commit.sh (Wstat: 256 Tests: 34 Failed: 1)\n Failed test: 34\n Non-zero exit status: 1\nt5407-post-rewrite-hook.sh (Wstat: 256 Tests: 14 Failed: 6)\n Failed tests: 9-14\n Non-zero exit status: 1\nt7001-mv.sh (Wstat: 256 Tests: 46 Failed: 5)\n Failed tests: 39-43\n Non-zero exit status: 1\nt7509-commit.sh (Wstat: 256 Tests: 12 Failed: 2)\n Failed tests: 11-12\n Non-zero exit status: 1\nt7512-status-help.sh (Wstat: 256 Tests: 39 Failed: 35)\n Failed tests: 5-39\n Non-zero exit status: 1\nt6030-bisect-porcelain.sh (Wstat: 256 Tests: 70 Failed: 1)\n Failed test: 13\n Non-zero exit status: 1\n\nAnd if a branch is named \"H\", \"HE\", or \"HEA\" then the current if clause\nerroneously considers it as matching \"HEAD\" because it only compares\nup to the end of the branch name.\n\nFix that by doing the comparison using strcmp() and only after the\nbranch name is extracted. This way neither too less nor too many\ncharacters are checked. While at it call strchrnul() to find the end\nof the branch name instead of open-coding it.\n\nHelped-by: Jeff King \nSigned-off-by: Rene Scharfe \n---\nWe can be more careful when parsing -- or avoid parsing and backtrack\nif we found \"HEAD\" after all. The latter is simpler, and string\nparsing is tricky enough that we better take such opportunities to\nsimplify the code..\n\nChanges since v1:\n* strcmp() instead of strncmp()\n* strchrnul() (unrelated cleanup)\n* adjusted subject (and commit message)\n\n wt-status.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 435fc28..ced53dd 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1317,15 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,\n \ttarget += strlen(\" to \");\n \tstrbuf_reset(&cb->buf);\n \thashcpy(cb->nsha1, nsha1);\n-\tfor (end = target; *end && *end != '\\n'; end++)\n-\t\t;\n-\tif (!memcmp(target, \"HEAD\", end - target)) {\n+\tend = strchrnul(target, '\\n');\n+\tstrbuf_add(&cb->buf, target, end - target);\n+\tif (!strcmp(cb->buf.buf, \"HEAD\")) {\n \t\t/* HEAD is relative. Resolve it to the right reflog entry. */\n+\t\tstrbuf_reset(&cb->buf);\n \t\tstrbuf_addstr(&cb->buf,\n \t\t\t find_unique_abbrev(nsha1, DEFAULT_ABBREV));\n-\t\treturn 1;\n \t}\n-\tstrbuf_add(&cb->buf, target, end - target);\n \treturn 1;\n }\n \n-- \n2.6.3\n"},{"id":"273741","messageId":"vpqvb8qt5hr.fsf@anie.imag.fr","threadId":"40742","inReplyTo":"5655C14A.1060308@web.de","subject":"Re: [PATCH v2] wt-status: correct and simplify check for detached HEAD","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-11-25T16:21:04Z","receivedAt":"2015-11-25T16:21:04Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"René Scharfe writes:\n\n> diff --git a/wt-status.c b/wt-status.c\n> index 435fc28..ced53dd 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1317,15 +1317,14 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,\n> \ttarget += strlen(\" to \");\n> \tstrbuf_reset(&cb->buf);\n> \thashcpy(cb->nsha1, nsha1);\n> -\tfor (end = target; *end && *end != '\\n'; end++)\n> -\t\t;\n> -\tif (!memcmp(target, \"HEAD\", end - target)) {\n> +\tend = strchrnul(target, '\\n');\n> +\tstrbuf_add(&cb->buf, target, end - target);\n> +\tif (!strcmp(cb->buf.buf, \"HEAD\")) {\n> \t\t/* HEAD is relative. Resolve it to the right reflog entry. */\n> +\t\tstrbuf_reset(&cb->buf);\n> \t\tstrbuf_addstr(&cb->buf,\n> \t\t\t find_unique_abbrev(nsha1, DEFAULT_ABBREV));\n> -\t\treturn 1;\n> \t}\n> -\tstrbuf_add(&cb->buf, target, end - target);\n> \treturn 1;\n> }\n\nIndeed, the code is much simpler like this than with the previous\nattempts. Looks all right to me.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"273816","messageId":"20151128173131.GF27264@sigill.intra.peff.net","threadId":"40742","inReplyTo":"5655C14A.1060308@web.de","subject":"Re: [PATCH v2] wt-status: correct and simplify check for detached HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-28T17:31:31Z","receivedAt":"2015-11-28T17:31:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 25, 2015 at 03:10:18PM +0100, René Scharfe wrote:\n\n> Fix that by doing the comparison using strcmp() and only after the\n> branch name is extracted. This way neither too less nor too many\n> characters are checked. While at it call strchrnul() to find the end\n> of the branch name instead of open-coding it.\n> \n> Helped-by: Jeff King \n> Signed-off-by: Rene Scharfe \n> ---\n> We can be more careful when parsing -- or avoid parsing and backtrack\n> if we found \"HEAD\" after all. The latter is simpler, and string\n> parsing is tricky enough that we better take such opportunities to\n> simplify the code..\n\nYeah, I think this is nicer. We end up allocating either way anyway, so\nthe only extra effort is copy the string.\n\n-Peff\n"}]}