{"thread":{"id":"46328","subject":"[PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","startedAt":"2017-07-08T08:59:08Z","lastAt":"2017-07-09T17:05:55Z","messageCount":12,"participants":["René Scharfe","Andreas Schwab","Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"323986","messageId":"d1bb978b-a7df-48a8-15c8-80730c77e11c@web.de","threadId":"46328","inReplyTo":null,"subject":"[PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-08T08:58:42Z","receivedAt":"2017-07-08T08:59:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid running over the end of another -- a C string whose length we\ndon't know -- by using strcmp(3) instead of memcmp(3) for comparing it\nwith another C string.\n\nSigned-off-by: Rene Scharfe \n---\n apply.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 946be4d2f5..9b3df8a3aa 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -962,13 +962,12 @@ static int gitdiff_verify_name(struct apply_state *state,\n \t}\n \n \tif (*name) {\n-\t\tint len = strlen(*name);\n \t\tchar *another;\n \t\tif (isnull)\n \t\t\treturn error(_(\"git apply: bad git-diff - expected /dev/null, got %s on line %d\"),\n \t\t\t\t *name, state->linenr);\n \t\tanother = find_name(state, line, NULL, state->p_value, TERM_TAB);\n-\t\tif (!another || memcmp(another, *name, len + 1)) {\n+\t\tif (!another || strcmp(another, *name)) {\n \t\t\tfree(another);\n \t\t\treturn error((side == DIFF_NEW_NAME) ?\n \t\t\t _(\"git apply: bad git-diff - inconsistent new filename on line %d\") :\n-- \n2.13.2\n"},{"id":"323991","messageId":"87tw2ngp94.fsf@linux-m68k.org","threadId":"46328","inReplyTo":"d1bb978b-a7df-48a8-15c8-80730c77e11c@web.de","subject":"Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2017-07-08T11:08:23Z","receivedAt":"2017-07-08T11:08:32Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Jul 08 2017, René Scharfe wrote:\n\n> Avoid running over the end of another -- a C string whose length we\n> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it\n> with another C string.\n\nThat's not a good justification for the change, since memcmp never reads\npast the differing characters.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"323992","messageId":"d83d2395-e667-9e52-cc94-af5fbec6054c@ramsayjones.plus.com","threadId":"46328","inReplyTo":"d1bb978b-a7df-48a8-15c8-80730c77e11c@web.de","subject":"Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-07-08T11:08:34Z","receivedAt":"2017-07-08T11:08:42Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 08/07/17 09:58, René Scharfe wrote:\n> Avoid running over the end of another -- a C string whose length we\n> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it\n> with another C string.\n\nI had to read this twice, along with the patch text, before this\nmade any sense. ;-) The missing information being that 'another'\nwas the name of the string variable that we were potentially\n'running over the end of'.\n\nATB,\nRamsay Jones\n\n> \n> Signed-off-by: Rene Scharfe \n> ---\n> apply.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/apply.c b/apply.c\n> index 946be4d2f5..9b3df8a3aa 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -962,13 +962,12 @@ static int gitdiff_verify_name(struct apply_state *state,\n> \t}\n> \n> \tif (*name) {\n> -\t\tint len = strlen(*name);\n> \t\tchar *another;\n> \t\tif (isnull)\n> \t\t\treturn error(_(\"git apply: bad git-diff - expected /dev/null, got %s on line %d\"),\n> \t\t\t\t *name, state->linenr);\n> \t\tanother = find_name(state, line, NULL, state->p_value, TERM_TAB);\n> -\t\tif (!another || memcmp(another, *name, len + 1)) {\n> +\t\tif (!another || strcmp(another, *name)) {\n> \t\t\tfree(another);\n> \t\t\treturn error((side == DIFF_NEW_NAME) ?\n> \t\t\t _(\"git apply: bad git-diff - inconsistent new filename on line %d\") :\n> \n"},{"id":"323993","messageId":"e46a7de9-3e0e-4eeb-c9a3-a8b5f22620b4@web.de","threadId":"46328","inReplyTo":"87tw2ngp94.fsf@linux-m68k.org","subject":"Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-08T11:43:02Z","receivedAt":"2017-07-08T11:43:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.07.2017 um 13:08 schrieb Andreas Schwab:\n> On Jul 08 2017, René Scharfe wrote:\n> \n>> Avoid running over the end of another -- a C string whose length we\n>> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it\n>> with another C string.\n> \n> That's not a good justification for the change, since memcmp never reads\n> past the differing characters.\n\nInteresting. Where does that guarantee come from?\n\nASan reports an overflow with the following test program for me on\nDebian testing x64:\n\n#include \n\nint main(int argc, char **argv)\n{\n char a[32] = \"1234567890123456789012345678901\";\n char b[2] = \"a\";\n return memcmp(a, b, 32);\n}\n"},{"id":"323994","messageId":"8bd7c4c6-c545-723b-ec4b-262abd8a7f5f@web.de","threadId":"46328","inReplyTo":"d83d2395-e667-9e52-cc94-af5fbec6054c@ramsayjones.plus.com","subject":"Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-08T11:52:11Z","receivedAt":"2017-07-08T11:52:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.07.2017 um 13:08 schrieb Ramsay Jones:\n> On 08/07/17 09:58, René Scharfe wrote:\n>> Avoid running over the end of another -- a C string whose length we\n>> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it\n>> with another C string.\n> \n> I had to read this twice, along with the patch text, before this\n> made any sense. ;-) The missing information being that 'another'\n> was the name of the string variable that we were potentially\n> 'running over the end of'.\n\nYeah, sorry, encasing that unusual variable name in quotes would\nprobably have helped.\n\nRené\n"},{"id":"323996","messageId":"87o9svgia8.fsf@linux-m68k.org","threadId":"46328","inReplyTo":"e46a7de9-3e0e-4eeb-c9a3-a8b5f22620b4@web.de","subject":"Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2017-07-08T13:38:55Z","receivedAt":"2017-07-08T13:39:04Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Jul 08 2017, René Scharfe wrote:\n\n> Am 08.07.2017 um 13:08 schrieb Andreas Schwab:\n>> On Jul 08 2017, René Scharfe wrote:\n>>\n>>> Avoid running over the end of another -- a C string whose length we\n>>> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it\n>>> with another C string.\n>>\n>> That's not a good justification for the change, since memcmp never reads\n>> past the differing characters.\n>\n> Interesting. Where does that guarantee come from?\n\nSorry, I misremembered. It's only memchr that has this restriction.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"324007","messageId":"xmqqo9sua7gz.fsf@gitster.mtv.corp.google.com","threadId":"46328","inReplyTo":"8bd7c4c6-c545-723b-ec4b-262abd8a7f5f@web.de","subject":"Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-08T22:29:00Z","receivedAt":"2017-07-08T22:29:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 08.07.2017 um 13:08 schrieb Ramsay Jones:\n>> On 08/07/17 09:58, René Scharfe wrote:\n>>> Avoid running over the end of another -- a C string whose length we\n>>> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it\n>>> with another C string.\n>>\n>> I had to read this twice, along with the patch text, before this\n>> made any sense. ;-) The missing information being that 'another'\n>> was the name of the string variable that we were potentially\n>> 'running over the end of'.\n>\n> Yeah, sorry, encasing that unusual variable name in quotes would\n> probably have helped.\n\nWhat makes it even more confusing is that the variable with the\nproblematic name is referred to as \"it\" in the last part of the\ndescription--- the second occurrence of 'another' is actually not\nreferring to that variable but yet another string that is being\ncompared with it ;-)\n\n"},{"id":"324023","messageId":"df3bd047-6ddd-c120-54dd-4165d2257bb0@web.de","threadId":"46328","inReplyTo":"87o9svgia8.fsf@linux-m68k.org","subject":"Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-09T12:20:32Z","receivedAt":"2017-07-09T12:20:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.07.2017 um 15:38 schrieb Andreas Schwab:\n> On Jul 08 2017, René Scharfe wrote:\n> \n>> Am 08.07.2017 um 13:08 schrieb Andreas Schwab:\n>>> On Jul 08 2017, René Scharfe wrote:\n>>>\n>>>> Avoid running over the end of another -- a C string whose length we\n>>>> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it\n>>>> with another C string.\n>>>\n>>> That's not a good justification for the change, since memcmp never reads\n>>> past the differing characters.\n>>\n>> Interesting. Where does that guarantee come from?\n> \n> Sorry, I misremembered. It's only memchr that has this restriction.\n\nHmm, I can't get ASan to complain about memchr reading beyond the end of\na C string, but I don't know why. Glibc reads full words [1], and I\ndon't see how the standard [2] forbids reading past the found byte.\n\nRené\n\n\n[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=string/memchr.c\n[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/memchr.html\n"},{"id":"324024","messageId":"147f287a-e66e-11b2-c76f-b7d54e076a6b@web.de","threadId":"46328","inReplyTo":"xmqqo9sua7gz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-09T12:20:27Z","receivedAt":"2017-07-09T12:20:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.07.2017 um 00:29 schrieb Junio C Hamano:\n> René Scharfe writes:\n> \n>> Am 08.07.2017 um 13:08 schrieb Ramsay Jones:\n>>> On 08/07/17 09:58, René Scharfe wrote:\n>>>> Avoid running over the end of another -- a C string whose length we\n>>>> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it\n>>>> with another C string.\n>>>\n>>> I had to read this twice, along with the patch text, before this\n>>> made any sense. ;-) The missing information being that 'another'\n>>> was the name of the string variable that we were potentially\n>>> 'running over the end of'.\n>>\n>> Yeah, sorry, encasing that unusual variable name in quotes would\n>> probably have helped.\n> \n> What makes it even more confusing is that the variable with the\n> problematic name is referred to as \"it\" in the last part of the\n> description--- the second occurrence of 'another' is actually not\n> referring to that variable but yet another string that is being\n> compared with it ;-)\n\nPerhaps like this instead?\n\nWe don't know the length of the C string \"another\". It could be\nshorter than \"name\", which we compare it to using memchr(3). Call\nstrcmp(3) instead to avoid running over the end of the former, and\nget rid of a strlen(3) call as a bonus.\n\nRené\n"},{"id":"324025","messageId":"87lgnxpyzu.fsf@linux-m68k.org","threadId":"46328","inReplyTo":"df3bd047-6ddd-c120-54dd-4165d2257bb0@web.de","subject":"Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2017-07-09T12:37:41Z","receivedAt":"2017-07-09T12:38:01Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Jul 09 2017, René Scharfe wrote:\n\n> [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/memchr.html\n\nYou are using an old revision.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"324026","messageId":"52d0ac32-7b3d-990d-8ad9-42ef9ee8d8cf@web.de","threadId":"46328","inReplyTo":"87lgnxpyzu.fsf@linux-m68k.org","subject":"Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-09T13:19:53Z","receivedAt":"2017-07-09T13:20:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.07.2017 um 14:37 schrieb Andreas Schwab:\n> On Jul 09 2017, René Scharfe wrote:\n> \n>> [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/memchr.html\n> \n> You are using an old revision.\n\nOK, the final draft of C11 [3] says \"The implementation shall behave as\nif it reads the characters sequentially and stops as soon as a matching\ncharacter is found.\"\n\nI wonder when we can begin to target C99 in git's source, though. :)\n\nRené\n\n\n[3] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf\n"},{"id":"324036","messageId":"xmqqpod98rrm.fsf@gitster.mtv.corp.google.com","threadId":"46328","inReplyTo":"52d0ac32-7b3d-990d-8ad9-42ef9ee8d8cf@web.de","subject":"Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-09T17:05:49Z","receivedAt":"2017-07-09T17:05:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> I wonder when we can begin to target C99 in git's source, though. :)\n\nLet's get the ball rolling by starting to use some of the useful\nfeatures like designated initializers, perhaps, in a small, critical\nand reasonably stable part of the system that anybody must compile,\nleave it in one full release cycle or two, and when we hear nobody\ncomplains, introduce it en masse for the remainder of the system?\n\nThat way, we will see if there are people who need pre-C99 soon\nenough, and we won't have to scramble reverting too many changes\nwhen it happens.\n\n"}]}