622167 - GC related crash/asserts: isFunctionFrame() / obj->numSlots() >= first + count

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Shell test case for first assertion

The attached two testcases trigger two (likely related) assertions with the shell:

Assertion failure: isFunctionFrame(), at ../jsinterp.h:312
Assertion failure: obj->numSlots() >= first + count, at jsfun.cpp:1370

The second assertion isn't always triggered, sometimes also the first one shows up instead. For a reliable run, try running in valgrind also.

Both versions will crash when continuing after the assertions.

The bug might be related to 617139 because these examples also involve infinite recursion and GC triggering. The assertions are different though.

Comment 1

[Christian Holler (:decoder)]

Attachment: Shell test case for second assertion

Comment 3

[Jeff Walden [:Waldo]]

Taking, I've been working with code involving these assertions, so it's probably quickest for me to fix this...

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch

What happens:

  * we call a heavyweight function, which recurs until just short of
    over-recursion, and then...
  * we create a Call object for the (very nested) frame
  * we set the Call object's private to that stack frame
  * we call js::Interpret, which immediately errors out from over-recursion
  * ...and ScriptEpilogue is never called, so js_PutCallObject never unsets
    that Call object's stack frame pointer!
  * now the Call object's stack frame pointer dangles, or points into newer
    frames, or whatever

This doesn't look to be a problem in 192.  See the putActivationObjects call that happens regardless whether js_Interpret succeeded or failed:

http://hg.mozilla.org/releases/mozilla-1.9.2/file/c5b805f21466/js/src/jsinterp.cpp#l1386

It's only a recent problem likely introduced when we added ScriptEpilogue.

Comment 5

[Jeff Walden [:Waldo]]

Also not a problem in 191, which unconditionally called js_PutCallObject after calling js_Interpret.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 501570 [details] [diff] [review]
Patch

Nice find.

Comment 7

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/b33fddc6066e

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

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

Comment 9

[Daniel Veditz [:dveditz]]

How likely would a web attacker be able to pull off the conditions in comment 4? Is the impact here really "critical" or more "moderate"?

Comment 10

[Jeff Walden [:Waldo]]

It's easy to pull off those conditions.  What's harder is getting the now-dead call object to be traced.  The recursion error happens early enough that the call object pointer never escapes into script, so either you need to keep it on the stack in a way that it gets reused, you have to "guess" what it is and get it on the stack somehow (easier said than done), or something.  I don't know an easy way to do that, but I don't know there isn't an easy way.  Given that the testcase here was consistently reproducible, I suspect there probably is an easy way.  But that's only halfway: you still need to leverage the failed assertion further.  It might or might not be possible to do that.  But I suspect, given how many more slots you'd be tracing than would be okay, it's probably possible.

I'd say this leans more to critical than moderate, but I could be mistaken.  There's too much complexity of behavior here, none of it in "normal" semantics (well-defined by the relevant function interfaces, that is), to say without investigating a fair bit, I think.

Comment 11

[Daniel Veditz [:dveditz]]

Given the uncertainty bumping the severity down a notch to "high".

Comment 13

[Christian Holler (:decoder)]

Fixed for a long time and not affecting old branches, opening this.

Comment 14

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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