| Summary: | Add replacements for wmemchr and wcsnlen on Linux | ||
|---|---|---|---|
| Product: | [Developer tools] valgrind | Reporter: | faragon.github |
| Component: | memcheck | Assignee: | Ivo Raisr <ivosh> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | fweimer, ivosh, nix |
| Priority: | NOR | ||
| Version First Reported In: | 3.13.0 | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| Latest Commit: | Version Fixed In: | ||
| Sentry Crash Report: | |||
| Attachments: |
Replace wmemchr() and wcsnlen() on Linux.
0001-Add-replacements-for-wmemchr-and-wcsnlen-on-Linux.patch Add-replacements-for-wmemchr-and-wcsnlen-on-Linux |
||
Could it be related to this other bug (2009): (?) https://bugzilla.redhat.com/show_bug.cgi?id=518247 I bet this is just a matter of new IFUNC resolvers. memchr and rawmemchr are in the list; wcsnlen and wmemchr are not. FYI, with glibc 2.27 I see ==38614== Conditional jump or move depends on uninitialised value(s) ==38614== at 0x5190B37: __wmemchr_avx2 (in /usr/lib/libc-2.27.so) ==38614== by 0x5104D98: internal_fnwmatch (in /usr/lib/libc-2.27.so) ==38614== by 0x5107F4C: fnmatch@@GLIBC_2.2.5 (in /usr/lib/libc-2.27.so) ==38614== by 0x409518: print_dir (in /pkg/coreutils/8.29/usr/bin/ls) ==38614== by 0x402FCC: main (in /pkg/coreutils/8.29/usr/bin/ls) which is clearly related. I'll see if I can work up a patch. Created attachment 111086 [details]
Replace wmemchr() and wcsnlen() on Linux.
Patch attached. There may be more things which need replacement, but "valgrind /bin/ls" works for me on glibc 2.27 without spurious errors now.
Will send to Ivo to see what he thinks of it.
Thank you for the patch. I had to make some fixes to make it actually work and do not introduce regressions. Please review the attachment. Created attachment 111142 [details]
0001-Add-replacements-for-wmemchr-and-wcsnlen-on-Linux.patch
Fixed patch originally from Nick Alcock.
I'll admit I don't understand why wmemchr() and wcsnlen() need to take UInt *s when the other wide character functions in the same file are quite happy with Ints, but if you prefer it, I don't see any problem doing it that way. Oh, alright, let's make it Int also for wcslen() so the whole file is consistent. Created attachment 111143 [details]
Add-replacements-for-wmemchr-and-wcsnlen-on-Linux
Make wcslen() operate on Int as well.
OK! I don't understand how my original wcsnlen() is compiling for me at all: I mean it's obvious that I forgot I was writing a while and changed to a for halfway through the statement, leading to something grossly syntactically invalid. I wonder why it didn't get a compilation error for me? (Because it didn't, and still doesn't. Weird upon weird! Ah well I'll be moving off this GCC release soon anyway, so I guess I don't care much. You caught it and that's what matters.) :-) There was also a problem with the actual replacement definition. For example wcsnlen had the following: #if defined(VGO_linux) STRNLEN(VG_Z_LIBC_SONAME, wcsnlen) STRNLEN(VG_Z_LIBC_SONAME, __GI_wcsnlen) #endif This could not have worked. OK OK I was clearly on some serious drugs that day. God only knows what I actually compiled (though it does seem to work, and is still working). I have now replaced it with your fixed patch, which also works while having the significant advantage of not being full of horrible syntax errors. :) Pushed as 23185f46a17079fcfca35c2ef335a598812cb23b. https://sourceware.org/git/?p=valgrind.git;a=commitdiff;h=23185f46a17079fcfca35c2ef335a598812cb23b |
I reported the problem yesterday in the GLIB bugzilla, because thinking it was a GLIBC bug ("sprintf "%ls": uninitialized memory access because of using SSE 4.1 (__wcsnlen_sse4_1)"), but Andreas Schwab pointed that it could be a false positive. Could it be a Valgrind regression problem? https://sourceware.org/bugzilla/show_bug.cgi?id=22703 From the GLIB ticket: The uninitialized memory access comes from the internal function __wcsnlen_sse4_1 (using SSE 4.1 on x86_64), both with and without optimizations -O0/-O3). I've found it with a Valgrind test that reported an error after updating the build machine from Ubuntu 16.04 to Ubuntu 17.10 (GCC 7.2.0, ldd --version shows "Ubuntu GLIBC 2.26-0ubuntu2"). Actual behavior seems "right", but because of Valgrind reporting conditional behavior based on uninitialized memory, I've set the severity to critical. Valgrind output: $ uname -a Linux luna 4.13.0-25-generic #29-Ubuntu SMP Mon Jan 8 21:14:41 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux $ gcc -O0 -ggdb sprintf_bug.c $ valgrind --tool=memcheck ./a.out (...) ==22373== Conditional jump or move depends on uninitialised value(s) ==22373== at 0x4F029D1: __wcsnlen_sse4_1 (strlen.S:161) ==22373== by 0x4EF0C4A: wcsrtombs (wcsrtombs.c:104) ==22373== by 0x4E91EE1: vfprintf (vfprintf.c:1643) ==22373== by 0x4EB513D: vsprintf (iovsprintf.c:42) ==22373== by 0x4E98FA3: sprintf (sprintf.c:32) ==22373== by 0x108833: main (sprintf_bug.c:13) ==22373== Uninitialised value was created by a heap allocation ==22373== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==22373== by 0x1087ED: main (sprintf_bug.c:9) (...) $ gcc -O3 sprintf_bug.c $ valgrind --tool=memcheck ./a.out (...) ==22707== Conditional jump or move depends on uninitialised value(s) ==22707== at 0x4F029D1: __wcsnlen_sse4_1 (strlen.S:161) ==22707== by 0x4EF0C4A: wcsrtombs (wcsrtombs.c:104) ==22707== by 0x4E91EE1: vfprintf (vfprintf.c:1643) ==22707== by 0x4F60A8A: __vsprintf_chk (vsprintf_chk.c:82) ==22707== by 0x4F609B9: __sprintf_chk (sprintf_chk.c:31) ==22707== by 0x108757: main (in /r0/done/repos/mlibsrt/a.out) (...) Source code for reproducing the bug: $ cat sprintf_bug.c #include <stdio.h> #include <string.h> #include <malloc.h> #include <wchar.h> int main() { char tmp[4096]; wchar_t *hello_bug = (wchar_t *)malloc(sizeof(wchar_t) * 4096); if (!hello_bug) return 1; wcscpy(hello_bug, L"Hello bug!"); sprintf(tmp, "%ls", hello_bug); /* <-- Valgrind blames this */ printf("%s\n", tmp); free(hello_bug); return 0; }