Bug 484002

Summary:	Add suppression for invalid read in glibc's __wcpncpy_avx2() via wcsxfrm()
Product:	[Developer tools] valgrind	Reporter:	Sebastian Dröge <slomo>
Component:	memcheck	Assignee:	Paul Floyd <pjfloyd>
Status:	RESOLVED FIXED
Severity:	normal	CC:	fweimer, mark, pjfloyd, sam
Priority:	NOR
Version First Reported In:	3.22.0
Target Milestone:	---
Platform:	Other
OS:	Linux
See Also:	https://sourceware.org/bugzilla/show_bug.cgi?id=31509
Latest Commit:		Version Fixed/Implemented In:
Sentry Crash Report:
Attachments:	first attempt at a patch

Description Sebastian Dröge 2024-03-19 14:28:47 UTC

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.

Comment 1 Paul Floyd 2024-03-19 16:29:03 UTC

This needs a redirect, not a suppression.

Comment 2 Mark Wielaard 2024-03-19 16:47:09 UTC

We already have an override for wcsncpy and stpncpy. But not wcpncpy.
See shared/vg_replace_strmem.c

Comment 3 Paul Floyd 2024-03-19 17:24:14 UTC

(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.

Comment 4 Paul Floyd 2024-03-19 19:18:36 UTC

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 5 Mark Wielaard 2024-03-19 19:47:43 UTC

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

Comment 6 Paul Floyd 2024-03-19 20:02:26 UTC

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.

Comment 7 Paul Floyd 2024-03-20 20:39:14 UTC

I just pushed a fix.

Could you confirm that it is OK?

Comment 8 Paul Floyd 2024-03-27 14:02:44 UTC

Haven't heard any complaints so closing this.

Comment 9 Sebastian Dröge 2024-04-01 20:28:09 UTC

(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!