710438 - Invalid read of size 8 with testcase at JSFlatString* JSRope::flattenInternal in Valgrind

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

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

Attachment: testcase

The attached testcase shows a Valgrind error in js opt shell on m-c changeset 63bff373cb94 without any CLI arguments with an invalid read of size 8.

The following command was used:

valgrind --smc-check=all-non-file --dsymutil=yes ./js testcase.js

Comment 1

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

Attachment: stack

Valgrind stack

Comment 2

[Luke Wagner [:luke]]

Hmm, I can't repro on 10.6 or linux64 for the given cset.

But the valgrind spew is interesting!  It clearly points to the PodCopy for the right child of the rope |"(fun" + code|.  Now, 'code' is a compiler-produced atom, which agrees with where-was-this-allocated stack.  The interesting thing is that the atom length is 196 so, since valgrind says "Address 0x102274148 is 392 bytes inside a block of size 394 alloc'd" that means we are doing an 8 byte load starting at the terminating null char.  That means either (1) the length of the atom is wrong (by exactly 8 bytes...) or (2) memcpy() is running past the end.  (2) may very well be the case if memcpy() thinks it knows something about how malloc() works (which valgrind has replaced).

Even if I can't repro locally, there is a simple way to test if Gary would be willing to try something out: could you add a printf to JSRope::flattenInternal (js/src/vm/String.cpp:191)

         size_t len = right.length();
+        printf("right.d.u1.chars: %p, len: %d\n", (void *)right.d.u1.chars, (int)len);
         PodCopy(pos, right.d.u1.chars, len);

and print the new valgrind spew (that should include this printf)?

Comment 3

[Nicholas Nethercote [inactive]]

I can't repro on linux64 either.

> (2) memcpy() is running past the end.  (2) may very well be the
> case if memcpy() thinks it knows something about how malloc() works (which
> valgrind has replaced).

Many implementations of memcpy() read past the end of buffers in "safe" ways, so Valgrind replaces memcpy() and various variants with its own version as well :)  
And judging from valgrind/coregrind/m_redir.c if Valgrind can't replace memcpy() it'll abort.  So that suggests (1) is the cause.

Comment 4

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

> Even if I can't repro locally, there is a simple way to test if Gary would
> be willing to try something out: could you add a printf to
> JSRope::flattenInternal (js/src/vm/String.cpp:191)
> 
>          size_t len = right.length();
> +        printf("right.d.u1.chars: %p, len: %d\n", (void *)right.d.u1.chars,
> (int)len);
>          PodCopy(pos, right.d.u1.chars, len);
> 
> and print the new valgrind spew (that should include this printf)?

I patched m-c changeset 472b4a4ebea7 with this patch and got:

right.d.u1.chars: 0x1f99880, len: 196
==19697== Invalid read of size 8

Comment 5

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

Attachment: valgrind log of modified opt shell

Comment 6

[Luke Wagner [:luke]]

Unless I'm making a silly arithmetic error, comment 5 seems to indicate that memset is doing an 8 byte load past the end of the declared range (0x1f99880 + 2*196 = 0x1f99a08).  Nick, perhaps you have some insights?

Comment 7

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

Attachment: valgrind log of 32-bit opt shell, with --trace-redir=yes

Comment 8

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

Attachment: valgrind log of 64-bit opt shell, with --trace-redir=yes

Comment 9

[Luke Wagner [:luke]]

Julian, Nick said you would be the one to ask about this valgrind issue:

What I *think* we have here is a case where a call to glibc memcpy is not getting redirected to valgrind's memcpy.  The reason I think this is that (as comment 5 and 6 explain) the bad load is happening outside the range we observe being passed to memcpy.  The log with --trace-redir=yes is in comment 7 and comment 8.  A naive grepping does not show any patching of the memcpy callsite in question (0x1847a3, nor any address containing 0x1847), although I don't really know if that is what the log contains... it seems to, though.

Perhaps you could advise us further?

Comment 10

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

(In reply to Luke Wagner [:luke] from comment #9)
> Perhaps you could advise us further?

Julian, re-ping?

Comment 11

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

> Julian, re-ping?

fwiw, I can't reproduce on Valgrind SVN tip on a separate machine running Lion. The testcase in comment 0 was run on Snow Leopard.

Comment 12

[Julian Seward [:jseward]]

(In reply to Luke Wagner [:luke] from comment #9)
> What I *think* we have here is a case where a call to glibc memcpy is not
> getting redirected to valgrind's memcpy.

Luke, I think this is a plausible explanation, and looking at the logs in
comments 7 and 8 seems to support that.  One question is why we have not
been flooded with complaints about this before.

Gary, can you tell me about the CPU in the machine you see the problem on?
Is it quite old?  And what OS version is it running?

Comment 13

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

> Gary, can you tell me about the CPU in the machine you see the problem on?
> Is it quite old?  And what OS version is it running?

The CPU is a Intel Core 2 Duo P7550, is a mid-2009 Mac Mini running Mac OS X 10.6.8.

Comment 14

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

> The CPU is a Intel Core 2 Duo P7550, is a mid-2009 Mac Mini running Mac OS X
> 10.6.8.

I can still reproduce this with m-c changeset b0e65467c4c8 and Valgrind SVN r12325.

Comment 15

[Julian Seward [:jseward]]

See bug 715750 comment 2.

Comment 16

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

Duping to bug 715750, there is a potential patch for Valgrind in that bug.

Comment 17

[Julian Seward [:jseward]]

On further investigation, this is different from bug 715750.  Reopening.

Comment 18

[Julian Seward [:jseward]]

I _believe_ this is now fixed in r12343.  I have smaller test case
which shows the same problem, and it runs cleanly on r12423.  Gary,
any chance you can re-test with the js shell?

Comment 19

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

(In reply to Julian Seward from comment #18)
> I _believe_ this is now fixed in r12343.  I have smaller test case
> which shows the same problem, and it runs cleanly on r12423.  Gary,
> any chance you can re-test with the js shell?

WFM now. Resolved FIXED by a known Valgrind patch. Thanks Julian!

Comment 20

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

WFM is a better resolution.