Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()
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, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()
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") :
>
Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()
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);
}
Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()
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é
Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()
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, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()
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 ;-)
Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()
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
Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()
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é
Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()
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, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()
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
Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()
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.
