[PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()

[PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()

[René Scharfe]

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 <l.s.r@web.de>
---
 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

Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()

[Andreas Schwab]

On Jul 08 2017, René Scharfe <l.s.r@web.de> 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."

Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()

[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'.

ATB,
Ramsay Jones

> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  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()

[René Scharfe]

Am 08.07.2017 um 13:08 schrieb Andreas Schwab:
> On Jul 08 2017, René Scharfe <l.s.r@web.de> 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 <string.h>

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()

[René Scharfe]

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()

[Andreas Schwab]

On Jul 08 2017, René Scharfe <l.s.r@web.de> wrote:

> Am 08.07.2017 um 13:08 schrieb Andreas Schwab:
>> On Jul 08 2017, René Scharfe <l.s.r@web.de> 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."

Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> 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()

[René Scharfe]

Am 09.07.2017 um 00:29 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> 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()

[René Scharfe]

Am 08.07.2017 um 15:38 schrieb Andreas Schwab:
> On Jul 08 2017, René Scharfe <l.s.r@web.de> wrote:
> 
>> Am 08.07.2017 um 13:08 schrieb Andreas Schwab:
>>> On Jul 08 2017, René Scharfe <l.s.r@web.de> 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()

[Andreas Schwab]

On Jul 09 2017, René Scharfe <l.s.r@web.de> 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."

Re: [PATCH] apply: use strcmp(3) for comparing strings in gitdiff_verify_name()

[René Scharfe]

Am 09.07.2017 um 14:37 schrieb Andreas Schwab:
> On Jul 09 2017, René Scharfe <l.s.r@web.de> 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()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> 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.