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

Status: VERIFIED FIXED

Description

[Gary Kwong [:gkw] [:nth10sd]]

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

Comment 1

[Gary Kwong [:gkw] [:nth10sd]]

Tabs-less version:

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

Comment 2

[Nicolas B. Pierron [:nbp] {backlog: 39}]

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.

Comment 3

[Nicolas B. Pierron [:nbp] {backlog: 39}]

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

Comment 4

[Jan de Mooij [:jandem]]

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().

Comment 5

[Nicolas B. Pierron [:nbp] {backlog: 39}]

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?

Comment 6

[Jan de Mooij [:jandem]]

(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.

Comment 7

[Nicolas B. Pierron [:nbp] {backlog: 39}]

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.

Comment 8

[Jan de Mooij [:jandem]]

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?

Comment 9

[Nicolas B. Pierron [:nbp] {backlog: 39}]

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.

Comment 10

[Nicolas B. Pierron [:nbp] {backlog: 39}]

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 11

[Al Billings [:abillings]]

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.

Comment 12

[Nicolas B. Pierron [:nbp] {backlog: 39}]

https://hg.mozilla.org/integration/mozilla-inbound/rev/197b182baf4f

Comment 13

[Nicolas B. Pierron [:nbp] {backlog: 39}]

Unapply wrong test case fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0aa8d7cd5b8f

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/197b182baf4f
https://hg.mozilla.org/mozilla-central/rev/0aa8d7cd5b8f

Comment 15

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 19

[Anthony Hughes (:ashughes) [QA]]

Can this be put in testsuite?