Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback
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];
}
/*
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback
Jeff King p...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback
On Tue, Jan 15, 2013 at 08:55:32AM -0800, Junio C Hamano wrote: Jeff King p...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback
Am 15.01.2013 21:27, schrieb Andreas Schwab:
René Scharfe rene.scha...@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.scha...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH] ignore memcmp() overreading in bsearch() callback
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 gits...@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
+}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback
Johannes Schindelin johannes.schinde...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
