IonMonkey: "Assertion failure: [infer failure] Missing type pushed 0: void,"

VERIFIED FIXED in Firefox 19

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla19
x86_64
Mac OS X
assertion, regression, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox16 unaffected, firefox17 unaffected, firefox18 unaffected, firefox19+ fixed, firefox-esr10 unaffected, firefox-esr17 unaffected)

Details

(Whiteboard: [ion:p1][adv-main19-])

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Created attachment 676699 [details]
stack

function f(code) {
    eval(code)
}
f("\
    function h({x}) {\
	    print(x)\
	}\
	h(/x/);\
")

asserts js debug shell on IonMonkey changeset a2a201dd7a85 with --ion-eager at Assertion failure: [infer failure] Missing type pushed 0: void,

s-s because inference failures are bad, assuming sec-critical worse-case.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   111708:4a2c17905a17
user:        Nicolas B. Pierron
date:        Mon Oct 29 14:48:45 2012 -0700
summary:     Bug 792631 - Add IC for missing properties. r=dvander
(Reporter)

Comment 1

6 years ago
Tabs-less version:

function f(code) {
    eval(code)
}
f("\
    function h({x}) {\
        print(x)\
    }\
    h(/x/);\
")
status-firefox-esr10: --- → unaffected
status-firefox16: --- → unaffected
status-firefox17: --- → unaffected
status-firefox18: --- → unaffected
status-firefox19: --- → affected
status-firefox-esr17: --- → unaffected
tracking-firefox19: --- → ?
Keywords: sec-critical
This bug seems to be masked by the patch provided in Bug 807035.  Still I doubt this might be a good reason.  This show that there is a missing monitored case, so this need more investigation to figure out what is wrong.

Especially I don't understand how Bug 807035's patch will fix this whitout it hitting the assertion replaced by the patch before.
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Jan, The problem seems to be that IonBuilder does not monitor missing types, do you think we should ensure that we add monitor instructions to all path which have a “missing” typeset?  Currently, when we have a missing type, IonBuilder does not produced any monitor instructions and do not add a type barrier.

Missing type at #4:00021 pushed 0: void
Function #4 _build/bug807047.js (line 3):
locals:
    return: void
    this: void
    arg0: object[1] [0x7f7bf3b081f0]
#4:00000:   3  getarg 0
  type 0: object[1] [0x7f7bf3b081f0]
#4:00003:   3  dup
  type 0: object[1] [0x7f7bf3b081f0]
  type 1: object[1] [0x7f7bf3b081f0]
#4:00004:   3  getprop "x"  <--- Ion GetPropertyCache is returning undefined.
  typeset 0: missing
  type 0: missing
#4:00009:   3  setlocal 0   <--- Store undefined.
  type 0: missing
#4:00012:   3  pop
#4:00013:   3  pop
#4:00014:   3  callname "print"  <--- bailout here.
  typeset 1: object[1] <0x7f7bf3b15480>
  type 0: object[1] <0x7f7bf3b15480>
  barrier: unknown
#4:00019:   3  undefined
  type 0: void
#4:00020:   3  notearg
#4:00021:   3  getlocal 0  <--- fails here, because we observe undefined.
  type 0: missing
#4:00024:   3  notearg
#4:00025:   3  call 1
  typeset 2: missing
  type 0: missing
#4:00028:   3  pop
#4:00029:   3  stop
Flags: needinfo?(jdemooij)
TI infers property types for "normal" data properties (and dense array elements) on native objects and assumes they exist. If we do anything else (read a missing property, invoke a getter, read from a non-native object, etc) the result has to be dynamically monitored.

The problem here seems to be that the cache is idempotent and in that case we don't (and can't) call TypeScript::Monitor. So where you call IsCacheableNoProperty you also have to check !cache.idempotent().
Flags: needinfo?(jdemooij)
How a cache which is checking that a property does not exists to return a knwon constant cannot be idempotent?  Can we just add the monitored undefined when we produce the IC path instead of checking for the idempotent flag and instead of monitoring all the time?
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #5)
> How a cache which is checking that a property does not exists to return a
> knwon constant cannot be idempotent?

Because TI requires us to monitor the "undefined" type at least once, and monitoring is not possible if the cache is idempotent (we have no script/pc because GVN may have 'combined' multiple ICs with different script/pc's, etc).

We could check in IonBuilder if the return type includes "undefined" and allow using an idempotent cache, because we don't have to monitor in that case. However this doesn't seem worth the complexity.

> Can we just add the monitored
> undefined when we produce the IC path instead of checking for the idempotent
> flag and instead of monitoring all the time?

If you check for the idempotent flag, we will just invalidate and mark the cache as not-idempotent. The next time we compile we will add a non-idempotent cache and we can attach a missing-property stub. Note that without --ion-eager or --no-jm, JM will mark caches as non-idempotent and we likely don't have to invalidate at all.
Created attachment 677568 [details] [diff] [review]
Only use missing property cache on non-idempotent IC.

Only inline missing properties if the IC is non-idempotent. Mark the IC as not-being idempotent in JM.
Attachment #677568 - Flags: review?(jdemooij)
Comment on attachment 677568 [details] [diff] [review]
Only use missing property cache on non-idempotent IC.

Review of attachment 677568 [details] [diff] [review]:
-----------------------------------------------------------------

Can you also add the testcase?
Attachment #677568 - Flags: review?(jdemooij) → review+
Created attachment 677847 [details] [diff] [review]
Only use missing property cache on non-idempotent IC.

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Easily, with the test case in it.  Otherwise not trivial

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes.

Which older supported branches are affected by this flaw?
None

If not all supported branches, which bug introduced the flaw?
Bug 792631

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
N/A

How likely is this patch to cause regressions; how much testing does it need?
Regression: small memory impact. (non-idempotent IC)
Testing: should not need much.
Attachment #677847 - Flags: sec-approval?
Attachment #677847 - Flags: review+
Comment on attachment 677847 [details] [diff] [review]
Only use missing property cache on non-idempotent IC.

Review of attachment 677847 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/ion/bug808023.js
@@ +1,1 @@
> +

Need to avoid breaking the test suite:

// |jit-test| error: TypeError
Comment on attachment 677847 [details] [diff] [review]
Only use missing property cache on non-idempotent IC.

Giving sec-approval+ since it doesn't affect any existing versions that we've shipped.
Attachment #677847 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/197b182baf4f
https://hg.mozilla.org/mozilla-central/rev/0aa8d7cd5b8f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox19: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Duplicate of this bug: 807943
Duplicate of this bug: 808453
Duplicate of this bug: 808023

Updated

6 years ago
tracking-firefox19: ? → +
Can this be put in testsuite?
Flags: in-testsuite?
Whiteboard: [ion:p1] → [ion:p1][adv-main19-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.