715750 - Invalid read of size 4 with testcase at js::Sprint and maybe js_GC on the stack in Valgrind

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Valgrind stacks

eval("(evalcx(''))".replace(""), newGlobal("new-compartment"))

throws a bunch of Valgrind "invalid read of size 4" errors on js opt shell from m-c changeset 44d992ccc97a without any CLI arguments.

s-s because Valgrind invalid reads aren't good, assuming everything is correctly working.


Example stacks:

==28284== Invalid read of size 4
==28284==    at 0x838F04: _get_cpu_capabilities (in /usr/lib/system/libsystem_c.dylib)
==28284==    by 0x834FCF: memmove (in /usr/lib/system/libsystem_c.dylib)
==28284==    by 0x83D139: __udivmoddi4 (in /usr/lib/system/libsystem_c.dylib)
==28284==    by 0xB590E: js::Sprint(js::Sprinter*, char const*, ...) (jsopcode.cpp:829)
==28284==    by 0xBE784: Decompile(SprintStack*, unsigned char*, int) (jsopcode.cpp:2780)
==28284==    by 0xB7BE9: DecompileCode(JSPrinter*, JSScript*, unsigned char*, unsigned int, unsigned int) (jsopcode.cpp:5440)
==28284==    by 0xB7807: DecompileExpression(JSContext*, JSScript*, JSFunction*, unsigned char*) (jsopcode.cpp:5839)
==28284==    by 0xB7269: js_DecompileValueGenerator (jsopcode.cpp:5737)
==28284==    by 0x2E5CE: js_ReportValueErrorFlags(JSContext*, unsigned int, unsigned int, int, JS::Value const&, JSString*, char const*, char const*) (jscntxt.cpp:1219)
==28284==    by 0x4C18F: js_ReportIsNotFunction(JSContext*, JS::Value const*, unsigned int) (jsfun.cpp:2416)
==28284==    by 0x8F1D8: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:499)
==28284==    by 0x7F9A8: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:3414)
==28284==  Address 0xffff0020 is not stack'd, malloc'd or (recently) free'd

or

==28284== Invalid read of size 4
==28284==    at 0x7D1895: __commpage_gettimeofday (in /usr/lib/system/libsystem_c.dylib)
==28284==    by 0x7D17FB: gettimeofday (in /usr/lib/system/libsystem_c.dylib)
==28284==    by 0x12B579: PRMJ_Now (prmjtime.cpp:313)
==28284==    by 0x180845: js::gcstats::Statistics::beginGC(JSCompartment*, js::gcstats::Reason) (Statistics.cpp:300)
==28284==    by 0x4F3B5: js_GC(JSContext*, JSCompartment*, JSGCInvocationKind, js::gcstats::Reason) (Statistics.h:159)
==28284==    by 0x2D76C: js_DestroyContext(JSContext*, JSDestroyContextMode) (jscntxt.cpp:641)
==28284==    by 0x15DF8: JS_DestroyContext (jsapi.cpp:1171)
==28284==    by 0x8856: main (js.cpp:4966)
==28284==  Address 0xffff006c is not stack'd, malloc'd or (recently) free'd

Comment 1

[Luke Wagner [:luke]]

Julian, does this look like cases where valgrind isn't trapping the system calls?

Comment 2

[Julian Seward [:jseward]]

Yeah, this has the same underlying cause as bug 710438.  You might as well
close it as a dup.

The problem here and in bug 710438 is that memcheck is set up to intercept
functions called "memcpy$VARIANT$sse3x" and "memcpy$VARIANT$sse42" etc but
not plain "memcpy" (on MacOS, that is).  Can you try the following patch
(untested!) that also asks it to intercept the plain versions?

Index: memcheck/mc_replace_strmem.c
===================================================================
--- memcheck/mc_replace_strmem.c        (revision 12325)
+++ 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 */
@@ -981,7 +981,7 @@
  MEMMOVE(VG_Z_LIBC_SONAME, memmove)
 
 #elif defined(VGO_darwin)
- //MEMMOVE(VG_Z_LIBC_SONAME, memmove)
+ MEMMOVE(VG_Z_LIBC_SONAME, memmove)
  //MEMMOVE(VG_Z_DYLD,        memmove)#
  MEMMOVE(VG_Z_LIBC_SONAME,  memmoveZDVARIANTZDsse3x) /* memmove$VARIANT$sse3x */
  MEMMOVE(VG_Z_LIBC_SONAME,  memmoveZDVARIANTZDsse42) /* memmove$VARIANT$sse42 */

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Yeah, this has the same underlying cause as bug 710438.  You might as well
> close it as a dup.

Opening up since it seems to be a Valgrind issue and not s-s.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: more information from patched Valgrind

> The problem here and in bug 710438 is that memcheck is set up to intercept
> functions called "memcpy$VARIANT$sse3x" and "memcpy$VARIANT$sse42" etc but
> not plain "memcpy" (on MacOS, that is).  Can you try the following patch
> (untested!) that also asks it to intercept the plain versions?

That patch totally knocked out my MBP, first w/ a kernel panic when I ran `valgrind --dsymutil=yes ./js`, and the second time it just froze when I re-ran the same command after rebooting. I had to forcibly reboot both times.

I tried running `valgrind ls` and even it failed, albeit more gracefully.

Comment 6

[Julian Seward [:jseward]]

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #5)
> That patch totally knocked out my MBP, first w/ a kernel panic when I ran

Mmmhm, that's ungood.  What about the older Mac Mini mentioned in bug
710438 (which I thought is what you were actually going to test on ..)

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Just to confirm:

This bug:   Occurs on Lion, in new Mac Mini / MBP
Bug 710438: Occurs on Snow Leopard, in mid-2009 Mac Mini

Your patch fixes bug 710438 but blows up Lion, so maybe Lion needs a different fix from Snow Leopard. :(

Comment 8

[Julian Seward [:jseward]]

Revised version of patch, in which we only replace plain memcpy and memmove on
Snow Leopard but not on Lion.  Can you give this a try?

Index: memcheck/mc_replace_strmem.c
===================================================================
--- memcheck/mc_replace_strmem.c        (revision 12325)
+++ memcheck/mc_replace_strmem.c        (working copy)
@@ -849,7 +849,9 @@
  MEMCPY(NONE, ZuintelZufastZumemcpy)
 
 #elif defined(VGO_darwin)
- //MEMCPY(VG_Z_LIBC_SONAME,  memcpy)
+# if DARWIN_VERS == DARWIN_10_6
+  MEMCPY(VG_Z_LIBC_SONAME,  memcpy)
+# endif
  //MEMCPY(VG_Z_DYLD,         memcpy)
  MEMCPY(VG_Z_LIBC_SONAME,  memcpyZDVARIANTZDsse3x) /* memcpy$VARIANT$sse3x */
  MEMCPY(VG_Z_LIBC_SONAME,  memcpyZDVARIANTZDsse42) /* memcpy$VARIANT$sse42 */
@@ -981,7 +983,9 @@
  MEMMOVE(VG_Z_LIBC_SONAME, memmove)
 
 #elif defined(VGO_darwin)
- //MEMMOVE(VG_Z_LIBC_SONAME, memmove)
+# if DARWIN_VERS == DARWIN_10_6
+  MEMMOVE(VG_Z_LIBC_SONAME, memmove)
+# endif
  //MEMMOVE(VG_Z_DYLD,        memmove)#
  MEMMOVE(VG_Z_LIBC_SONAME,  memmoveZDVARIANTZDsse3x) /* memmove$VARIANT$sse3x */
  MEMMOVE(VG_Z_LIBC_SONAME,  memmoveZDVARIANTZDsse42) /* memmove$VARIANT$sse42 */

Comment 9

[Julian Seward [:jseward]]

This is filed in Valgrind's bug tracker at
https://bugs.kde.org/show_bug.cgi?id=285662

I can't reproduce this on a low end Mac Mini though.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Julian Seward from comment #9)
> This is filed in Valgrind's bug tracker at
> https://bugs.kde.org/show_bug.cgi?id=285662
> 
> I can't reproduce this on a low end Mac Mini though.

My Mac Mini (Lion) is a quad core i7 server model 2011 one, 8Gb ram.
My other Mac Mini (Snow Leopard) is a 2009 Core 2 Duo (dual core) one, 4Gb ram.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This patch seems to no longer cause a kernel panic in Lion, but with the testcase in comment 0, I still get invalid reads:

Index: memcheck/mc_replace_strmem.c
===================================================================
--- memcheck/mc_replace_strmem.c	(revision 12400)
+++ memcheck/mc_replace_strmem.c	(working copy)
@@ -850,7 +850,9 @@
  MEMCPY(NONE, ZuintelZufastZumemcpy)
 
 #elif defined(VGO_darwin)
- //MEMCPY(VG_Z_LIBC_SONAME,  memcpy)
+# if DARWIN_VERS == DARWIN_10_6
+ MEMCPY(VG_Z_LIBC_SONAME,  memcpy)
+# endif
  //MEMCPY(VG_Z_DYLD,         memcpy)
  MEMCPY(VG_Z_LIBC_SONAME,  memcpyZDVARIANTZDsse3x) /* memcpy$VARIANT$sse3x */
  MEMCPY(VG_Z_LIBC_SONAME,  memcpyZDVARIANTZDsse42) /* memcpy$VARIANT$sse42 */
@@ -982,7 +984,9 @@
  MEMMOVE(VG_Z_LIBC_SONAME, memmove)
 
 #elif defined(VGO_darwin)
- //MEMMOVE(VG_Z_LIBC_SONAME, memmove)
+# if DARWIN_VERS == DARWIN_10_6
+ MEMMOVE(VG_Z_LIBC_SONAME, memmove)
+# endif
  //MEMMOVE(VG_Z_DYLD,        memmove)#
  MEMMOVE(VG_Z_LIBC_SONAME,  memmoveZDVARIANTZDsse3x) /* memmove$VARIANT$sse3x */
  MEMMOVE(VG_Z_LIBC_SONAME,  memmoveZDVARIANTZDsse42) /* memmove$VARIANT$sse42 */

Comment 12

[Julian Seward [:jseward]]

(In reply to Julian Seward from comment #2)
> Yeah, this has the same underlying cause as bug 710438.  You might as well
> close it as a dup.

This is wrong .. 710438 is unrelated to this.  This seems to be related
to accesses to high memory (0xFFFFxxxx) and is unrelated to function
redirection.  Will reopen 710438.

Comment 13

[Julian Seward [:jseward]]

Confirming as a bug in V.  It could also have been caused by the OSX
kernel giving us bogus information about memory segments, but that's
not the case.  The logic in VG_(get_changed_segments) doesn't correctly
compute the changed segment set, leading Memcheck to incorrectly conclude
that the addresses in the range 0xFFFF0000 to 0xFFFFF000 (in these traces)
is inaccessible.

Comment 14

[Julian Seward [:jseward]]

A trip into the murky world of the OSX kernel moving memory mappings
around under our feet.  Anyway, fixed, r12422.  Thanks for the help
with finding a repro case.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Julian Seward from comment #14)
> A trip into the murky world of the OSX kernel moving memory mappings
> around under our feet.  Anyway, fixed, r12422.  Thanks for the help
> with finding a repro case.

WFM now. Resolved FIXED by a known Valgrind patch. You're welcome!

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

WFM is a better resolution.