{"thread":{"id":"32633","subject":"[RFC/PATCH] ignore memcmp() overreading in bsearch() callback","startedAt":"2013-01-14T23:36:21Z","lastAt":"2013-01-16T01:08:16Z","messageCount":9,"participants":["Junio C Hamano","Johannes Schindelin","Jeff King","René Scharfe","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"206851","messageId":"7v38y38hhm.fsf@alter.siamese.dyndns.org","threadId":"32633","inReplyTo":null,"subject":"[RFC/PATCH] ignore memcmp() overreading in bsearch() callback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-14T23:36:21Z","receivedAt":"2013-01-14T23:36:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It appears that memcmp() uses the usual \"one word at a time\"\ncomparison and triggers valgrind in a callback of bsearch() used in\nthe refname search. I can easily trigger problems in any script\nwith test_commit (e.g. \"sh t0101-at-syntax.sh --valgrind -i -v\")\nwithout this suppression.\n\nSigned-off-by: Junio C Hamano \n---\n t/valgrind/default.supp | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/valgrind/default.supp b/t/valgrind/default.supp\nindex 0a6724f..032332f 100644\n--- a/t/valgrind/default.supp\n+++ b/t/valgrind/default.supp\n@@ -49,3 +49,11 @@\n \tMemcheck:Addr4\n \tfun:copy_ref\n }\n+\n+{\n+\tignore-memcmp-reading-too-much-in-bsearch-callback\n+\tMemcheck:Addr4\n+\tfun:ref_entry_cmp_sslice\n+\tfun:bsearch\n+\tfun:search_ref_dir\n+}\n"},{"id":"206855","messageId":"alpine.DEB.1.00.1301150055520.32206@s15462909.onlinehome-server.info","threadId":"32633","inReplyTo":"7v38y38hhm.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2013-01-14T23:56:24Z","receivedAt":"2013-01-14T23:56:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 14 Jan 2013, Junio C Hamano wrote:\n\n> It appears that memcmp() uses the usual \"one word at a time\"\n> comparison and triggers valgrind in a callback of bsearch() used in\n> the refname search. I can easily trigger problems in any script\n> with test_commit (e.g. \"sh t0101-at-syntax.sh --valgrind -i -v\")\n> without this suppression.\n\nI have no way to replicate that issue, but I take your word for it. With\nthat in mind, here is my ACK.\n\nCiao,\nJohannes\n"},{"id":"206876","messageId":"7va9sb6u68.fsf@alter.siamese.dyndns.org","threadId":"32633","inReplyTo":"alpine.DEB.1.00.1301150055520.32206@s15462909.onlinehome-server.info","subject":"Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-15T02:45:19Z","receivedAt":"2013-01-15T02:45:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Mon, 14 Jan 2013, Junio C Hamano wrote:\n>\n>> It appears that memcmp() uses the usual \"one word at a time\"\n>> comparison and triggers valgrind in a callback of bsearch() used in\n>> the refname search. I can easily trigger problems in any script\n>> with test_commit (e.g. \"sh t0101-at-syntax.sh --valgrind -i -v\")\n>> without this suppression.\n>\n> I have no way to replicate that issue, but I take your word for it. With\n> that in mind, here is my ACK.\n\nThanks.\n"},{"id":"206939","messageId":"20130115155043.GA21815@sigill.intra.peff.net","threadId":"32633","inReplyTo":"7v38y38hhm.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-15T15:50:43Z","receivedAt":"2013-01-15T15:50:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 14, 2013 at 03:36:21PM -0800, Junio C Hamano wrote:\n\n> It appears that memcmp() uses the usual \"one word at a time\"\n> comparison and triggers valgrind in a callback of bsearch() used in\n> the refname search. I can easily trigger problems in any script\n> with test_commit (e.g. \"sh t0101-at-syntax.sh --valgrind -i -v\")\n> without this suppression.\n\nOut of curiosity, what platform do you see this on? I can't reproduce on\nglibc.\n\n> diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp\n> index 0a6724f..032332f 100644\n> --- a/t/valgrind/default.supp\n> +++ b/t/valgrind/default.supp\n> @@ -49,3 +49,11 @@\n> \tMemcheck:Addr4\n> \tfun:copy_ref\n> }\n> +\n> +{\n> +\tignore-memcmp-reading-too-much-in-bsearch-callback\n> +\tMemcheck:Addr4\n> +\tfun:ref_entry_cmp_sslice\n> +\tfun:bsearch\n> +\tfun:search_ref_dir\n> +}\n\nGiven that it is valgrind-clean on my platform, and reading the code I\ndon't see any problems, I think it probably is a false positive, and\nthis suppression makes sense.\n\n-Peff\n"},{"id":"206941","messageId":"50F57BDF.1050400@lsrfire.ath.cx","threadId":"32633","inReplyTo":"7v38y38hhm.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-01-15T15:55:11Z","receivedAt":"2013-01-15T15:55:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.01.2013 00:36, schrieb Junio C Hamano:\n> It appears that memcmp() uses the usual \"one word at a time\"\n> comparison and triggers valgrind in a callback of bsearch() used in\n> the refname search. I can easily trigger problems in any script\n> with test_commit (e.g. \"sh t0101-at-syntax.sh --valgrind -i -v\")\n> without this suppression.\n\nI can't reproduce it on Debian, but can we perhaps do without the\nsuppression with a patch like this instead? I would expect it to\nbe slightly faster because we lose the strlen() call, but didn't\ncheck. It's also simpler, perhaps with the exception of the last\nline. Does it help in your case?\n\nRené\n\n---\n refs.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 541fec2..1a0e049 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -335,12 +335,10 @@ static int ref_entry_cmp_sslice(const void *key_, const void *ent_)\n {\n \tstruct string_slice *key = (struct string_slice *)key_;\n \tstruct ref_entry *ent = *(struct ref_entry **)ent_;\n-\tint entlen = strlen(ent->name);\n-\tint cmplen = key->len < entlen ? key->len : entlen;\n-\tint cmp = memcmp(key->str, ent->name, cmplen);\n+\tint cmp = strncmp(key->str, ent->name, key->len);\n \tif (cmp)\n \t\treturn cmp;\n-\treturn key->len - entlen;\n+\treturn '\\0' - ent->name[key->len];\n }\n \n /*\n"},{"id":"206949","messageId":"7vmwwa4c8r.fsf@alter.siamese.dyndns.org","threadId":"32633","inReplyTo":"20130115155043.GA21815@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-15T16:55:32Z","receivedAt":"2013-01-15T16:55:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jan 14, 2013 at 03:36:21PM -0800, Junio C Hamano wrote:\n>\n>> It appears that memcmp() uses the usual \"one word at a time\"\n>> comparison and triggers valgrind in a callback of bsearch() used in\n>> the refname search. I can easily trigger problems in any script\n>> with test_commit (e.g. \"sh t0101-at-syntax.sh --valgrind -i -v\")\n>> without this suppression.\n>\n> Out of curiosity, what platform do you see this on? I can't reproduce on\n> glibc.\n\n Debian GNU/Linux 6.0.6 (squeeze), on Linux 2.6.32-5-amd64.\n libc-bin 2.11.3-4\n valgrind-3.6.0.SVN-Debian\n gcc 4:4.4.5-1\n\n>> diff --git a/t/valgrind/default.supp b/t/valgrind/default.supp\n>> index 0a6724f..032332f 100644\n>> --- a/t/valgrind/default.supp\n>> +++ b/t/valgrind/default.supp\n>> @@ -49,3 +49,11 @@\n>> \tMemcheck:Addr4\n>> \tfun:copy_ref\n>> }\n>> +\n>> +{\n>> +\tignore-memcmp-reading-too-much-in-bsearch-callback\n>> +\tMemcheck:Addr4\n>> +\tfun:ref_entry_cmp_sslice\n>> +\tfun:bsearch\n>> +\tfun:search_ref_dir\n>> +}\n>\n> Given that it is valgrind-clean on my platform, and reading the code I\n> don't see any problems, I think it probably is a false positive, and\n> this suppression makes sense.\n\nThanks for a sanity check.\n"},{"id":"206953","messageId":"20130115171844.GA32227@sigill.intra.peff.net","threadId":"32633","inReplyTo":"7vmwwa4c8r.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-15T17:18:44Z","receivedAt":"2013-01-15T17:18:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 15, 2013 at 08:55:32AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Mon, Jan 14, 2013 at 03:36:21PM -0800, Junio C Hamano wrote:\n> >\n> >> It appears that memcmp() uses the usual \"one word at a time\"\n> >> comparison and triggers valgrind in a callback of bsearch() used in\n> >> the refname search. I can easily trigger problems in any script\n> >> with test_commit (e.g. \"sh t0101-at-syntax.sh --valgrind -i -v\")\n> >> without this suppression.\n> >\n> > Out of curiosity, what platform do you see this on? I can't reproduce on\n> > glibc.\n> \n> Debian GNU/Linux 6.0.6 (squeeze), on Linux 2.6.32-5-amd64.\n> libc-bin 2.11.3-4\n> valgrind-3.6.0.SVN-Debian\n> gcc 4:4.4.5-1\n\nInteresting. I can reproduce easily on my squeeze machine, but not my\nwheezy. So presumably it is a false positive fixed either in libc (I\nhave 2.13-38 on the \"good\" box) or valgrind (1:3.8.1-1).\n\nHowever, the error that valgrind reports is on the call to\n\"strlen(ent->name)\", not memcmp (but it has suffered from the same SSE\nissues in the past).\n\nSo I feel pretty confident that it really is a false positive; you may\nwant to double-check the offending call for the commit message (though I\nwould not be surprised if it is triggerable from both). I think it also\nmeans that René's suggestion to use strncmp cannot be relied on to\nsilence the warning (though I am not opposed to doing it anyway if we\nthink it is more clear).\n\n-Peff\n"},{"id":"206971","messageId":"m2libunqdj.fsf@igel.home","threadId":"32633","inReplyTo":"50F57BDF.1050400@lsrfire.ath.cx","subject":"Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2013-01-15T20:27:36Z","receivedAt":"2013-01-15T20:27:36Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"René Scharfe writes:\n\n> +\treturn '\\0' - ent->name[key->len];\n\nYou need to cast to unsigned char first to make it consistent with\nmemcmp and strcmp.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"206999","messageId":"50F5FD80.1000808@lsrfire.ath.cx","threadId":"32633","inReplyTo":"m2libunqdj.fsf@igel.home","subject":"Re: [RFC/PATCH] ignore memcmp() overreading in bsearch() callback","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-01-16T01:08:16Z","receivedAt":"2013-01-16T01:08:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.01.2013 21:27, schrieb Andreas Schwab:\n> René Scharfe writes:\n> \n>> +\treturn '\\0' - ent->name[key->len];\n> \n> You need to cast to unsigned char first to make it consistent with\n> memcmp and strcmp.\n\nThanks for catching this!\n\n-- >8 --\nSubject: [PATCH] refs: use strncmp() instead of strlen() and memcmp()\n\nSimplify ref_entry_cmp_sslice() by using strncmp() to compare the\nlength-limited key and a NUL-terminated entry. While we're at it,\nretain the const attribute of the input pointers.\n\nSigned-off-by: Rene Scharfe \n---\n refs.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 541fec2..5129da0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -333,14 +333,12 @@ struct string_slice {\n \n static int ref_entry_cmp_sslice(const void *key_, const void *ent_)\n {\n-\tstruct string_slice *key = (struct string_slice *)key_;\n-\tstruct ref_entry *ent = *(struct ref_entry **)ent_;\n-\tint entlen = strlen(ent->name);\n-\tint cmplen = key->len < entlen ? key->len : entlen;\n-\tint cmp = memcmp(key->str, ent->name, cmplen);\n+\tconst struct string_slice *key = key_;\n+\tconst struct ref_entry *ent = *(const struct ref_entry * const *)ent_;\n+\tint cmp = strncmp(key->str, ent->name, key->len);\n \tif (cmp)\n \t\treturn cmp;\n-\treturn key->len - entlen;\n+\treturn '\\0' - (unsigned char)ent->name[key->len];\n }\n \n /*\n-- \n1.8.0\n"}]}