557168 - 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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 1

[Brendan Eich [:brendan]]

AutoBisect blames what change?

/be

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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

Comment 5

[Jesse Ruderman]

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

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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.

Comment 7

[Robert Sayre]

the bug is open now. what do we win?

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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]

Comment 9

[Robert Sayre]

(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?

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 11

[Brendan Eich [:brendan]]

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

/be

Comment 12

[Andreas Gal :gal]

I can reproduce with TM tip.

Comment 13

[Brendan Eich [:brendan]]

Attachment: 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

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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)

Comment 15

[Brendan Eich [:brendan]]

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

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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

[Andreas Gal :gal]

Comment on attachment 437218 [details] [diff] [review]
fix

Did you check the write path too?

Comment 18

[Andreas Gal :gal]

Also please add the testcase to trace-tests. Nice catch Gary. Thanks.

Comment 19

[Brendan Eich [:brendan]]

(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

Comment 20

[Brendan Eich [:brendan]]

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

Comment 21

[Jeff Walden [:Waldo]]

(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.

Comment 22

[Nicholas Nethercote [inactive]]

Three cheers for LIR type-checking:  hip hip...

Comment 23

[Brendan Eich [:brendan]]

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

[Robert Sayre]

(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

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e84d48305e39

Comment 26

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/e4x/bug557168-1.js.