Bug 285662 - Memcheck needs to replace memcpy/memmove (and most certainly others) in VG_Z_LIBC_SONAME on Darwin
Summary: Memcheck needs to replace memcpy/memmove (and most certainly others) in VG_Z_...
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: memcheck (show other bugs)
Version: 3.7 SVN
Platform: Unlisted Binaries macOS
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-03 15:27 UTC by Alexander Potapenko
Modified: 2015-01-18 08:38 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Potapenko 2011-11-03 15:27:52 UTC
$ cat p.cc
#include <stdio.h>
#include <stdlib.h>
#define SIZE 142
int main() {
  int i;
  char *m = (char*)valloc(SIZE);
  for (i = 0; i < SIZE; i++) m[i] = 'a';
  m[SIZE-1] = 0;
  printf("&m: %p\n", m);
  printf("m: %s\n", m);
  return 0;
}

$ g++ p.cc -o p
$ inst/bin/valgrind ./p
==38912== Memcheck, a memory error detector
--38912-- ./p:
--38912-- dSYM directory is missing; consider using --dsymutil=yes
&m: 0x100356000
==38912== Invalid read of size 8
==38912==    at 0x7FFFFFE00D78: ???
==38912==    by 0x10010DE74: __sfvwrite (in /usr/lib/libSystem.B.dylib)
==38912==    by 0x10010D7DB: __vfprintf (in /usr/lib/libSystem.B.dylib)
==38912==    by 0x10014B8AA: vfprintf_l (in /usr/lib/libSystem.B.dylib)
==38912==    by 0x100179006: printf (in /usr/lib/libSystem.B.dylib)
==38912==    by 0x100000EF5: main (in ./p)
==38912==  Address 0x100356088 is 136 bytes inside a block of size 142 alloc'd
==38912==    at 0x1000101F3: memalign (vg_replace_malloc.c:697)
==38912==    by 0x100010A84: valloc (vg_replace_malloc.c:735)
==38912==    by 0x100000E99: main (in ./p)
==38912== 
m: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
==38912==
Comment 1 Alexander Potapenko 2011-11-03 15:37:50 UTC
This code triggers a very subtle optimization in memcpy, which sometimes reads 64-byte blocks of memory if the source pointer is properly aligned:

0xffff0d65:	movdqa 0x3(%esi,%edx,1),%xmm1
0xffff0d6b:	movdqa 0x13(%esi,%edx,1),%xmm2
0xffff0d71:	movdqa 0x23(%esi,%edx,1),%xmm3
0xffff0d77:	movdqa 0x33(%esi,%edx,1),%xmm4

I haven't found any kernel sources containing this, but have spent some time debugging the case. If necessary, I can give more details. The full listing of the kernel memcpy implementation can be obtained from gdb by disassembling at 0xffff07a0

This happens because the line replacing memcpy on Darwin is commented out.
The following patch fixes the problem for me:

$ svn diff memcheck/mc_replace_strmem.c 
Index: memcheck/mc_replace_strmem.c
===================================================================
--- memcheck/mc_replace_strmem.c	(revision 12257)
+++ memcheck/mc_replace_strmem.c	(working copy)
@@ -849,7 +849,7 @@
  MEMCPY(NONE, ZuintelZufastZumemcpy)
 
 #elif defined(VGO_darwin)
- //MEMCPY(VG_Z_LIBC_SONAME,  memcpy)
+ MEMCPY(VG_Z_LIBC_SONAME,  memcpy)
  //MEMCPY(VG_Z_DYLD,         memcpy)
  MEMCPY(VG_Z_LIBC_SONAME,  memcpyZDVARIANTZDsse3x) /* memcpy$VARIANT$sse3x */
  MEMCPY(VG_Z_LIBC_SONAME,  memcpyZDVARIANTZDsse42) /* memcpy$VARIANT$sse42 */

$ inst/bin/valgrind ./p
--40937-- ./p:
--40937-- dSYM directory is missing; consider using --dsymutil=yes
&m: 0x100356000
m: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
==40937==
Comment 2 Alexander Potapenko 2011-11-03 15:45:11 UTC
The problem affects at least Mac OS 10.5 and 10.6
We have first encountered this problem running Chromium tests on Mac OS 10.5. Valgrind reported a number of invalid reads in __sfvwrite, which were related to incorrect handling of memcpy(), and some similar invalid reads in __CFStringCreateImmutableFunnel3, which were related to memmove().

This makes me think that the bunch of other string functions from VG_Z_LIBC_SONAME should be uncommented in 
memcheck/mc_replace_strmem.c instead of cleaning them up (see the comment to r12043)
Comment 3 Alexander Potapenko 2012-02-06 14:13:50 UTC
Because on Lion memcpy falls back to memmove this patch is invalid and should be applied iff DARWIN_VERS < DARWIN_10_7.
See http://code.google.com/p/valgrind-variant/issues/detail?id=5 for additional info.
Comment 4 Julian Seward 2012-02-22 17:32:20 UTC
I can't reproduce this at all, on my old low end Mac Mini.

The code you mention ..

  0xffff0d65:    movdqa 0x3(%esi,%edx,1),%xmm1
  0xffff0d6b:    movdqa 0x13(%esi,%edx,1),%xmm2
  0xffff0d71:    movdqa 0x23(%esi,%edx,1),%xmm3
  0xffff0d77:    movdqa 0x33(%esi,%edx,1),%xmm4

is very similar to code in _memcpy$VARIANT$sse42 and
_memcpy$VARIANT$sse3x.  But memcheck intercepts both of those.

In the stack trace you show, it's not clear what function
the offending access is in:

==38912== Invalid read of size 8
==38912==    at 0x7FFFFFE00D78: ???

I guess the really bad scenario is that that is a page of code
created by the kernel; it has no name (and hence is uninterceptable)
and it is dynamically dispatched to from plain "memcpy".  But I'm
just guessing here.
Comment 5 Julian Seward 2012-03-07 15:31:20 UTC
I committed a patch that reinstates the memcpy and memmove intercepts
for <= 10.6, and that seems to stop the problem.  Thanks for the
test case btw.

> This makes me think that the bunch of other string functions from
> VG_Z_LIBC_SONAME should be uncommented in 
> memcheck/mc_replace_strmem.c instead of cleaning them up

Which particular string functions are you thinking of?
Comment 6 Julian Seward 2012-06-30 15:27:43 UTC
Fixed, r12423.
Comment 7 Rhys Kidd 2014-10-09 10:36:41 UTC
Bug can probably be closed now?