Last Comment Bug 563243 - (64-bit) Invalid read / write when testcase is run in valgrind
: (64-bit) Invalid read / write when testcase is run in valgrind
Status: RESOLVED FIXED
: [sg:critical?][critsmash:resolved] fi...
: crash, regression, testcase, valgrind
Product: Core
Classification: Components
Component: JavaScript Engine
: Trunk
: x86 Linux
: -- critical (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: general
:
:
: jsfunfuzz 412296 563133
  Show dependency treegraph
 
Reported: 2010-05-02 10:14 PDT by Gary Kwong [:nth10sd]
Modified: 2011-03-29 19:25 PDT (History)
7 users (show)
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  .14+
  .14-fixed
  .17+
  .17-fixed


Attachments
valgrind log (5.79 KB, text/plain)
2010-05-02 10:14 PDT, Gary Kwong [:nth10sd]
no flags Details
gdb info for an opt 1.9.2 shell build (7.43 KB, text/plain)
2010-05-02 10:29 PDT, Gary Kwong [:nth10sd]
no flags Details
Proposed fix (4.09 KB, patch)
2010-05-03 13:39 PDT, Blake Kaplan (:mrbkap)
jorendorff: review+
Details | Diff | Splinter Review
fix for 192 (32.22 KB, patch)
2011-01-20 08:49 PST, Igor Bukanov
no flags Details | Diff | Splinter Review
fix for 192 анд 191 (4.14 KB, patch)
2011-01-20 08:51 PST, Igor Bukanov
clegnitto: approval1.9.2.14+
clegnitto: approval1.9.1.17+
Details | Diff | Splinter Review

Summon comment box

Description Gary Kwong [:nth10sd] 2010-05-02 10:14:18 PDT
Created attachment 442997 [details]
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 [:nth10sd] 2010-05-02 10:14:50 PDT
This shows up as a crash when jsfunfuzz is being run.
Comment 2 Gary Kwong [:nth10sd] 2010-05-02 10:17:47 PDT
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 [:nth10sd] 2010-05-02 10:29:59 PDT
Created attachment 442998 [details]
gdb info for an opt 1.9.2 shell build
Comment 4 Blake Kaplan (:mrbkap) 2010-05-03 13:39:28 PDT
Created attachment 443173 [details] [review]
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) 2010-05-03 13:40:22 PDT
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 2010-05-04 13:34:31 PDT
When did this regress? Does it affect any of our shipping branches?
Comment 7 Blake Kaplan (:mrbkap) 2010-05-04 15:28:06 PDT
This is a regression from bug 412296. It affects all active branches (note: it does *not* affect 1.9.0).
Comment 8 Gary Kwong [:nth10sd] 2010-05-04 19:08:31 PDT
(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) 2010-05-05 19:41:14 PDT
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 11 Daniel Veditz 2011-01-10 10:23:37 PST
Fixes bug 563133 for branches
Comment 12 Igor Bukanov 2011-01-20 08:44:35 PST
*** Bug 563133 has been marked as a duplicate of this bug. ***
Comment 13 Igor Bukanov 2011-01-20 08:49:27 PST
Created attachment 505425 [details] [review]
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 2011-01-20 08:51:19 PST
Created attachment 505426 [details] [review]
fix for 192 анд 191

The previous attachment had an unrelated diff.
Comment 15 Igor Bukanov 2011-01-20 09:00:45 PST
Comment on attachment 505426 [details] [review]
fix for 192 анд 191

The patch applies to 191 as-is
Comment 16 Christian Legnitto [:LegNeato] 2011-01-20 10:10:18 PST
Comment on attachment 505426 [details] [review]
fix for 192 анд 191

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

Note You need to log in before you can comment on or make changes to this bug.