# [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() 12 messages from 2017-07-08 to 2017-07-09. Participants: René Scharfe, Andreas Schwab, Ramsay Jones, Junio C Hamano. Thread: https://gitlist.dev/t/46328 ## René Scharfe, 2017-07-08 08:58 Subject: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: URL: https://gitlist.dev/e/d1bb978b-a7df-48a8-15c8-80730c77e11c%40web.de ``` Avoid running over the end of another -- a C string whose length we don't know -- by using strcmp(3) instead of memcmp(3) for comparing it with another C string. Signed-off-by: Rene Scharfe --- apply.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apply.c b/apply.c index 946be4d2f5..9b3df8a3aa 100644 --- a/apply.c +++ b/apply.c @@ -962,13 +962,12 @@ static int gitdiff_verify_name(struct apply_state *state, } if (*name) { - int len = strlen(*name); char *another; if (isnull) return error(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"), *name, state->linenr); another = find_name(state, line, NULL, state->p_value, TERM_TAB); - if (!another || memcmp(another, *name, len + 1)) { + if (!another || strcmp(another, *name)) { free(another); return error((side == DIFF_NEW_NAME) ? _("git apply: bad git-diff - inconsistent new filename on line %d") : -- 2.13.2 ``` ## Andreas Schwab, 2017-07-08 11:08 Subject: Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: <87tw2ngp94.fsf@linux-m68k.org> URL: https://gitlist.dev/e/87tw2ngp94.fsf%40linux-m68k.org In-Reply-To: ``` On Jul 08 2017, René Scharfe wrote: > Avoid running over the end of another -- a C string whose length we > don't know -- by using strcmp(3) instead of memcmp(3) for comparing it > with another C string. That's not a good justification for the change, since memcmp never reads past the differing characters. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ``` ## Ramsay Jones, 2017-07-08 11:08 Subject: Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: URL: https://gitlist.dev/e/d83d2395-e667-9e52-cc94-af5fbec6054c%40ramsayjones.plus.com In-Reply-To: ``` On 08/07/17 09:58, René Scharfe wrote: > Avoid running over the end of another -- a C string whose length we > don't know -- by using strcmp(3) instead of memcmp(3) for comparing it > with another C string. I had to read this twice, along with the patch text, before this made any sense. ;-) The missing information being that 'another' was the name of the string variable that we were potentially 'running over the end of'. ATB, Ramsay Jones > > Signed-off-by: Rene Scharfe > --- > apply.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/apply.c b/apply.c > index 946be4d2f5..9b3df8a3aa 100644 > --- a/apply.c > +++ b/apply.c > @@ -962,13 +962,12 @@ static int gitdiff_verify_name(struct apply_state *state, > } > > if (*name) { > - int len = strlen(*name); > char *another; > if (isnull) > return error(_("git apply: bad git-diff - expected /dev/null, got %s on line %d"), > *name, state->linenr); > another = find_name(state, line, NULL, state->p_value, TERM_TAB); > - if (!another || memcmp(another, *name, len + 1)) { > + if (!another || strcmp(another, *name)) { > free(another); > return error((side == DIFF_NEW_NAME) ? > _("git apply: bad git-diff - inconsistent new filename on line %d") : > ``` ## René Scharfe, 2017-07-08 11:43 Subject: Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: URL: https://gitlist.dev/e/e46a7de9-3e0e-4eeb-c9a3-a8b5f22620b4%40web.de In-Reply-To: <87tw2ngp94.fsf@linux-m68k.org> ``` Am 08.07.2017 um 13:08 schrieb Andreas Schwab: > On Jul 08 2017, René Scharfe wrote: > >> Avoid running over the end of another -- a C string whose length we >> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it >> with another C string. > > That's not a good justification for the change, since memcmp never reads > past the differing characters. Interesting. Where does that guarantee come from? ASan reports an overflow with the following test program for me on Debian testing x64: #include int main(int argc, char **argv) { char a[32] = "1234567890123456789012345678901"; char b[2] = "a"; return memcmp(a, b, 32); } ``` ## René Scharfe, 2017-07-08 11:52 Subject: Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: <8bd7c4c6-c545-723b-ec4b-262abd8a7f5f@web.de> URL: https://gitlist.dev/e/8bd7c4c6-c545-723b-ec4b-262abd8a7f5f%40web.de In-Reply-To: ``` Am 08.07.2017 um 13:08 schrieb Ramsay Jones: > On 08/07/17 09:58, René Scharfe wrote: >> Avoid running over the end of another -- a C string whose length we >> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it >> with another C string. > > I had to read this twice, along with the patch text, before this > made any sense. ;-) The missing information being that 'another' > was the name of the string variable that we were potentially > 'running over the end of'. Yeah, sorry, encasing that unusual variable name in quotes would probably have helped. René ``` ## Andreas Schwab, 2017-07-08 13:38 Subject: Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: <87o9svgia8.fsf@linux-m68k.org> URL: https://gitlist.dev/e/87o9svgia8.fsf%40linux-m68k.org In-Reply-To: ``` On Jul 08 2017, René Scharfe wrote: > Am 08.07.2017 um 13:08 schrieb Andreas Schwab: >> On Jul 08 2017, René Scharfe wrote: >> >>> Avoid running over the end of another -- a C string whose length we >>> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it >>> with another C string. >> >> That's not a good justification for the change, since memcmp never reads >> past the differing characters. > > Interesting. Where does that guarantee come from? Sorry, I misremembered. It's only memchr that has this restriction. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ``` ## Junio C Hamano, 2017-07-08 22:29 Subject: Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: URL: https://gitlist.dev/e/xmqqo9sua7gz.fsf%40gitster.mtv.corp.google.com In-Reply-To: <8bd7c4c6-c545-723b-ec4b-262abd8a7f5f@web.de> ``` René Scharfe writes: > Am 08.07.2017 um 13:08 schrieb Ramsay Jones: >> On 08/07/17 09:58, René Scharfe wrote: >>> Avoid running over the end of another -- a C string whose length we >>> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it >>> with another C string. >> >> I had to read this twice, along with the patch text, before this >> made any sense. ;-) The missing information being that 'another' >> was the name of the string variable that we were potentially >> 'running over the end of'. > > Yeah, sorry, encasing that unusual variable name in quotes would > probably have helped. What makes it even more confusing is that the variable with the problematic name is referred to as "it" in the last part of the description--- the second occurrence of 'another' is actually not referring to that variable but yet another string that is being compared with it ;-) ``` ## René Scharfe, 2017-07-09 12:20 Subject: Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: URL: https://gitlist.dev/e/df3bd047-6ddd-c120-54dd-4165d2257bb0%40web.de In-Reply-To: <87o9svgia8.fsf@linux-m68k.org> ``` Am 08.07.2017 um 15:38 schrieb Andreas Schwab: > On Jul 08 2017, René Scharfe wrote: > >> Am 08.07.2017 um 13:08 schrieb Andreas Schwab: >>> On Jul 08 2017, René Scharfe wrote: >>> >>>> Avoid running over the end of another -- a C string whose length we >>>> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it >>>> with another C string. >>> >>> That's not a good justification for the change, since memcmp never reads >>> past the differing characters. >> >> Interesting. Where does that guarantee come from? > > Sorry, I misremembered. It's only memchr that has this restriction. Hmm, I can't get ASan to complain about memchr reading beyond the end of a C string, but I don't know why. Glibc reads full words [1], and I don't see how the standard [2] forbids reading past the found byte. René [1] https://sourceware.org/git/?p=glibc.git;a=blob;f=string/memchr.c [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/memchr.html ``` ## René Scharfe, 2017-07-09 12:20 Subject: Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: <147f287a-e66e-11b2-c76f-b7d54e076a6b@web.de> URL: https://gitlist.dev/e/147f287a-e66e-11b2-c76f-b7d54e076a6b%40web.de In-Reply-To: ``` Am 09.07.2017 um 00:29 schrieb Junio C Hamano: > René Scharfe writes: > >> Am 08.07.2017 um 13:08 schrieb Ramsay Jones: >>> On 08/07/17 09:58, René Scharfe wrote: >>>> Avoid running over the end of another -- a C string whose length we >>>> don't know -- by using strcmp(3) instead of memcmp(3) for comparing it >>>> with another C string. >>> >>> I had to read this twice, along with the patch text, before this >>> made any sense. ;-) The missing information being that 'another' >>> was the name of the string variable that we were potentially >>> 'running over the end of'. >> >> Yeah, sorry, encasing that unusual variable name in quotes would >> probably have helped. > > What makes it even more confusing is that the variable with the > problematic name is referred to as "it" in the last part of the > description--- the second occurrence of 'another' is actually not > referring to that variable but yet another string that is being > compared with it ;-) Perhaps like this instead? We don't know the length of the C string "another". It could be shorter than "name", which we compare it to using memchr(3). Call strcmp(3) instead to avoid running over the end of the former, and get rid of a strlen(3) call as a bonus. René ``` ## Andreas Schwab, 2017-07-09 12:37 Subject: Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: <87lgnxpyzu.fsf@linux-m68k.org> URL: https://gitlist.dev/e/87lgnxpyzu.fsf%40linux-m68k.org In-Reply-To: ``` On Jul 09 2017, René Scharfe wrote: > [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/memchr.html You are using an old revision. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ``` ## René Scharfe, 2017-07-09 13:19 Subject: Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: <52d0ac32-7b3d-990d-8ad9-42ef9ee8d8cf@web.de> URL: https://gitlist.dev/e/52d0ac32-7b3d-990d-8ad9-42ef9ee8d8cf%40web.de In-Reply-To: <87lgnxpyzu.fsf@linux-m68k.org> ``` Am 09.07.2017 um 14:37 schrieb Andreas Schwab: > On Jul 09 2017, René Scharfe wrote: > >> [2] http://pubs.opengroup.org/onlinepubs/009695399/functions/memchr.html > > You are using an old revision. OK, the final draft of C11 [3] says "The implementation shall behave as if it reads the characters sequentially and stops as soon as a matching character is found." I wonder when we can begin to target C99 in git's source, though. :) René [3] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf ``` ## Junio C Hamano, 2017-07-09 17:05 Subject: Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name() Message-ID: URL: https://gitlist.dev/e/xmqqpod98rrm.fsf%40gitster.mtv.corp.google.com In-Reply-To: <52d0ac32-7b3d-990d-8ad9-42ef9ee8d8cf@web.de> ``` René Scharfe writes: > I wonder when we can begin to target C99 in git's source, though. :) Let's get the ball rolling by starting to use some of the useful features like designated initializers, perhaps, in a small, critical and reasonably stable part of the system that anybody must compile, leave it in one full release cycle or two, and when we hear nobody complains, introduce it en masse for the remainder of the system? That way, we will see if there are people who need pre-C99 soon enough, and we won't have to scramble reverting too many changes when it happens. ```