563243 - (64-bit) Invalid read / write when testcase is run in valgrind

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: valgrind log

(function () {
    for (a in (function () {
        yield Array.reduce()
    })()) function () {}
})()

causes invalid reads and writes when run in a 1.9.2 64-bit Linux shell without -j on 1.9.2 tip. (Pass into 1.9.2 shell as a CLI argument, e.g. ./js a.js)

A valgrind log is attached. This does not seem to occur with TM tip. Setting s-s because I'm not sure how bad this is.

Comment 1

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

This shows up as a crash when jsfunfuzz is being run.

Comment 2

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

Tested on Ubuntu Linux 10.04 AMD64 on this 1.9.2 changeset:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d1aab61eb130

Comment 3

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

Attachment: gdb info for an opt 1.9.2 shell build

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

We haven't been able to assume anything about vp[2 + n] for n >= argc since 2006 or so... We don't actually fill missing args for fast natives.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Note: the changes to the slow version weren't necessary, but the two versions should probably match each other in behavior.

Comment 6

[Daniel Veditz [:dveditz]]

When did this regress? Does it affect any of our shipping branches?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

This is a regression from bug 412296. It affects all active branches (note: it does *not* affect 1.9.0).

Comment 8

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

(In reply to comment #7)
> This is a regression from bug 412296. It affects all active branches (note: it
> does *not* affect 1.9.0).

Strange - might just be me, but I couldn't seem to reproduce on TM tip, as per comment 0.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/0f5867192284

Yeah, I saw that, but I've definitely reproduced it on all branches. Not sure what you're seeing.

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0f5867192284

Comment 11

[Daniel Veditz [:dveditz]]

Fixes bug 563133 for branches

Comment 13

[Igor Bukanov]

Attachment: fix for 192

The patch is a trivial backport of the tm patch. Here is a plain diff between tm and this patch to confirm this:

24c24
<      if (!js_ComputeThis(cx, vp + 2))
---
>      if (!js_ComputeThis(cx, JS_FALSE, vp + 2))
59,60c59,60
< @@ -4333,15 +4325,8 @@ js_generic_native_method_dispatcher(JSCo
<      js_GetTopStackFrame(cx)->thisv = argv[-1];
---
> @@ -4463,15 +4455,8 @@ js_generic_native_method_dispatcher(JSCo
>      js_GetTopStackFrame(cx)->thisp = JSVAL_TO_OBJECT(argv[-1]);

Comment 14

[Igor Bukanov]

Attachment: fix for 192 анд 191

The previous attachment had an unrelated diff.

Comment 15

[Igor Bukanov]

Comment on attachment 505426 [details] [diff] [review]
fix for 192 анд 191

The patch applies to 191 as-is

Comment 16

[LegNeato]

Comment on attachment 505426 [details] [diff] [review]
fix for 192 анд 191

Approved for 1.9.2.14 and 1.9.1.17. Please land this as soon as possible!

Comment 17

[Igor Bukanov]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84f9dd0ecf14

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/223a9cfef4d8

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/08dff7a8e784