Last Comment Bug 558618 - TM: Crash [@ js_ValueToString] - Segfault when accessing out-of-range arguments on trace
: TM: Crash [@ js_ValueToString] - Segfault when accessing out-of-range argumen...
Status: RESOLVED FIXED
[sg:critical][ccbr] fixed-in-tracemonkey
: crash, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on:
Blocks: 453730
  Show dependency treegraph
 
Reported: 2010-04-10 21:13 PDT by David Anderson [:dvander]
Modified: 2013-01-19 14:26 PST (History)
11 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
needed
.7-fixed
unaffected


Attachments
fix (847 bytes, patch)
2010-04-10 21:27 PDT, David Anderson [:dvander]
gal: review+
Details | Diff | Splinter Review
fix (833 bytes, patch)
2010-04-21 00:58 PDT, David Anderson [:dvander]
dmandelin: review+
christian: approval1.9.2.7+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2010-04-10 21:13:00 PDT
/* vim: set ts=4 sw=4 tw=99 et: */

function f() {
    this.search = function(a, b, c) {
        arguments[3] = { }
        arguments.length = 4;
        for (var i = 0; i < 100; i++) {
            print(arguments[3]);
        }
    }
}

var o = new f();
o.search({x: -1, y: -1, w: 100600, h: 100600});
Comment 1 David Anderson [:dvander] 2010-04-10 21:27:00 PDT
Created attachment 438309 [details] [diff] [review]
fix

basically another variant of bug 554670
Comment 2 Andreas Gal :gal 2010-04-11 09:58:59 PDT
Can you explain to me why we crash? I remember that we made this trace for a reason, so we are going to break some insane benchmark code.
Comment 3 David Anderson [:dvander] 2010-04-11 16:12:43 PDT
We crash because the recorder state does not match the interpreter state. The interpreter is placing an object on the stack and the tracer is placing a void.
Comment 4 Andreas Gal :gal 2010-04-11 16:33:17 PDT
What object and why?
Comment 5 Andreas Gal :gal 2010-04-11 16:33:41 PDT
Is this the { } object there? Can we access that from trace?
Comment 6 David Anderson [:dvander] 2010-04-11 16:47:15 PDT
When the JIT goes to box up the stack for Print(), it applies the interpreter's type (object) to the value in the JIT frame, which is an unboxed JSVAL_VOID - the result is Print() gets a vp with a "2" in it, NULL tagged as double.

We don't access overridden arguments anywhere on trace, as comment #1 hinted this is a variant of a similar case that was fixed recently:

> var a = arguments;
> a[3] = { };
> a.length = 4;

It's probably possible to trace it if we treat it as a generic GETELEM or something, but that would be a big patch. At least for 1.9.2 I'd prefer to just not trace it.
Comment 7 Andreas Gal :gal 2010-04-12 16:26:47 PDT
Comment on attachment 438309 [details] [diff] [review]
fix

Double check with sayrer that this doesn't regress that weird argc overshooting benchmark.
Comment 8 Robert Sayre 2010-04-15 10:37:48 PDT
try this in a browser with the peacekeeper tests.
Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-19 10:47:17 PDT
Giving this the same status (wanted, not blocking) as bug 554670, renominate for blocking if you feel that's inappropriate!
Comment 10 Gary Kwong [:gkw] [:nth10sd] 2010-04-19 21:43:14 PDT
autoBisect confirms that this is probably related to bug 453730:

The first bad revision is:
changeset:   30020:c76558a87dd9
user:        David Mandelin
date:        Wed Jul 08 11:16:41 2009 -0700
summary:     Bug 453730: trace JSOP_ARGUMENTS, r=gal
Comment 11 Gary Kwong [:gkw] [:nth10sd] 2010-04-19 21:46:55 PDT
Stack:

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000000
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   js-opt-32-tm-darwin           	0x000dbd13 js_ValueToString + 211
1   js-opt-32-tm-darwin           	0x00004406 Print(JSContext*, unsigned int, long*) + 166
2   ???                           	0x003d2f84 0 + 4009860
3   js-opt-32-tm-darwin           	0x00127480 js::ExecuteTree(JSContext*, js::TreeFragment*, unsigned int&, js::VMSideExit**) + 736
4   js-opt-32-tm-darwin           	0x001427c8 js::MonitorLoopEdge(JSContext*, unsigned int&, js::RecordReason) + 1064
5   js-opt-32-tm-darwin           	0x00062533 js_Interpret + 52291
6   js-opt-32-tm-darwin           	0x00065653 js_Execute + 531
7   js-opt-32-tm-darwin           	0x0000f33c JS_ExecuteScript + 60
8   js-opt-32-tm-darwin           	0x00004daf Process(JSContext*, JSObject*, char*, int) + 1647
9   js-opt-32-tm-darwin           	0x00008dea main + 1626
10  js-opt-32-tm-darwin           	0x000028dd _start + 208
11  js-opt-32-tm-darwin           	0x0000280c start + 40
Comment 12 David Anderson [:dvander] 2010-04-21 00:58:22 PDT
Created attachment 440436 [details] [diff] [review]
fix

branch fix. does not regress peacekeeper.
Comment 13 David Anderson [:dvander] 2010-04-21 01:11:59 PDT
http://hg.mozilla.org/tracemonkey/rev/7dcbfdfd7f48

I don't think this is exploitable on x86 because the low 32-bits will always be 0x2, and it'll act like a NULL deref/correctness bug if it escapes. On x64 the high 32-bits will be garbage. Well, at the least it's a pretty trivial crash to hit and experiment with so I won't mind if the bug stays closed/peach-coloured.
Comment 15 christian 2010-06-11 15:43:34 PDT
Comment on attachment 440436 [details] [diff] [review]
fix

a=LegNeato for 1.9.2.6. Please land this on mozilla-1.9.2 default.
Comment 17 :Ehsan Akhgari 2010-06-25 14:55:14 PDT
Bustage fix (caused by the fact that I transplanted instead of pushing the other patch in the bug): http://hg.mozilla.org/releases/mozilla-1.9.2/rev/02eb574eb149
Comment 18 christian 2010-06-30 22:40:12 PDT
I want to confirm in the bug (because it is unclear)...is 1.9.1 affected? If so we'll mark it as such.
Comment 19 christian 2010-06-30 22:40:50 PDT
err, I mean we'll mark it as unaffected if this is not an issue on 1.9.1.
Comment 20 David Anderson [:dvander] 2010-06-30 22:48:42 PDT
1.9.1 is not affected.
Comment 21 christian 2010-06-30 22:57:33 PDT
Thanks so much for the confirmation!
Comment 22 Al Billings [:abillings] 2010-07-15 18:28:08 PDT
What is the STR for this for verification in 1.9.2? I tried the code in comment 0 in a JS Shell but it didn't crash (just gave me a bunch of objects).
Comment 23 Christian Holler (:decoder) 2013-01-19 14:26:49 PST
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929

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