threads / rfc / 32633

RFC patchignore memcmp() overreading in bsearch() callback

Subject: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback

## tl;dr

9 messages between Jan 14, 2013 and Jan 16, 2013. Diffs are folded; open one to read it.

replies: 8people: 5as markdown or json

Junio C Hamano· Jan 14, 2013, 23:36 UTC · lore

It appears that memcmp() uses the usual "one word at a time" comparison and triggers valgrind in a callback of bsearch() used in the refname search. I can easily trigger problems in any script with test_commit (e.g. "sh t0101-at-syntax.sh --valgrind -i -v") without this suppression.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/valgrind/default.supp | 8 ++++++++
 1 file changed, 8 insertions(+)
Show changes to t/valgrind/default.supp +8 −0
diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
index 0a6724f..032332f 100644
--- a/t/valgrind/default.supp
+++ b/t/valgrind/default.supp
@@ -49,3 +49,11 @@
 	Memcheck:Addr4
 	fun:copy_ref
 }
+
+{
+	ignore-memcmp-reading-too-much-in-bsearch-callback
+	Memcheck:Addr4
+	fun:ref_entry_cmp_sslice
+	fun:bsearch
+	fun:search_ref_dir
+}
Johannes Schindelin· Jan 14, 2013, 23:56 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback

Hi Junio,
On Mon, 14 Jan 2013, Junio C Hamano wrote:
Show 5 quoted lines
> It appears that memcmp() uses the usual "one word at a time"
> comparison and triggers valgrind in a callback of bsearch() used in
> the refname search.  I can easily trigger problems in any script
> with test_commit (e.g. "sh t0101-at-syntax.sh --valgrind -i -v")
> without this suppression.

I have no way to replicate that issue, but I take your word for it. With that in mind, here is my ACK.

Ciao, Johannes

Junio C Hamano· Jan 15, 2013, 02:45 UTC · re: Johannes Schindelin · lore

Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
> On Mon, 14 Jan 2013, Junio C Hamano wrote:
>
>> It appears that memcmp() uses the usual "one word at a time"
>> comparison and triggers valgrind in a callback of bsearch() used in
>> the refname search.  I can easily trigger problems in any script
>> with test_commit (e.g. "sh t0101-at-syntax.sh --valgrind -i -v")
>> without this suppression.
>
> I have no way to replicate that issue, but I take your word for it. With
> that in mind, here is my ACK.
Thanks.
Jeff King· Jan 15, 2013, 15:50 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback

On Mon, Jan 14, 2013 at 03:36:21PM -0800, Junio C Hamano wrote:
Show 5 quoted lines
> It appears that memcmp() uses the usual "one word at a time"
> comparison and triggers valgrind in a callback of bsearch() used in
> the refname search.  I can easily trigger problems in any script
> with test_commit (e.g. "sh t0101-at-syntax.sh --valgrind -i -v")
> without this suppression.

Out of curiosity, what platform do you see this on? I can't reproduce on glibc.

Show 16 quoted lines
> diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
> index 0a6724f..032332f 100644
> --- a/t/valgrind/default.supp
> +++ b/t/valgrind/default.supp
> @@ -49,3 +49,11 @@
>  	Memcheck:Addr4
>  	fun:copy_ref
>  }
> +
> +{
> +	ignore-memcmp-reading-too-much-in-bsearch-callback
> +	Memcheck:Addr4
> +	fun:ref_entry_cmp_sslice
> +	fun:bsearch
> +	fun:search_ref_dir
> +}

Given that it is valgrind-clean on my platform, and reading the code I don't see any problems, I think it probably is a false positive, and this suppression makes sense.

-Peff
Junio C Hamano· Jan 15, 2013, 16:55 UTC · re: Jeff King · lore

Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> On Mon, Jan 14, 2013 at 03:36:21PM -0800, Junio C Hamano wrote:
>
>> It appears that memcmp() uses the usual "one word at a time"
>> comparison and triggers valgrind in a callback of bsearch() used in
>> the refname search.  I can easily trigger problems in any script
>> with test_commit (e.g. "sh t0101-at-syntax.sh --valgrind -i -v")
>> without this suppression.
>
> Out of curiosity, what platform do you see this on? I can't reproduce on
> glibc.
    Debian GNU/Linux 6.0.6 (squeeze), on Linux 2.6.32-5-amd64.
    libc-bin              2.11.3-4
    valgrind-3.6.0.SVN-Debian
    gcc                   4:4.4.5-1
Show 20 quoted lines
>> diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp
>> index 0a6724f..032332f 100644
>> --- a/t/valgrind/default.supp
>> +++ b/t/valgrind/default.supp
>> @@ -49,3 +49,11 @@
>>  	Memcheck:Addr4
>>  	fun:copy_ref
>>  }
>> +
>> +{
>> +	ignore-memcmp-reading-too-much-in-bsearch-callback
>> +	Memcheck:Addr4
>> +	fun:ref_entry_cmp_sslice
>> +	fun:bsearch
>> +	fun:search_ref_dir
>> +}
>
> Given that it is valgrind-clean on my platform, and reading the code I
> don't see any problems, I think it probably is a false positive, and
> this suppression makes sense.
Thanks for a sanity check.
Jeff King· Jan 15, 2013, 17:18 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback

On Tue, Jan 15, 2013 at 08:55:32AM -0800, Junio C Hamano wrote:
Show 17 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > On Mon, Jan 14, 2013 at 03:36:21PM -0800, Junio C Hamano wrote:
> >
> >> It appears that memcmp() uses the usual "one word at a time"
> >> comparison and triggers valgrind in a callback of bsearch() used in
> >> the refname search.  I can easily trigger problems in any script
> >> with test_commit (e.g. "sh t0101-at-syntax.sh --valgrind -i -v")
> >> without this suppression.
> >
> > Out of curiosity, what platform do you see this on? I can't reproduce on
> > glibc.
> 
>     Debian GNU/Linux 6.0.6 (squeeze), on Linux 2.6.32-5-amd64.
>     libc-bin              2.11.3-4
>     valgrind-3.6.0.SVN-Debian
>     gcc                   4:4.4.5-1

Interesting. I can reproduce easily on my squeeze machine, but not my wheezy. So presumably it is a false positive fixed either in libc (I have 2.13-38 on the "good" box) or valgrind (1:3.8.1-1).

However, the error that valgrind reports is on the call to "strlen(ent->name)", not memcmp (but it has suffered from the same SSE issues in the past).

So I feel pretty confident that it really is a false positive; you may want to double-check the offending call for the commit message (though I would not be surprised if it is triggerable from both). I think it also means that René's suggestion to use strncmp cannot be relied on to silence the warning (though I am not opposed to doing it anyway if we think it is more clear).

-Peff
René Scharfe· Jan 15, 2013, 15:55 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback

Am 15.01.2013 00:36, schrieb Junio C Hamano:
Show 5 quoted lines
> It appears that memcmp() uses the usual "one word at a time"
> comparison and triggers valgrind in a callback of bsearch() used in
> the refname search.  I can easily trigger problems in any script
> with test_commit (e.g. "sh t0101-at-syntax.sh --valgrind -i -v")
> without this suppression.

I can't reproduce it on Debian, but can we perhaps do without the suppression with a patch like this instead? I would expect it to be slightly faster because we lose the strlen() call, but didn't check. It's also simpler, perhaps with the exception of the last line. Does it help in your case?

René
---
 refs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
Show changes to refs.c +2 −4
diff --git a/refs.c b/refs.c
index 541fec2..1a0e049 100644
--- a/refs.c
+++ b/refs.c
@@ -335,12 +335,10 @@ static int ref_entry_cmp_sslice(const void *key_, const void *ent_)
 {
 	struct string_slice *key = (struct string_slice *)key_;
 	struct ref_entry *ent = *(struct ref_entry **)ent_;
-	int entlen = strlen(ent->name);
-	int cmplen = key->len < entlen ? key->len : entlen;
-	int cmp = memcmp(key->str, ent->name, cmplen);
+	int cmp = strncmp(key->str, ent->name, key->len);
 	if (cmp)
 		return cmp;
-	return key->len - entlen;
+	return '\0' - ent->name[key->len];
 }
 
 /*
Andreas Schwab· Jan 15, 2013, 20:27 UTC · re: René Scharfe · lore

Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> +	return '\0' - ent->name[key->len];

You need to cast to unsigned char first to make it consistent with memcmp and strcmp.

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· Jan 16, 2013, 01:08 UTC · re: Andreas Schwab · lore

Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback

Am 15.01.2013 21:27, schrieb Andreas Schwab:
Show 6 quoted lines
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
>> +	return '\0' - ent->name[key->len];
> 
> You need to cast to unsigned char first to make it consistent with
> memcmp and strcmp.
Thanks for catching this!
-- >8 --
Subject: [PATCH] refs: use strncmp() instead of strlen() and memcmp()

Simplify ref_entry_cmp_sslice() by using strncmp() to compare the length-limited key and a NUL-terminated entry. While we're at it, retain the const attribute of the input pointers.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 refs.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
Show changes to refs.c +4 −6
diff --git a/refs.c b/refs.c
index 541fec2..5129da0 100644
--- a/refs.c
+++ b/refs.c
@@ -333,14 +333,12 @@ struct string_slice {
 
 static int ref_entry_cmp_sslice(const void *key_, const void *ent_)
 {
-	struct string_slice *key = (struct string_slice *)key_;
-	struct ref_entry *ent = *(struct ref_entry **)ent_;
-	int entlen = strlen(ent->name);
-	int cmplen = key->len < entlen ? key->len : entlen;
-	int cmp = memcmp(key->str, ent->name, cmplen);
+	const struct string_slice *key = key_;
+	const struct ref_entry *ent = *(const struct ref_entry * const *)ent_;
+	int cmp = strncmp(key->str, ent->name, key->len);
 	if (cmp)
 		return cmp;
-	return key->len - entlen;
+	return '\0' - (unsigned char)ent->name[key->len];
 }
 
 /*

-- 
1.8.0

← back to recent threads