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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

({crash, regression, testcase})

Trunk
crash, regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 needed, status1.9.2 .7-fixed, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical][ccbr] fixed-in-tracemonkey, crash signature)

Attachments

(2 attachments)

(Assignee)

Description

7 years ago
/* 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});
(Assignee)

Comment 1

7 years ago
Created attachment 438309 [details] [diff] [review]
fix

basically another variant of bug 554670
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #438309 - Flags: review?(gal)

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
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

7 years ago
What object and why?

Comment 5

7 years ago
Is this the { } object there? Can we access that from trace?
(Assignee)

Comment 6

7 years ago
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

7 years ago
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+

Updated

7 years ago
Whiteboard: [sg:crit]
Whiteboard: [sg:crit] → [sg:critical]

Updated

7 years ago
Keywords: testcase
Whiteboard: [sg:critical] → [sg:critical][ccbr]

Comment 8

7 years ago
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: ? → -
status1.9.2: --- → wanted
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
(Assignee)

Comment 12

7 years ago
Created attachment 440436 [details] [diff] [review]
fix

branch fix. does not regress peacekeeper.
Attachment #440436 - Flags: review?(dmandelin)
(Assignee)

Comment 13

7 years ago
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+
(Assignee)

Updated

7 years ago
Attachment #440436 - Flags: approval1.9.2.5?

Comment 14

7 years ago
http://hg.mozilla.org/mozilla-central/rev/7dcbfdfd7f48
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
blocking1.9.2: - → needed
status1.9.1: --- → ?

Comment 15

7 years ago
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+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e4bf0cdd829e
status1.9.2: wanted → .6-fixed
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

7 years ago
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

7 years ago
err, I mean we'll mark it as unaffected if this is not an issue on 1.9.1.
(Assignee)

Comment 20

7 years ago
1.9.1 is not affected.
status1.9.1: ? → unaffected

Comment 21

7 years ago
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.