Closed Bug 558618 Opened 14 years ago Closed 14 years ago

TM: Crash [@ js_ValueToString] - Segfault when accessing out-of-range arguments on trace

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking1.9.2 --- needed
status1.9.2 --- .7-fixed
status1.9.1 --- unaffected

People

(Reporter: dvander, Assigned: dvander)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [sg:critical][ccbr] fixed-in-tracemonkey)

Crash Data

Attachments

(2 files)

/* 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});
Attached patch fixSplinter Review
basically another variant of bug 554670
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #438309 - Flags: review?(gal)
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.
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.
What object and why?
Is this the { } object there? Can we access that from trace?
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 on attachment 438309 [details] [diff] [review]
fix

Double check with sayrer that this doesn't regress that weird argc overshooting benchmark.
Attachment #438309 - Flags: review?(gal) → review+
Whiteboard: [sg:crit]
Whiteboard: [sg:crit] → [sg:critical]
Keywords: testcase
Whiteboard: [sg:critical] → [sg:critical][ccbr]
try this in a browser with the peacekeeper tests.
Giving this the same status (wanted, not blocking) as bug 554670, renominate for blocking if you feel that's inappropriate!
blocking1.9.2: ? → -
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
Blocks: 453730
Severity: normal → critical
Keywords: crash, regression
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
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
Summary: TM: segfault when accessing out-of-range arguments on trace → TM: Crash [@ js_ValueToString] - Segfault when accessing out-of-range arguments on trace
Attached patch fixSplinter Review
branch fix. does not regress peacekeeper.
Attachment #440436 - Flags: review?(dmandelin)
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.
Whiteboard: [sg:critical][ccbr] → [sg:critical][ccbr] fixed-in-tracemonkey
Attachment #440436 - Flags: review?(dmandelin) → review+
http://hg.mozilla.org/mozilla-central/rev/7dcbfdfd7f48
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
blocking1.9.2: - → needed
Comment on attachment 440436 [details] [diff] [review]
fix

a=LegNeato for 1.9.2.6. Please land this on mozilla-1.9.2 default.
Attachment #440436 - Flags: approval1.9.2.5? → approval1.9.2.6+
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
I want to confirm in the bug (because it is unclear)...is 1.9.1 affected? If so we'll mark it as such.
err, I mean we'll mark it as unaffected if this is not an issue on 1.9.1.
Thanks so much for the confirmation!
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).
Group: core-security
Crash Signature: [@ js_ValueToString]
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.