On a system that supports the vector enhancements facility 2 (arch13 or higher), glibc runs an optimized version of memmem that uses the "vector string search" instruction. That implementation contains a conditional jump that may depend on undefined values; however, the end result of the function is still independent from these undefined values. Since memcheck can't see that, it may report a false positive, as reproduced like this: $ valgrind perl -e "use lib '.'; require BarXXXXXXXXXXXXXXX;" ... ==3271940== Conditional jump or move depends on uninitialised value(s) ==3271940== at 0x4CAABB6: __memmem_arch13 (memmem-arch13.S:65) ==3271940== by 0x49B64FF: Perl_pp_require (in /usr/lib64/libperl.so.5.32.1) ==3271940== by 0x49654C1: Perl_runops_standard (in /usr/lib64/libperl.so.5.32.1) ==3271940== by 0x48DC3F5: perl_run (in /usr/lib64/libperl.so.5.32.1) ==3271940== by 0x108E37: main (in /usr/bin/perl)
Created attachment 148994 [details] Add intercept for memmem on s390x Add an intercept for memmem on s390x platforms. This fixes the problem in my testing.
Looks good and fixes the issue for me. + /* If the needle is the empty string, match immediately. */ \ + if (nlen == 0) return CONST_CAST(void *,h); \ + \ + HChar n0 = n[0]; \ + \ + for (; hlen >= nlen; hlen--, h++) { \ + const HChar hh = *h; \ + if (hh != n0) continue; \ + \ + UWord i; \ + for (i = 0; i < nlen; i++) { \ + if (n[i] != h[i]) \ + break; \ + } \ + if (i == nlen) \ + return CONST_CAST(HChar *,h); \ I think the for loop could/should be from i = 1. needle being zero sized (nlen == 0) and n[0] == h[0] (hh != n0) has already been checked above.
Created attachment 149008 [details] Add memcheck memmem vgtest I wrote a testcase that fails before the patch (on s390x) and succeeds afterwards. It would be nice to add it so we check the same isn't an issue on other arches and to test our own memmem intercept.
(In reply to Mark Wielaard from comment #2) > Looks good and fixes the issue for me. > I think the for loop could/should be from i = 1. needle being zero sized > (nlen == 0) and n[0] == h[0] (hh != n0) has already been checked above. Thanks, you're right. Also, the hh variable is only accessed once and doesn't improve readability. I'll attach an updated patch. (In reply to Mark Wielaard from comment #3) > Created attachment 149008 [details] > Add memcheck memmem vgtest > > I wrote a testcase that fails before the patch (on s390x) and succeeds > afterwards. It would be nice to add it so we check the same isn't an issue > on other arches and to test our own memmem intercept. Great! Do we also want to check cases where memcheck is expected to complain about the use of uninitialized values in memmem?
Created attachment 149025 [details] Patch for adding a memmem intercept, version 2 This addresses Mark's comment and removes the 'hh' variable.
Created attachment 149209 [details] memcheck memmem tests > > I wrote a testcase that fails before the patch (on s390x) and succeeds > > afterwards. It would be nice to add it so we check the same isn't an issue > > on other arches and to test our own memmem intercept. > Great! Do we also want to check cases where memcheck is expected to complain > about the use of uninitialized values in memmem? Good idea. But that turned out trickier than expected. Since there are multiple different memmem implementations some of which call overridden memchr and now we have an actual memmem override for s390x matching on the backtrace is hard. So the two uninit needle and uninit haystack testcases just filter on the main file line numbers.
(In reply to Andreas Arnez from comment #5) > Created attachment 149025 [details] > Patch for adding a memmem intercept, version 2 > > This addresses Mark's comment and removes the 'hh' variable. Looks good to me and works with the new testcase I just attached.
I believe this is good to go. I have been testing it, including the new testcase on various fedora systems. Andreas, could you push the fix. Let me know if you want me to add the testcase separately.
(In reply to Mark Wielaard from comment #8) > Andreas, could you push the fix. Let me know if you want me to add the > testcase separately. Thanks, I pushed the fix now, including your testcase.