Title: [255528] trunk
- Revision
- 255528
- Author
- [email protected]
- Date
- 2020-01-31 16:06:51 -0800 (Fri, 31 Jan 2020)
Log Message
GetGetterSetterByOffset and GetGetter/GetSetter are not always safe to execute
https://bugs.webkit.org/show_bug.cgi?id=206805
<rdar://problem/58898161>
Reviewed by Yusuke Suzuki.
JSTests:
* stress/get-getter-setter-by-offset-not-always-safe-to-execute.js: Added.
Source/_javascript_Core:
This patch fixes two bugs. The first is GetGetterSetterByOffset. Previously,
we were just checking that we could load the value safely. However, because
GetGetterSetterByOffset returns a GetterSetter object, we can only safely
move this node into a context where it's guaranteed that the offset loaded
will return a GetterSetter.
The second fix is GetGetter/GetSetter were both always marked as safe to execute.
However, they're only safe to execute when the incoming value to load from
is a GetterSetter object.
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (255527 => 255528)
--- trunk/JSTests/ChangeLog 2020-01-31 23:56:22 UTC (rev 255527)
+++ trunk/JSTests/ChangeLog 2020-02-01 00:06:51 UTC (rev 255528)
@@ -1,3 +1,13 @@
+2020-01-31 Saam Barati <[email protected]>
+
+ GetGetterSetterByOffset and GetGetter/GetSetter are not always safe to execute
+ https://bugs.webkit.org/show_bug.cgi?id=206805
+ <rdar://problem/58898161>
+
+ Reviewed by Yusuke Suzuki.
+
+ * stress/get-getter-setter-by-offset-not-always-safe-to-execute.js: Added.
+
2020-01-31 Alexey Shvayka <[email protected]>
Unmatched ] or } brackets should be syntax errors in Unicode patterns only
Added: trunk/JSTests/stress/get-getter-setter-by-offset-not-always-safe-to-execute.js (0 => 255528)
--- trunk/JSTests/stress/get-getter-setter-by-offset-not-always-safe-to-execute.js (rev 0)
+++ trunk/JSTests/stress/get-getter-setter-by-offset-not-always-safe-to-execute.js 2020-02-01 00:06:51 UTC (rev 255528)
@@ -0,0 +1,22 @@
+//@ runDefault("--dumpFTLDisassembly=1", "--thresholdForOptimizeSoon=10", "--useConcurrentJIT=0", "--useConcurrentGC=0", "--thresholdForJITAfterWarmUp=10", "--thresholdForOptimizeAfterWarmUp=100", "--thresholdForFTLOptimizeAfterWarmUp=1000")
+
+function main() {
+ function v1(v2) {
+ try {
+ const v3 = v2.__proto__;
+ v2.e = 0x12341234;
+ for (let v9 = 0; v9 < 10; v9++) {
+ v1(v3);
+ }
+ for (let v20 = 0; v20 < 6; v20++) {
+ const v21 = v2.__proto__;
+ const v24 = {a:13.37};
+ const v26 = [v24];
+ }
+ } catch(v29) {}
+ }
+ v1(v1);
+}
+noDFG(main);
+noFTL(main);
+main();
Modified: trunk/Source/_javascript_Core/ChangeLog (255527 => 255528)
--- trunk/Source/_javascript_Core/ChangeLog 2020-01-31 23:56:22 UTC (rev 255527)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-02-01 00:06:51 UTC (rev 255528)
@@ -1,3 +1,24 @@
+2020-01-31 Saam Barati <[email protected]>
+
+ GetGetterSetterByOffset and GetGetter/GetSetter are not always safe to execute
+ https://bugs.webkit.org/show_bug.cgi?id=206805
+ <rdar://problem/58898161>
+
+ Reviewed by Yusuke Suzuki.
+
+ This patch fixes two bugs. The first is GetGetterSetterByOffset. Previously,
+ we were just checking that we could load the value safely. However, because
+ GetGetterSetterByOffset returns a GetterSetter object, we can only safely
+ move this node into a context where it's guaranteed that the offset loaded
+ will return a GetterSetter.
+
+ The second fix is GetGetter/GetSetter were both always marked as safe to execute.
+ However, they're only safe to execute when the incoming value to load from
+ is a GetterSetter object.
+
+ * dfg/DFGSafeToExecute.h:
+ (JSC::DFG::safeToExecute):
+
2020-01-31 Alexey Shvayka <[email protected]>
Unmatched ] or } brackets should be syntax errors in Unicode patterns only
Modified: trunk/Source/_javascript_Core/dfg/DFGSafeToExecute.h (255527 => 255528)
--- trunk/Source/_javascript_Core/dfg/DFGSafeToExecute.h 2020-01-31 23:56:22 UTC (rev 255527)
+++ trunk/Source/_javascript_Core/dfg/DFGSafeToExecute.h 2020-02-01 00:06:51 UTC (rev 255528)
@@ -435,8 +435,6 @@
case Int52Rep:
case BooleanToNumber:
case FiatInt52:
- case GetGetter:
- case GetSetter:
case GetEnumerableLength:
case HasGenericProperty:
case HasStructureProperty:
@@ -519,6 +517,17 @@
return false;
}
+ case GetGetter:
+ case GetSetter: {
+ if (!state.forNode(node->child1()).isType(SpecCell))
+ return false;
+ StructureAbstractValue& value = state.forNode(node->child1()).m_structure;
+ if (value.isInfinite() || value.size() != 1)
+ return false;
+
+ return value[0].get() == graph.m_vm.getterSetterStructure;
+ }
+
case BottomValue:
// If in doubt, assume that this isn't safe to execute, just because we have no way of
// compiling this node.
@@ -570,9 +579,35 @@
return state.forNode(node->child1()).m_structure.isSubsetOf(
RegisteredStructureSet(node->transition()->previous));
+ case GetGetterSetterByOffset: {
+ // If it's an inline property, we need to make sure it's a cell before trusting what the structure set tells us.
+ if (node->child1().node() == node->child2().node() && !state.forNode(node->child2()).isType(SpecCell))
+ return false;
+
+ StorageAccessData& data = ""
+ auto* uid = graph.identifiers()[data.identifierNumber];
+ PropertyOffset desiredOffset = data.offset;
+ StructureAbstractValue& value = state.forNode(node->child2()).m_structure;
+ if (value.isInfinite())
+ return false;
+ for (unsigned i = value.size(); i--;) {
+ Structure* thisStructure = value[i].get();
+ if (thisStructure->isUncacheableDictionary())
+ return false;
+ unsigned attributes = 0;
+ PropertyOffset checkOffset = thisStructure->getConcurrently(uid, attributes);
+ if (checkOffset != desiredOffset || !(attributes & PropertyAttribute::Accessor))
+ return false;
+ }
+ return true;
+ }
+
case GetByOffset:
- case GetGetterSetterByOffset:
case PutByOffset: {
+ // If it's an inline property, we need to make sure it's a cell before trusting what the structure set tells us.
+ if (node->child1().node() == node->child2().node() && !state.forNode(node->child2()).isType(SpecCell))
+ return false;
+
StorageAccessData& data = ""
PropertyOffset offset = data.offset;
// Graph::isSafeToLoad() is all about proofs derived from PropertyConditions. Those don't
@@ -590,6 +625,8 @@
return false;
for (unsigned i = value.size(); i--;) {
Structure* thisStructure = value[i].get();
+ if (thisStructure->isUncacheableDictionary())
+ return false;
if (!thisStructure->isValidOffset(offset))
return false;
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
