847606 - Crash with ion, setTimeout(Array.prototype.filter)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (10000 elements)

Testcase can cause:

* Assertion failure: hasfp(), at js/src/vm/Stack.h:1663
* Crash [@ js::ion::FastInvoke]

Security-sensitive because the testcase reminds me of bug 815010.

Comment 1

[Jesse Ruderman]

Attachment: testcase (10 elements)

This one requires:
  user_pref("javascript.options.ion.parallel_compilation", false);
  user_pref("javascript.options.methodjit.content", false);

Comment 2

[Boris Zbarsky [:bzbarsky]]

So all window.setTimeout does is cause a JS_CallFunctionValue on the bound function with absolutely nothing on the stack.

Eventually we end up in array_filter which does:

  FastInvokeGuard fig(cx, ObjectValue(*callable));
....
    if (!fig.invoke(cx))

And js::ion::FastInvoke does:

  StackFrame *fp = cx->fp();

but cx doesn't have an fp on it, so that hits an assert...  Not sure where the bug is here exactly, but it sure doesn't seem to be in setTimeout stuff.

Comment 3

[Jesse Ruderman]

bp-15f1bc07-f564-4394-bd85-a075f2130304

Comment 4

[Daniel Veditz [:dveditz]]

This case is a null deref, but could we do the same with a more dangerous bogus value?

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

We need js team input to sec rate this.

Comment 6

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

Pushing to Naveed to find an owner.

Comment 7

[Jan de Mooij [:jandem]]

Good find! The way FastInvoke works is pretty hackish, the patch in bug 868437 will make this much nicer (it removes the cx->fp() call and a lot of code around it).

This affects all branches, but should be uncommon: FastInvoke is only used when some natives implemented in C++ (Array.prototype.filter/sort/.., String.prototype.replace, etc) call back into JS.

Comment 8

[Jesse Ruderman]

Does the patch in bug 868437 fix this?

Do you have another plan for branches?  ("Don't bother fixing it on branches" is one possibility.)

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Jesse Ruderman from comment #8)
> Does the patch in bug 868437 fix this?

I just downloaded mozilla-inbound builds with/without that patch. Bug 868437 indeed fixed this.

> Do you have another plan for branches?  ("Don't bother fixing it on
> branches" is one possibility.)

Yeah I don't think it's worth fixing on branches: it's a safe NULL-deref and the bug has been there since Firefox 18.