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)
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)
7.24 KB,
text/plain
|
Details | |
4.92 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
5.57 KB,
patch
|
nbp
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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•13 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
Assignee | ||
Comment 2•13 years ago
|
||
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
Assignee | ||
Comment 3•13 years ago
|
||
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)
![]() |
||
Updated•13 years ago
|
Whiteboard: [ion:p1]
Comment 4•13 years ago
|
||
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)
Assignee | ||
Comment 5•13 years ago
|
||
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•13 years ago
|
||
(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.
Assignee | ||
Comment 7•13 years ago
|
||
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 8•13 years ago
|
||
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+
Assignee | ||
Comment 9•13 years ago
|
||
[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+
Assignee | ||
Comment 10•13 years ago
|
||
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•13 years ago
|
||
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+
Assignee | ||
Comment 12•13 years ago
|
||
Assignee | ||
Comment 13•13 years ago
|
||
Unapply wrong test case fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0aa8d7cd5b8f
Comment 14•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/197b182baf4f
https://hg.mozilla.org/mozilla-central/rev/0aa8d7cd5b8f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•13 years ago
|
Status: RESOLVED → VERIFIED
Comment 15•13 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•13 years ago
|
Updated•12 years ago
|
Whiteboard: [ion:p1] → [ion:p1][adv-main19-]
Updated•11 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•