SUMMARY The following C testcase reliably gives an invalid read inside glibc. glibc developers suggest that this is fine and just needs a suppression to be added to valgrind. See https://sourceware.org/bugzilla/show_bug.cgi?id=31509 for discussion. -------- #include <stdio.h> #include <stdlib.h> #include <string.h> #include <wchar.h> int main() { const wchar_t in[] = {L'a', L'b', L'c', 0}; wchar_t out[3+1] = {0, }; printf("%p %p\n", in, out); size_t res = wcsxfrm(out, in, 3); printf("%lu\n", res); wchar_t *in2 = malloc(sizeof(wchar_t) * 4); memcpy(in2, in, sizeof(in)); printf("%p %p\n", in2, out); res = wcsxfrm(out, in2, 3); printf("%lu\n", res); free(in2); wchar_t *in3 = malloc(sizeof(wchar_t) * 4); memcpy(in3, in, sizeof(in)); printf("%p %p\n", in3, out); res = wcsxfrm(out, in3, 3); printf("%lu\n", res); free(in3); } -------- ==139735== Memcheck, a memory error detector ==139735== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al. ==139735== Using Valgrind-3.22.0 and LibVEX; rerun with -h for copyright info ==139735== Command: ./test ==139735== 0x1fff000330 0x1fff000320 3 0x4a5c480 0x1fff000320 3 0x4a5c4d0 0x1fff000320 ==139735== Invalid read of size 32 ==139735== at 0x49DAA2E: __wcpncpy_avx2 (strncpy-avx2.S:85) ==139735== by 0x493C560: wcsxfrm_l (strxfrm_l.c:679) ==139735== by 0x4012A1: main (in /home/slomo/tmp/test/test) ==139735== Address 0x4a5c4d0 is 0 bytes inside a block of size 16 alloc'd ==139735== at 0x484280F: malloc (vg_replace_malloc.c:442) ==139735== by 0x401258: main (in /home/slomo/tmp/test/test) ==139735== 3 ==139735== ==139735== HEAP SUMMARY: ==139735== in use at exit: 0 bytes in 0 blocks ==139735== total heap usage: 3 allocs, 3 frees, 1,056 bytes allocated ==139735== ==139735== All heap blocks were freed -- no leaks are possible ==139735== ==139735== For lists of detected and suppressed errors, rerun with: -s ==139735== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) SOFTWARE/OS VERSIONS This is with valgrind 3.22.0 and glibc 2.38 on Fedora 39. x86-64.
This needs a redirect, not a suppression.
We already have an override for wcsncpy and stpncpy. But not wcpncpy. See shared/vg_replace_strmem.c
(In reply to Mark Wielaard from comment #2) > We already have an override for wcsncpy and stpncpy. But not wcpncpy. > See shared/vg_replace_strmem.c Yes I think that this should be virtually the same as wcsncpy with only the return value being different.
Created attachment 167483 [details] first attempt at a patch I don't think I have access to a machine with F29 and avx2. Could you try this please?
Comment on attachment 167483 [details] first attempt at a patch Very nice. Before the patch: $ ~/src/valgrind/vg-in-place ./t ==66177== Memcheck, a memory error detector ==66177== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al. ==66177== Using Valgrind-3.23.0.GIT and LibVEX; rerun with -h for copyright info ==66177== Command: ./t ==66177== 0x1ffefff9f0 0x1ffefff9e0 3 0x4a58480 0x1ffefff9e0 3 0x4a584d0 0x1ffefff9e0 ==66177== Invalid read of size 32 ==66177== at 0x49D6A2E: __wcpncpy_avx2 (strncpy-avx2.S:85) ==66177== by 0x4938560: wcsxfrm_l (strxfrm_l.c:679) ==66177== by 0x4012A1: main (t.c:25) ==66177== Address 0x4a584d0 is 0 bytes inside a block of size 16 alloc'd ==66177== at 0x484278B: malloc (vg_replace_malloc.c:446) ==66177== by 0x401258: main (t.c:22) ==66177== 3 ==66177== ==66177== HEAP SUMMARY: ==66177== in use at exit: 0 bytes in 0 blocks ==66177== total heap usage: 3 allocs, 3 frees, 1,056 bytes allocated ==66177== ==66177== All heap blocks were freed -- no leaks are possible ==66177== ==66177== For lists of detected and suppressed errors, rerun with: -s ==66177== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) After the patch: $ ~/src/valgrind/vg-in-place ./t ==67120== Memcheck, a memory error detector ==67120== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al. ==67120== Using Valgrind-3.23.0.GIT and LibVEX; rerun with -h for copyright info ==67120== Command: ./t ==67120== 0x1ffefff9e0 0x1ffefff9d0 3 0x4a58480 0x1ffefff9d0 3 0x4a584d0 0x1ffefff9d0 3 ==67120== ==67120== HEAP SUMMARY: ==67120== in use at exit: 0 bytes in 0 blocks ==67120== total heap usage: 3 allocs, 3 frees, 1,056 bytes allocated ==67120== ==67120== All heap blocks were freed -- no leaks are possible ==67120== ==67120== For lists of detected and suppressed errors, rerun with: -s ==67120== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) Implementation looks correct. Two small comments. + /* This checks for overlap after copying, unavoidable without */ \ + /* pre-counting length... should be ok */ \ + /* +4 because sizeof(wchar_t) == 4 */ \ + SizeT srclen = ((m < n) ? m+1 : n)*4; \ + RECORD_COPY(srclen); \ I found the comment a little confusing until I understood that the +4 really meant *4. + RECORD_OVERLAP_ERROR("wcspcpy", dst_orig, src_orig, 0); \ ^wcpncpy
I saw the "wcspcpy" as well. The +4 was a copy and paste, there are a few wide character functions with +4 and also some with *4. The testcase needs improving - printing the addresses of the stack and heap variables isn't much use. Also FreeBSD wcsxfrm doesn't use wcpncpy so I'd like a test directly using wcpncpy.
I just pushed a fix. Could you confirm that it is OK?
Haven't heard any complaints so closing this.
(In reply to Paul Floyd from comment #7) > I just pushed a fix. > > Could you confirm that it is OK? Yes, this works correctly. Thanks for fixing it and sorry for taking so long to test it!