- Revision
- 203802
- Author
- benja...@webkit.org
- Date
- 2016-07-27 16:22:55 -0700 (Wed, 27 Jul 2016)
Log Message
[JSC] Fix a bunch of use-after-free of DFG::Node
https://bugs.webkit.org/show_bug.cgi?id=160228
Patch by Benjamin Poulain <bpoul...@apple.com> on 2016-07-27
Reviewed by Mark Lam.
FTL had a few places where we use a node after it has been
deleted. The dangling pointers come from the SSA liveness information
kept on the basic blocks.
This patch fixes the issues I could find and adds liveness invalidation
to help finding dependencies like these.
* dfg/DFGBasicBlock.h:
(JSC::DFG::BasicBlock::SSAData::invalidate):
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::run):
Constant folding phase was deleting nodes in the loop over basic blocks.
The problem is the deleted nodes can be referenced by other blocks.
When the abstract interpreter was manipulating the abstract values of those
it was doing so on the dead nodes.
* dfg/DFGConstantHoistingPhase.cpp:
Just invalidation. Nothing wrong here since the useless nodes were
kept live while iterating the blocks.
* dfg/DFGGraph.cpp:
(JSC::DFG::Graph::killBlockAndItsContents):
(JSC::DFG::Graph::killUnreachableBlocks):
(JSC::DFG::Graph::invalidateNodeLiveness):
* dfg/DFGGraph.h:
* dfg/DFGPlan.cpp:
(JSC::DFG::Plan::compileInThreadImpl):
We had a lot of use-after-free in LCIM because we were using the stale
live nodes deleted by previous phases.
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (203801 => 203802)
--- trunk/Source/_javascript_Core/ChangeLog 2016-07-27 22:34:02 UTC (rev 203801)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-07-27 23:22:55 UTC (rev 203802)
@@ -1,3 +1,42 @@
+2016-07-27 Benjamin Poulain <bpoul...@apple.com>
+
+ [JSC] Fix a bunch of use-after-free of DFG::Node
+ https://bugs.webkit.org/show_bug.cgi?id=160228
+
+ Reviewed by Mark Lam.
+
+ FTL had a few places where we use a node after it has been
+ deleted. The dangling pointers come from the SSA liveness information
+ kept on the basic blocks.
+
+ This patch fixes the issues I could find and adds liveness invalidation
+ to help finding dependencies like these.
+
+ * dfg/DFGBasicBlock.h:
+ (JSC::DFG::BasicBlock::SSAData::invalidate):
+
+ * dfg/DFGConstantFoldingPhase.cpp:
+ (JSC::DFG::ConstantFoldingPhase::run):
+ Constant folding phase was deleting nodes in the loop over basic blocks.
+ The problem is the deleted nodes can be referenced by other blocks.
+ When the abstract interpreter was manipulating the abstract values of those
+ it was doing so on the dead nodes.
+
+ * dfg/DFGConstantHoistingPhase.cpp:
+ Just invalidation. Nothing wrong here since the useless nodes were
+ kept live while iterating the blocks.
+
+ * dfg/DFGGraph.cpp:
+ (JSC::DFG::Graph::killBlockAndItsContents):
+ (JSC::DFG::Graph::killUnreachableBlocks):
+ (JSC::DFG::Graph::invalidateNodeLiveness):
+
+ * dfg/DFGGraph.h:
+ * dfg/DFGPlan.cpp:
+ (JSC::DFG::Plan::compileInThreadImpl):
+ We had a lot of use-after-free in LCIM because we were using the stale
+ live nodes deleted by previous phases.
+
2016-07-27 Keith Miller <keith_mil...@apple.com>
concatAppendOne should allocate using the indexing type of the array if it cannot merge
Modified: trunk/Source/_javascript_Core/dfg/DFGBasicBlock.h (203801 => 203802)
--- trunk/Source/_javascript_Core/dfg/DFGBasicBlock.h 2016-07-27 22:34:02 UTC (rev 203801)
+++ trunk/Source/_javascript_Core/dfg/DFGBasicBlock.h 2016-07-27 23:22:55 UTC (rev 203802)
@@ -239,6 +239,14 @@
struct SSAData {
WTF_MAKE_FAST_ALLOCATED;
public:
+ void invalidate()
+ {
+ liveAtTail.clear();
+ liveAtHead.clear();
+ valuesAtHead.clear();
+ valuesAtTail.clear();
+ }
+
AvailabilityMap availabilityAtHead;
AvailabilityMap availabilityAtTail;
Modified: trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp (203801 => 203802)
--- trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp 2016-07-27 22:34:02 UTC (rev 203801)
+++ trunk/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp 2016-07-27 23:22:55 UTC (rev 203802)
@@ -71,6 +71,7 @@
// It's now possible to simplify basic blocks by placing an Unreachable terminator right
// after anything that invalidates AI.
bool didClipBlock = false;
+ Vector<Node*> nodesToDelete;
for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
m_state.beginBasicBlock(block);
for (unsigned nodeIndex = 0; nodeIndex < block->size(); ++nodeIndex) {
@@ -83,7 +84,7 @@
if (!m_state.isValid()) {
NodeOrigin origin = block->at(nodeIndex)->origin;
for (unsigned killIndex = nodeIndex; killIndex < block->size(); ++killIndex)
- m_graph.m_allocator.free(block->at(killIndex));
+ nodesToDelete.append(block->at(killIndex));
block->resize(nodeIndex);
block->appendNode(m_graph, SpecNone, Unreachable, origin);
didClipBlock = true;
@@ -96,6 +97,12 @@
if (didClipBlock) {
changed = true;
+
+ m_graph.invalidateNodeLiveness();
+
+ for (Node* node : nodesToDelete)
+ m_graph.m_allocator.free(node);
+
m_graph.invalidateCFG();
m_graph.resetReachability();
m_graph.killUnreachableBlocks();
Modified: trunk/Source/_javascript_Core/dfg/DFGConstantHoistingPhase.cpp (203801 => 203802)
--- trunk/Source/_javascript_Core/dfg/DFGConstantHoistingPhase.cpp 2016-07-27 22:34:02 UTC (rev 203801)
+++ trunk/Source/_javascript_Core/dfg/DFGConstantHoistingPhase.cpp 2016-07-27 23:22:55 UTC (rev 203802)
@@ -128,6 +128,7 @@
}
// And finally free the constants that we removed.
+ m_graph.invalidateNodeLiveness();
for (Node* node : toFree)
m_graph.m_allocator.free(node);
Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.cpp (203801 => 203802)
--- trunk/Source/_javascript_Core/dfg/DFGGraph.cpp 2016-07-27 22:34:02 UTC (rev 203801)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.cpp 2016-07-27 23:22:55 UTC (rev 203802)
@@ -744,6 +744,8 @@
void Graph::killBlockAndItsContents(BasicBlock* block)
{
+ if (auto& ssaData = block->ssa)
+ ssaData->invalidate();
for (unsigned phiIndex = block->phis.size(); phiIndex--;)
m_allocator.free(block->phis[phiIndex]);
for (unsigned nodeIndex = block->size(); nodeIndex--;)
@@ -754,9 +756,8 @@
void Graph::killUnreachableBlocks()
{
- // FIXME: This probably creates dangling references from Upsilons to Phis in SSA.
- // https://bugs.webkit.org/show_bug.cgi?id=159164
-
+ invalidateNodeLiveness();
+
for (BlockIndex blockIndex = 0; blockIndex < numBlocks(); ++blockIndex) {
BasicBlock* block = this->block(blockIndex);
if (!block)
@@ -778,6 +779,15 @@
m_backwardsCFG = nullptr;
}
+void Graph::invalidateNodeLiveness()
+{
+ if (m_form != SSA)
+ return;
+
+ for (BasicBlock* block : blocksInNaturalOrder())
+ block->ssa->invalidate();
+}
+
void Graph::substituteGetLocal(BasicBlock& block, unsigned startIndexInBlock, VariableAccessData* variableAccessData, Node* newGetLocal)
{
for (unsigned indexInBlock = startIndexInBlock; indexInBlock < block.size(); ++indexInBlock) {
Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.h (203801 => 203802)
--- trunk/Source/_javascript_Core/dfg/DFGGraph.h 2016-07-27 22:34:02 UTC (rev 203801)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.h 2016-07-27 23:22:55 UTC (rev 203802)
@@ -533,6 +533,7 @@
void substituteGetLocal(BasicBlock& block, unsigned startIndexInBlock, VariableAccessData* variableAccessData, Node* newGetLocal);
void invalidateCFG();
+ void invalidateNodeLiveness();
void clearFlagsOnAllNodes(NodeFlags);
Modified: trunk/Source/_javascript_Core/dfg/DFGPlan.cpp (203801 => 203802)
--- trunk/Source/_javascript_Core/dfg/DFGPlan.cpp 2016-07-27 22:34:02 UTC (rev 203801)
+++ trunk/Source/_javascript_Core/dfg/DFGPlan.cpp 2016-07-27 23:22:55 UTC (rev 203802)
@@ -428,6 +428,8 @@
// wrong with running LICM earlier, if we wanted to put other CFG transforms above this point.
// Alternatively, we could run loop pre-header creation after SSA conversion - but if we did that
// then we'd need to do some simple SSA fix-up.
+ performLivenessAnalysis(dfg);
+ performCFA(dfg);
performLICM(dfg);
// FIXME: Currently: IntegerRangeOptimization *must* be run after LICM.
