Closed Bug 807047 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox16 --- unaffected
firefox17 --- unaffected
firefox18 --- unaffected
firefox19 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: gkw, Assigned: nbp)

References

Details

(4 keywords, Whiteboard: [ion:p1][adv-main19-])

Attachments

(3 files)

Attached file 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
Tabs-less version: function f(code) { eval(code) } f("\ function h({x}) {\ print(x)\ }\ h(/x/);\ ")
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.
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+
[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+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
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.

Attachment

General

Created:
Updated:
Size: