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

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

[Junio C Hamano]

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

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
+}

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

[Johannes Schindelin]

Hi Junio,

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.

Ciao,
Johannes

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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

[Jeff King]

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.

> 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

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

[Junio C Hamano]

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

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

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

[Jeff King]

On Tue, Jan 15, 2013 at 08:55:32AM -0800, Junio C Hamano wrote:

> 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

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

[René Scharfe]

Am 15.01.2013 00:36, schrieb Junio C Hamano:
> 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(-)

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];
 }
 
 /*

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

[Andreas Schwab]

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."

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

[René Scharfe]

Am 15.01.2013 21:27, schrieb Andreas Schwab:
> 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(-)

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