Changeset 245018 in webkit

Timestamp:

May 7, 2019 11:39:27 AM (5 years ago)

Author:

Tadeu Zagallo

Message:

tryCachePutByID should not crash if target offset changes
​https://bugs.webkit.org/show_bug.cgi?id=197311
<rdar://problem/48033612>

Reviewed by Filip Pizlo.

JSTests:

Add a series of tests related tryCachePutByID. Two of these tests used to crash and were fixed
by this patch: cache-put-by-id-different-attributes.js and cache-put-by-id-different-offset.js

• stress/cache-put-by-id-delete-prototype.js: Added.

(A.prototype.set y):
(A):
(B.prototype.set y):
(B):
(C):

• stress/cache-put-by-id-different-proto.js: Added.

(A.prototype.set y):
(A):
(B1):
(B2.prototype.set y):
(B2):
(C):
(D):

• stress/cache-put-by-id-different-attributes.js: Added.

(Foo):
(set x):

• stress/cache-put-by-id-different-offset.js: Added.

(Foo):
(set x):

• stress/cache-put-by-id-insert-prototype.js: Added.

(A.prototype.set y):
(A):
(C):

• stress/cache-put-by-id-poly-proto.js: Added.

(Foo):
(set _):
(createBar.Bar):
(createBar):

Source/JavaScriptCore:

When tryCachePutID is called with a cacheable setter, if the target object where the setter was
found is still in the prototype chain and there's no poly protos in the chain, we use
generateConditionsForPrototypePropertyHit to validate that the target object remains the same.
It checks for the absence of the property in every object in the prototype chain from the base
down to the target object and checks that the property is still present in the target object. It
also bails if there are any uncacheable objects, proxies or dictionary objects in the prototype
chain. However, it does not consider two edge cases:

• It asserts that the property should still be at the same offset in the target object, but this

assertion does not hold if the setter deletes properties of the object and causes the structure
to be flattened after the deletion. Instead of asserting, we just use the updated offset.

• It does not check whether the new slot is also a setter, which leads to a crash in case it's not.

• jit/Repatch.cpp:

(JSC::tryCachePutByID):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/cache-put-by-id-delete-prototype.js (added)
• JSTests/stress/cache-put-by-id-different-__proto__.js (added)
• JSTests/stress/cache-put-by-id-different-attributes.js (added)
• JSTests/stress/cache-put-by-id-different-offset.js (added)
• JSTests/stress/cache-put-by-id-insert-prototype.js (added)
• JSTests/stress/cache-put-by-id-poly-proto.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/jit/Repatch.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-05-07  Tadeu Zagallo  <tzagallo@apple.com>
+
+        tryCachePutByID should not crash if target offset changes
+        https://bugs.webkit.org/show_bug.cgi?id=197311
+        <rdar://problem/48033612>
+
+        Reviewed by Filip Pizlo.
+
+        Add a series of tests related tryCachePutByID. Two of these tests used to crash and were fixed
+        by this patch: `cache-put-by-id-different-attributes.js` and `cache-put-by-id-different-offset.js`
+
+        * stress/cache-put-by-id-delete-prototype.js: Added.
+        (A.prototype.set y):
+        (A):
+        (B.prototype.set y):
+        (B):
+        (C):
+        * stress/cache-put-by-id-different-__proto__.js: Added.
+        (A.prototype.set y):
+        (A):
+        (B1):
+        (B2.prototype.set y):
+        (B2):
+        (C):
+        (D):
+        * stress/cache-put-by-id-different-attributes.js: Added.
+        (Foo):
+        (set x):
+        * stress/cache-put-by-id-different-offset.js: Added.
+        (Foo):
+        (set x):
+        * stress/cache-put-by-id-insert-prototype.js: Added.
+        (A.prototype.set y):
+        (A):
+        (C):
+        * stress/cache-put-by-id-poly-proto.js: Added.
+        (Foo):
+        (set _):
+        (createBar.Bar):
+        (createBar):
+
 2019-05-07  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-05-07  Tadeu Zagallo  <tzagallo@apple.com>
+
+        tryCachePutByID should not crash if target offset changes
+        https://bugs.webkit.org/show_bug.cgi?id=197311
+        <rdar://problem/48033612>
+
+        Reviewed by Filip Pizlo.
+
+        When tryCachePutID is called with a cacheable setter, if the target object where the setter was
+        found is still in the prototype chain and there's no poly protos in the chain, we use
+        generateConditionsForPrototypePropertyHit to validate that the target object remains the same.
+        It checks for the absence of the property in every object in the prototype chain from the base
+        down to the target object and checks that the property is still present in the target object. It
+        also bails if there are any uncacheable objects, proxies or dictionary objects in the prototype
+        chain. However, it does not consider two edge cases:
+        - It asserts that the property should still be at the same offset in the target object, but this
+        assertion does not hold if the setter deletes properties of the object and causes the structure
+        to be flattened after the deletion. Instead of asserting, we just use the updated offset.
+        - It does not check whether the new slot is also a setter, which leads to a crash in case it's not.
+
+        * jit/Repatch.cpp:
+        (JSC::tryCachePutByID):
+
 2019-05-07  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/jit/Repatch.cpp

@@ -577 +577 @@
                             return GiveUpOnCache;
 
-                        PropertyOffset conditionSetOffset = conditionSet.slotBaseCondition().offset();
-                        if (UNLIKELY(offset != conditionSetOffset))
-                            CRASH_WITH_INFO(offset, conditionSetOffset, slot.base()->type(), baseCell->type(), conditionSet.size());
+                        if (!(conditionSet.slotBaseCondition().attributes() & PropertyAttribute::Accessor))
+                            return GiveUpOnCache;
+
+                        offset = conditionSet.slotBaseCondition().offset();
                     }