Closed Bug 1126318 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: cbook, Assigned: jandem)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion, testcase)

Attachments

(5 files)

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
stack trace etc as requested.

Jan: could you take a look, thx!
Flags: needinfo?(jdemooij)
The medium exploitable part here isn't really relevant, because this is a crash on an assertion.
Summary: Medium Exploitable Assertion failure: isNative(), at c:\users\mozilla\debug-builds\mozilla-central\ js\src\jsfun.h:413 → Assertion failure: isNative(), at c:\users\mozilla\debug-builds\mozilla-central\ js\src\jsfun.h:413
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     }
Keywords: assertion, testcase
Attached file reduced local testcase
Attached patch PatchSplinter Review
Thanks for the analysis. This patch moves the is-cacheable-native-call up, before the jitInfo check.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8556425 - Flags: review?(bzbarsky)
Same patch but with -w to ignore the indentation changes.
I verified the patch fixes both crashes (original URL and the one in comment 6).

Good find, thanks.
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.
Attachment #8556425 - Flags: review?(bzbarsky) → review+
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.
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/66338a89015f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Attachment #8556425 - Flags: approval-mozilla-aurora?
Comment on attachment 8556425 [details] [diff] [review]
Patch

Aurora+
Attachment #8556425 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.