# [RFC/PATCH] ignore memcmp() overreading in bsearch() callback 9 messages from 2013-01-14 to 2013-01-16. Participants: Junio C Hamano, Johannes Schindelin, Jeff King, René Scharfe, Andreas Schwab. Thread: https://gitlist.dev/t/32633 ## Junio C Hamano, 2013-01-14 23:36 Subject: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback Message-ID: <7v38y38hhm.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v38y38hhm.fsf%40alter.siamese.dyndns.org ``` 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 --- 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 +} ``` ## Johannes Schindelin, 2013-01-14 23:56 Subject: Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.1301150055520.32206%40s15462909.onlinehome-server.info In-Reply-To: <7v38y38hhm.fsf@alter.siamese.dyndns.org> ``` 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 ``` ## Junio C Hamano, 2013-01-15 02:45 Subject: Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback Message-ID: <7va9sb6u68.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7va9sb6u68.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin 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. ``` ## Jeff King, 2013-01-15 15:50 Subject: Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback Message-ID: <20130115155043.GA21815@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130115155043.GA21815%40sigill.intra.peff.net In-Reply-To: <7v38y38hhm.fsf@alter.siamese.dyndns.org> ``` 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 ``` ## René Scharfe, 2013-01-15 15:55 Subject: Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback Message-ID: <50F57BDF.1050400@lsrfire.ath.cx> URL: https://gitlist.dev/e/50F57BDF.1050400%40lsrfire.ath.cx In-Reply-To: <7v38y38hhm.fsf@alter.siamese.dyndns.org> ``` 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]; } /* ``` ## Junio C Hamano, 2013-01-15 16:55 Subject: Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback Message-ID: <7vmwwa4c8r.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmwwa4c8r.fsf%40alter.siamese.dyndns.org In-Reply-To: <20130115155043.GA21815@sigill.intra.peff.net> ``` Jeff King 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. ``` ## Jeff King, 2013-01-15 17:18 Subject: Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback Message-ID: <20130115171844.GA32227@sigill.intra.peff.net> URL: https://gitlist.dev/e/20130115171844.GA32227%40sigill.intra.peff.net In-Reply-To: <7vmwwa4c8r.fsf@alter.siamese.dyndns.org> ``` On Tue, Jan 15, 2013 at 08:55:32AM -0800, Junio C Hamano wrote: > Jeff King 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 ``` ## Andreas Schwab, 2013-01-15 20:27 Subject: Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback Message-ID: URL: https://gitlist.dev/e/m2libunqdj.fsf%40igel.home In-Reply-To: <50F57BDF.1050400@lsrfire.ath.cx> ``` René Scharfe 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, 2013-01-16 01:08 Subject: Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback Message-ID: <50F5FD80.1000808@lsrfire.ath.cx> URL: https://gitlist.dev/e/50F5FD80.1000808%40lsrfire.ath.cx In-Reply-To: ``` Am 15.01.2013 21:27, schrieb Andreas Schwab: > René Scharfe 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 --- 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 ```