threads / patch / 46328

patchapply: use strcmp(3) for comparing strings in gitdiff_verify_name()

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

## tl;dr

12 messages between Jul 8, 2017 and Jul 9, 2017. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

René Scharfe· Jul 8, 2017, 08:58 UTC · lore

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(-)
Show changes to apply.c +1 −2
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· Jul 8, 2017, 11:08 UTC · re: René Scharfe · lore

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

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."
René Scharfe· Jul 8, 2017, 11:43 UTC · re: Andreas Schwab · lore

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

Am 08.07.2017 um 13:08 schrieb Andreas Schwab:
Show 8 quoted lines
> 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);
}
Andreas Schwab· Jul 8, 2017, 13:38 UTC · re: René Scharfe · lore

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

On Jul 08 2017, René Scharfe <l.s.r@web.de> wrote:
Show 11 quoted lines
> 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."
René Scharfe· Jul 9, 2017, 12:20 UTC · re: Andreas Schwab · lore

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

Am 08.07.2017 um 15:38 schrieb Andreas Schwab:
Show 15 quoted lines
> 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

Andreas Schwab· Jul 9, 2017, 12:37 UTC · re: René Scharfe · lore

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

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."
René Scharfe· Jul 9, 2017, 13:19 UTC · re: Andreas Schwab · lore

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

Am 09.07.2017 um 14:37 schrieb Andreas Schwab:
Show 5 quoted lines
> 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
Junio C Hamano· Jul 9, 2017, 17:05 UTC · re: René Scharfe · lore

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

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.

Ramsay Jones· Jul 8, 2017, 11:08 UTC · re: René Scharfe · lore

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

Show 26 quoted lines
> 
> 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") :
>
René Scharfe· Jul 8, 2017, 11:52 UTC · re: Ramsay Jones · lore

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

Am 08.07.2017 um 13:08 schrieb Ramsay Jones:
Show 9 quoted lines
> 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é
Junio C Hamano· Jul 8, 2017, 22:29 UTC · re: René Scharfe · lore

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

René Scharfe <l.s.r@web.de> writes:
Show 13 quoted lines
> 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· Jul 9, 2017, 12:20 UTC · re: Junio C Hamano · lore

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

Am 09.07.2017 um 00:29 schrieb Junio C Hamano:
Show 21 quoted lines
> 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é

← back to recent threads