bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Various Baseline IC stubs guard on post-operation shapes even when the operation changed the shape

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla36
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

For example, DoSetPropFallback does the set first, then outputs stubs in the scripted/jsnative setter case that use the then-current shape of the object in the shape guard.  So if the setter mutates the shape, we're guarding on the shape we have _after_ the set, not the one we have before.
Actually, this may not be a security issue.  Just a case of generating IC stubs that we know will never match, if the setter evolved the shape of obj.

We'll also never match if the shape of holder changes during the setter, but that's likely to be rare, and also harder to detect (e.g. have to do a full property lookup before we perform the set).
Created attachment 8506588 [details] [diff] [review]
Don't generate a getter or setter baseline IC stub if we know up front it won't match the sort of object we just did a get or set on
Attachment #8506588 - Flags: review?(efaustbmo)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
I don't think there's a security issue here.
Group: core-security
Summary: Various Ion IC stubs guard on post-operation shapes when they should guard on pre-operation shapes → Various Baseline IC stubs guard on post-operation shapes even when the operation changed the shape

Comment 4

4 years ago
Comment on attachment 8506588 [details] [diff] [review]
Don't generate a getter or setter baseline IC stub if we know up front it won't match the sort of object we just did a get or set on

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

Hmmm, we certainly don't want to be doing what we're doing, because it's a correctness hole. So r=me for this to land immediately.

That being said, we could also just generate cache entries with the post-change shape, right? Can we file a followup of at least "investigate the performance implications of..." rigor, please? In general, we hope that there are lots of "similar objects" at the sites, right? For this reason, Ion even has "add property ICs" which cache the new shape at a property add site and reshape the object in jitcode. If that's useful, maybe the pre-shape thing will be as well?
Attachment #8506588 - Flags: review?(efaustbmo) → review+
I don't think there's a correctness hole here.  We generate the IC entry with the post-change shape but also the post-shape getter/setter.  So it'll behave correctly.  All this patch is doing is trying to make the perf a _bit_ better by not generating ICs we're pretty sure will miss.

You're right that generating based on pre-change shape would be even better; that requires grabbing more info before we do the change (things like getters and whatnot).  Or doing the IC before, but baseline also has an add-property IC, and _that_ needs to get generated after the set.  I filed bug 1089050 on this.
https://hg.mozilla.org/mozilla-central/rev/e4cb0111a14b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.