1126318 - Assertion failure: isNative(), at c:\users\mozilla\debug-builds\mozilla-central\ js\src\jsfun.h:413

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Carsten Book [:Tomcat]]

found via Bughunter and classified medium exploitable on mac at least:

Assertion failure: isNative(), at c:\users\mozilla\debug-builds\mozilla-central\
js\src\jsfun.h:413


Steps to reproduce:
-> Load http://www.jagodnik.pl/ in a Trunk Debug Build
--> Assertion failure

Comment 1

[Carsten Book [:Tomcat]]

Attachment: windows !exploitable report

Comment 2

[Carsten Book [:Tomcat]]

Attachment: windgb data win 7 trunk debug build

stack trace etc as requested.

Jan: could you take a look, thx!

Comment 3

[Andrew McCreight [:mccr8]]

The medium exploitable part here isn't really relevant, because this is a crash on an assertion.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Um.

So we're in js::jit::TryAttachNativeGetPropStub but callee.ptr->isInterpreted() is true.

Also, cacheableCall is false and isScripted is false in this case.  As in, we had a scripted function with no jitcode.

Looks like the code added in bug 1007631 doesn't really handle this case.

Maybe we should early-return right before we start looking for jitinfo if !cacheableCall?  Or check cacheableCall && !isScripted when we're checking for a getter function here:

6542     if (!shape || !shape->hasGetterValue() || !shape->getterValue().isObject() ||
6543         !shape->getterObject()->is<JSFunction>())
6544     {
6545         return true;
6546     }

Comment 5

[Carsten Book [:Tomcat]]

Attachment: reduced local testcase

Comment 6

[Carsten Book [:Tomcat]]

seems now bughunter found this on a different site: http://www.liteonodd.com/en/service-support/download/cat_view/44-software-tool/54-utilities

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

Thanks for the analysis. This patch moves the is-cacheable-native-call up, before the jitInfo check.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Patch (ignore white space)

Same patch but with -w to ignore the indentation changes.

Comment 9

[Jan de Mooij [:jandem]]

I verified the patch fixes both crashes (original URL and the one in comment 6).

Good find, thanks.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

r=me, but could you add a testcase?  Seems like all that's needed to trigger the assert is a non-cacheable scripted getter, right?  Well, and I guess outerClass needs to be true too.

I don't see a way to do the outerClass thing in shell (e.g. the ComplexObject stuff is dead code, afaict) but in a jsreftest or other test running in browser this:

<script>
Object.defineProperty(window, "foo",
  {
    get: function() { return 5; },
    configurable: true
  }
);
for (var i = 0; i < 100; ++i) window.foo;
</script>

reproduces the assert quite nicely for me.

Comment 11

[Jan de Mooij [:jandem]]

Added a jsreftest and verified it asserted without the patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/66338a89015f

Also this is not security-sensitive. If |callee| is scripted, the callee->jitInfo()->needsOuterizedThisObject() call could return a bogus value, but it doesn't matter as the is-native check below that will be false, so nothing depends on this value.

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/66338a89015f

Comment 13

[Jan de Mooij [:jandem]]

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

Approval Request Comment
[Feature/regressing bug #]: Bug 1007631.
[User impact if declined]: Potential crashes or assertion failures in debug builds.
[Describe test coverage new/current, TreeHerder]: On m-c for a few days.
[Risks and why]: Very low, just moves a check up.
[String/UUID change made/needed]: None.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Aurora+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e9b093db1fb