TM: Crash [@ js_ValueToString] or [@ js_ValueToNumber] or "Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'eqq' is 'imml' which has type int32 (expected int64): 0 (../nanojit/LIR.cpp" with e4x

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: gkw, Assigned: brendan)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey, crash signature)

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
x = <x/>
for (var z = 0; z < 4; z++) {
    print(x.z)
}

crashes js debug and opt shells at js_ValueToString on TM tip with -j.
(Assignee)

Comment 1

8 years ago
AutoBisect blames what change?

/be
(Reporter)

Comment 2

8 years ago
autoBisect shows this is probably related to bug 554996:

The first bad revision is:
changeset:   39598:eb5999d8b46f
user:        Jason Orendorff
date:        Mon Mar 29 10:35:38 2010 -0500
summary:     Bug 554996 - Eliminate native-ops check before testing property cache. Part 2: tracer. r=gal.
Blocks: 554996
OS: Linux → All
Hardware: x86 → All
(Reporter)

Comment 3

8 years ago
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-dbg-32-tm-darwin           	0x00118c5e js_ValueToString + 203
1   js-dbg-32-tm-darwin           	0x0001655f JS_ValueToString + 24
2   js-dbg-32-tm-darwin           	0x00009cdd Print(JSContext*, unsigned int, long*) + 60 (js.cpp:1061)
3   ???                           	0x003eaf76 0 + 4108150
4   js-dbg-32-tm-darwin           	0x00150c32 js::ExecuteTrace(JSContext*, nanojit::Fragment*, js::InterpState&) + 89
5   js-dbg-32-tm-darwin           	0x0016735e js::ExecuteTree(JSContext*, js::TreeFragment*, unsigned int&, js::VMSideExit**) + 595
6   js-dbg-32-tm-darwin           	0x0016f99b js::MonitorLoopEdge(JSContext*, unsigned int&, js::RecordReason) + 934
7   js-dbg-32-tm-darwin           	0x00081fcf js_Interpret + 41718
8   js-dbg-32-tm-darwin           	0x0009ff76 js_Execute + 1401
9   js-dbg-32-tm-darwin           	0x000122cb JS_ExecuteScript + 54
10  js-dbg-32-tm-darwin           	0x0000aab4 Process(JSContext*, JSObject*, char*, int) + 458 (js.cpp:455)
11  js-dbg-32-tm-darwin           	0x0000b7f4 ProcessArgs(JSContext*, JSObject*, char**, int) + 2281 (js.cpp:869)
12  js-dbg-32-tm-darwin           	0x0000bbc1 main + 953 (js.cpp:4977)
13  js-dbg-32-tm-darwin           	0x0000288d _start + 208
14  js-dbg-32-tm-darwin           	0x000027bc start + 40
(Reporter)

Comment 4

8 years ago
(In reply to comment #2)
> autoBisect shows this is probably related to bug 554996:
> 
> The first bad revision is:
> changeset:   39598:eb5999d8b46f
> user:        Jason Orendorff
> date:        Mon Mar 29 10:35:38 2010 -0500
> summary:     Bug 554996 - Eliminate native-ops check before testing property
> cache. Part 2: tracer. r=gal.

Turning this sensitive because this supposed cause has already landed on mozilla-central and it is not suggested to 0-day nightly testers... Also due to the memory address found on line 3 of the stack:

3   ???                               0x003eaf76 0 + 4108150
Group: core-security

Comment 5

8 years ago
That memory address is trace code (it's called by js::ExecuteTrace), so its presence on the stack is not evidence of exploitability.
(Reporter)

Comment 6

8 years ago
(In reply to comment #5)
> That memory address is trace code (it's called by js::ExecuteTrace), so its
> presence on the stack is not evidence of exploitability.

Good point. Apologies, it slipped my mind, no wonder I didn't lock it during the bug filing process.

Opening.
Group: core-security

Comment 7

8 years ago
the bug is open now. what do we win?
(Reporter)

Comment 8

8 years ago
(In reply to comment #7)
> the bug is open now. what do we win?

Let's move on. Here's more testcases: (All tested on 64-bit Linux, haven't yet autoBisected, but they look similar)

x = <x/>
Function("\
  (function f() {\
    ({x:{b}}=x);\
    f()\
  })()\
")()

Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'eqq' is 'imml' which has type int32 (expected int64): 0 (../nanojit/LIR.cpp:2634)

x = <x/>
Function("\
  (function f() {\
    ({x}=x);\
    f()\
  })()\
")()

Assertion failure: ((jsval) obj & JSVAL_TAGMASK) == JSVAL_OBJECT, at ../jsapi.h:204

x = <x/>
Function("\
  for(var a = 0; a < 6; a++) {\
    Number(x.u)\
  }\
")()

Crash [@ js_ValueToNumber]
Summary: TM: Crash [@ js_ValueToString] with e4x → TM: Crash [@ js_ValueToString] or [@ js_ValueToNumber] or "Assertion failed: LIR type error (start of writer pipeline): arg 1 of 'eqq' is 'imml' which has type int32 (expected int64): 0 (../nanojit/LIR.cpp" with e4x

Comment 9

8 years ago
(In reply to comment #8)
> (In reply to comment #7)
> > the bug is open now. what do we win?
> 
> Let's move on.

Um, no. Seriously. Does it make you feel warm and fuzzy?
Group: core-security
(Reporter)

Comment 10

8 years ago
autoBisect confirms that all three testcases in comment 8 are probably related to bug 554996:

The first bad revision is:
changeset:   39598:eb5999d8b46f
user:        Jason Orendorff
date:        Mon Mar 29 10:35:38 2010 -0500
summary:     Bug 554996 - Eliminate native-ops check before testing property cache. Part 2: tracer. r=gal.

which is identical to comment 2, thus all testcases (including the initial one) may be related.
(Assignee)

Comment 11

8 years ago
This is a bug only in tracemonkey, not yet in m-c.

/be

Comment 12

8 years ago
I can reproduce with TM tip.
(Assignee)

Comment 13

8 years ago
Created attachment 437218 [details] [diff] [review]
fix

We always load obj->map->shape, even if it is SHAPELESS. We never fill the property cache for shapeless objects.

But we have to abort recording if a native object with other than the expect JSObjectOp.getProperty hook, namely js_GetProperty (whose fast path helper subroutine fills the cache when called directly from the interpreter). We count on shapes differing across classes, and classes differing if object-ops (any ops) differ, to keep us from entering a trace and getting past a shape or class guard on the wrong kind of object (E4X ones, in the tests at hand).

/be
Assignee: general → brendan
Status: NEW → ASSIGNED
Attachment #437218 - Flags: review?(jorendorff)
(Reporter)

Comment 14

8 years ago
(In reply to comment #11)
> This is a bug only in tracemonkey, not yet in m-c.
> 
> /be

On the contrary, I could reproduce the crashes in 64-bit Linux shell with -j, off mozilla-central. I am unable to get the browser to crash yet though.

Console output:

~/Desktop/jsfunfuzz-dbg-64-mc-40461-ac526bc2af65$ ./js-opt-64-mc-linux -j
js> x = <x/>
<x/>
js> Function("\
  for(var a = 0; a < 6; a++) {\
    Number(x.u)\
  }\
")()
Segmentation fault (core dumped)
(Assignee)

Comment 15

8 years ago
Gary, your comment 14 seemed to be contradicted by your comment 10, because bug 554996 is marked fixed-in-tracemonkey but still open. However I see its patch has already landed in m-c:

http://hg.mozilla.org/mozilla-central/log?rev=554996+

Sayrer, why is it still open?

I guess this bug should be s-s after all :-(.

/be
(Reporter)

Comment 16

8 years ago
(In reply to comment #15)
> Gary, your comment 14 seemed to be contradicted by your comment 10, because bug
> 554996 is marked fixed-in-tracemonkey but still open. However I see its patch
> has already landed in m-c:
> 
> http://hg.mozilla.org/mozilla-central/log?rev=554996+

I based comment 10 off tracemonkey testing - then verified that mozilla-central was affected in comment 14.

I had checked the mozilla-central `hg log` that the regressing patch was landed in m-c and thus made the comment that mozilla-central might be affected, as in comment 4.

I usually do not check the openness of bugs to deduce whether m-c is affected - instead checking the `hg log` whether the regressing patch is checked in, would be more reliable, in addition to actual verification.

Again apologies for the confusion, but let me move on by adding that the patch has fixed the crashes / asserts in both debug and opt shells in the patched m-c and tm sources, all tested in 64-bit Linux.

:)

Comment 17

8 years ago
Comment on attachment 437218 [details] [diff] [review]
fix

Did you check the write path too?
Attachment #437218 - Flags: review?(jorendorff) → review+

Comment 18

8 years ago
Also please add the testcase to trace-tests. Nice catch Gary. Thanks.
(Assignee)

Comment 19

8 years ago
(In reply to comment #17)
> (From update of attachment 437218 [details] [diff] [review])
> Did you check the write path too?

That is already covered in TR::record_JSOP_SETPROP:

    JSObject* obj = JSVAL_TO_OBJECT(l);
    if (obj->map->ops->setProperty != js_SetProperty)
        RETURN_STOP_A("non-native JSObjectOps::setProperty");

/be
(Assignee)

Comment 20

8 years ago
http://hg.mozilla.org/tracemonkey/rev/e84d48305e39
http://hg.mozilla.org/mozilla-central/rev/e631eb6b0006

Sayrer, sorry for assuming you would close open fixed-in-tm almost instantly -- I didn't see that you'd merged to m-c just today!

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
(Assignee)

Updated

8 years ago
Group: core-security
(In reply to comment #15)
> I guess this bug should be s-s after all :-(.

This sort of snafu is why I continue to believe paranoia wins the day unless proven otherwise.  The problem isn't an excess of hiddenness, it's that we aren't fixing the problems fast enough as they come in to keep hiddenness to the absolute minimum.  But that's to be addressed this quarter, so we're poised to treat the basic disease.
Three cheers for LIR type-checking:  hip hip...
(Assignee)

Comment 23

8 years ago
Yes, this was a case of merging tm to m-c with an unfixed and s-s blocker bug. It happened, so good for the s-s. But the vuln was on m-c less than half a day, from what I can tell. Still a good thing this bug was restricted during that time.

Nick: hurray!

/be

Comment 24

8 years ago
(In reply to comment #15)
> Gary, your comment 14 seemed to be contradicted by your comment 10, because bug
> 554996 is marked fixed-in-tracemonkey but still open. However I see its patch
> has already landed in m-c:
> 
> http://hg.mozilla.org/mozilla-central/log?rev=554996+
> 
> Sayrer, why is it still open?

I merged yesterday afternoon, don't usually mark it all fixed until I'm sure it's going to stick. So, doing that right now.

Comment 25

8 years ago
http://hg.mozilla.org/mozilla-central/rev/e84d48305e39
Crash Signature: [@ js_ValueToString] [@ js_ValueToNumber]
A testcase for this bug was automatically identified at js/src/jit-test/tests/e4x/bug557168-1.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.