Merge pull request #12471 from ethereum/modifiers-control-flow-11483

Properly generated control flows for inherited modifiers

Changelog.md

@@ -10,6 +10,7 @@ Compiler Features:
 
 
 Bugfixes:
+ * Control Flow Graph: Perform proper virtual lookup for modifiers for uninitialized variable and unreachable code analysis.
 
 
 Solc-Js:

libsolidity/analysis/ControlFlowBuilder.cpp

@@ -25,19 +25,21 @@ using namespace solidity::langutil;
 using namespace solidity::frontend;
 using namespace std;
 
-ControlFlowBuilder::ControlFlowBuilder(CFG::NodeContainer& _nodeContainer, FunctionFlow const& _functionFlow):
+ControlFlowBuilder::ControlFlowBuilder(CFG::NodeContainer& _nodeContainer, FunctionFlow const& _functionFlow, ContractDefinition const* _contract):
 	m_nodeContainer(_nodeContainer),
 	m_currentNode(_functionFlow.entry),
 	m_returnNode(_functionFlow.exit),
 	m_revertNode(_functionFlow.revert),
-	m_transactionReturnNode(_functionFlow.transactionReturn)
+	m_transactionReturnNode(_functionFlow.transactionReturn),
+	m_contract(_contract)
 {
 }
 
 
 unique_ptr<FunctionFlow> ControlFlowBuilder::createFunctionFlow(
 	CFG::NodeContainer& _nodeContainer,
-	FunctionDefinition const& _function
+	FunctionDefinition const& _function,
+	ContractDefinition const* _contract
 )
 {
 	auto functionFlow = make_unique<FunctionFlow>();
@@ -45,7 +47,7 @@ unique_ptr<FunctionFlow> ControlFlowBuilder::createFunctionFlow(
 	functionFlow->exit = _nodeContainer.newNode();
 	functionFlow->revert = _nodeContainer.newNode();
 	functionFlow->transactionReturn = _nodeContainer.newNode();
-	ControlFlowBuilder builder(_nodeContainer, *functionFlow);
+	ControlFlowBuilder builder(_nodeContainer, *functionFlow, _contract);
 	builder.appendControlFlow(_function);
 
 	return functionFlow;
@@ -297,7 +299,8 @@ bool ControlFlowBuilder::visit(FunctionCall const& _functionCall)
 				_functionCall.expression().accept(*this);
 				ASTNode::listAccept(_functionCall.arguments(), *this);
 
-				m_currentNode->functionCalls.emplace_back(&_functionCall);
+				solAssert(!m_currentNode->functionCall);
+				m_currentNode->functionCall = &_functionCall;
 
 				auto nextNode = newLabel();
 
@@ -321,8 +324,20 @@ bool ControlFlowBuilder::visit(ModifierInvocation const& _modifierInvocation)
 	auto modifierDefinition = dynamic_cast<ModifierDefinition const*>(
 		_modifierInvocation.name().annotation().referencedDeclaration
 	);
-	if (!modifierDefinition) return false;
+
-	if (!modifierDefinition->isImplemented()) return false;
+	if (!modifierDefinition)
+		return false;
+
+	VirtualLookup const& requiredLookup = *_modifierInvocation.name().annotation().requiredLookup;
+
+	if (requiredLookup == VirtualLookup::Virtual)
+		modifierDefinition = &modifierDefinition->resolveVirtual(*m_contract);
+	else
+		solAssert(requiredLookup == VirtualLookup::Static);
+
+	if (!modifierDefinition->isImplemented())
+		return false;
+
 	solAssert(!!m_returnNode, "");
 
 	m_placeholderEntry = newLabel();
@@ -355,8 +370,8 @@ bool ControlFlowBuilder::visit(FunctionDefinition const& _functionDefinition)
 
 	}
 
-	for (auto const& modifier: _functionDefinition.modifiers())
+	for (auto const& modifierInvocation: _functionDefinition.modifiers())
-		appendControlFlow(*modifier);
+		appendControlFlow(*modifierInvocation);
 
 	appendControlFlow(_functionDefinition.body());
 

libsolidity/analysis/ControlFlowBuilder.h

@@ -37,13 +37,15 @@ class ControlFlowBuilder: private ASTConstVisitor, private yul::ASTWalker
 public:
 	static std::unique_ptr<FunctionFlow> createFunctionFlow(
 		CFG::NodeContainer& _nodeContainer,
-		FunctionDefinition const& _function
+		FunctionDefinition const& _function,
+		ContractDefinition const* _contract = nullptr
 	);
 
 private:
 	explicit ControlFlowBuilder(
 		CFG::NodeContainer& _nodeContainer,
-		FunctionFlow const& _functionFlow
+		FunctionFlow const& _functionFlow,
+		ContractDefinition const* _contract = nullptr
 	);
 
 	// Visits for constructing the control flow.
@@ -158,6 +160,8 @@ private:
 	CFGNode* m_revertNode = nullptr;
 	CFGNode* m_transactionReturnNode = nullptr;
 
+	ContractDefinition const* m_contract = nullptr;
+
 	/// The current jump destination of break Statements.
 	CFGNode* m_breakJump = nullptr;
 	/// The current jump destination of continue Statements.

libsolidity/analysis/ControlFlowGraph.cpp

@@ -44,7 +44,7 @@ bool CFG::visit(ContractDefinition const& _contract)
 		for (FunctionDefinition const* function: contract->definedFunctions())
 			if (function->isImplemented())
 				m_functionControlFlow[{&_contract, function}] =
-					ControlFlowBuilder::createFunctionFlow(m_nodeContainer, *function);
+					ControlFlowBuilder::createFunctionFlow(m_nodeContainer, *function, &_contract);
 
 	return true;
 }

libsolidity/analysis/ControlFlowGraph.h

@@ -98,8 +98,8 @@ struct CFGNode
 	std::vector<CFGNode*> entries;
 	/// Exit nodes. All CFG nodes to which control flow may continue after this node.
 	std::vector<CFGNode*> exits;
-	/// Function calls done by this node
+	/// Function call done by this node
-	std::vector<FunctionCall const*> functionCalls;
+	FunctionCall const* functionCall = nullptr;
 
 	/// Variable occurrences in the node.
 	std::vector<VariableOccurrence> variableOccurrences;

libsolidity/analysis/ControlFlowRevertPruner.cpp

@@ -81,27 +81,27 @@ void ControlFlowRevertPruner::findRevertStates()
 				if (_node == functionFlow.exit)
 					foundExit = true;
 
-				for (auto const* functionCall: _node->functionCalls)
+				if (auto const* functionCall = _node->functionCall)
 				{
 					auto const* resolvedFunction = ASTNode::resolveFunctionCall(*functionCall, item.contract);
 
-					if (resolvedFunction == nullptr || !resolvedFunction->isImplemented())
+					if (resolvedFunction && resolvedFunction->isImplemented())
-						continue;
-
-					CFG::FunctionContractTuple calledFunctionTuple{
-						findScopeContract(*resolvedFunction, item.contract),
-						resolvedFunction
-					};
-					switch (m_functions.at(calledFunctionTuple))
 					{
-						case RevertState::Unknown:
+						CFG::FunctionContractTuple calledFunctionTuple{
-							wakeUp[calledFunctionTuple].insert(item);
+							findScopeContract(*resolvedFunction, item.contract),
-							foundUnknown = true;
+							resolvedFunction
-							return;
+						};
-						case RevertState::AllPathsRevert:
+						switch (m_functions.at(calledFunctionTuple))
-							return;
+						{
-						case RevertState::HasNonRevertingPath:
+							case RevertState::Unknown:
-							break;
+								wakeUp[calledFunctionTuple].insert(item);
+								foundUnknown = true;
+								return;
+							case RevertState::AllPathsRevert:
+								return;
+							case RevertState::HasNonRevertingPath:
+								break;
+						}
 					}
 				}
 
@@ -135,31 +135,29 @@ void ControlFlowRevertPruner::modifyFunctionFlows()
 		FunctionFlow const& functionFlow = m_cfg.functionFlow(*item.first.function, item.first.contract);
 		solidity::util::BreadthFirstSearch<CFGNode*>{{functionFlow.entry}}.run(
 			[&](CFGNode* _node, auto&& _addChild) {
-				for (auto const* functionCall: _node->functionCalls)
+				if (auto const* functionCall = _node->functionCall)
 				{
 					auto const* resolvedFunction = ASTNode::resolveFunctionCall(*functionCall, item.first.contract);
 
-					if (resolvedFunction == nullptr || !resolvedFunction->isImplemented())
+					if (resolvedFunction && resolvedFunction->isImplemented())
-						continue;
+						switch (m_functions.at({findScopeContract(*resolvedFunction, item.first.contract), resolvedFunction}))
+						{
+							case RevertState::Unknown:
+								[[fallthrough]];
+							case RevertState::AllPathsRevert:
+								// If the revert states of the functions do not
+								// change anymore, we treat all "unknown" states as
+								// "reverting", since they can only be caused by
+								// recursion.
+								for (CFGNode * node: _node->exits)
+									ranges::remove(node->entries, _node);
 
-					switch (m_functions.at({findScopeContract(*resolvedFunction, item.first.contract), resolvedFunction}))
+								_node->exits = {functionFlow.revert};
-					{
+								functionFlow.revert->entries.push_back(_node);
-						case RevertState::Unknown:
+								return;
-							[[fallthrough]];
+							default:
-						case RevertState::AllPathsRevert:
+								break;
-							// If the revert states of the functions do not
+						}
-							// change anymore, we treat all "unknown" states as
-							// "reverting", since they can only be caused by
-							// recursion.
-							for (CFGNode * node: _node->exits)
-								ranges::remove(node->entries, _node);
-
-							_node->exits = {functionFlow.revert};
-							functionFlow.revert->entries.push_back(_node);
-							return;
-						default:
-							break;
-					}
 				}
 
 				for (CFGNode* exit: _node->exits)

test/libsolidity/syntaxTests/controlFlow/localStorageVariables/modifier_declaration_fine.sol

@@ -1,5 +1,5 @@
 contract C {
-    modifier revertIfNoReturn() {
+    modifier alwaysRevert() {
         _;
         revert();
     }
@@ -9,10 +9,10 @@ contract C {
     }
     struct S { uint a; }
     S s;
-    function f(bool flag) revertIfNoReturn() internal view {
+    function f(bool flag) alwaysRevert() internal view {
         if (flag) s;
     }
-    function g(bool flag) revertIfNoReturn() ifFlag(flag) internal view {
+    function g(bool flag) alwaysRevert() ifFlag(flag) internal view {
         s;
     }
 

test/libsolidity/syntaxTests/controlFlow/modifiers/modifier_different_functions.sol

@@ -0,0 +1,12 @@
+contract A {
+	function f() mod internal returns (uint[] storage) {
+		revert();
+	}
+	function g() mod internal returns (uint[] storage) {
+	}
+	modifier mod() virtual {
+		_;
+	}
+}
+// ----
+// TypeError 3464: (118-132): This variable is of storage pointer type and can be returned without prior assignment, which would lead to undefined behaviour.

test/libsolidity/syntaxTests/controlFlow/modifiers/modifier_override.sol

@@ -0,0 +1,17 @@
+contract A {
+	function f() mod internal returns (uint[] storage) {
+	}
+	modifier mod() virtual {
+		revert();
+		_;
+	}
+}
+contract B is A {
+	modifier mod() override { _; }
+	function g() public {
+		f()[0] = 42;
+	}
+}
+// ----
+// Warning 5740: (65-69): Unreachable code.
+// TypeError 3464: (49-63): This variable is of storage pointer type and can be returned without prior assignment, which would lead to undefined behaviour.

test/libsolidity/syntaxTests/controlFlow/storageReturn/modifier_err.sol

@@ -1,5 +1,5 @@
 contract C {
-    modifier revertIfNoReturn() {
+    modifier callAndRevert() {
         _;
         revert();
     }
@@ -13,10 +13,10 @@ contract C {
         return s;
     }
 
-    function g(bool flag) ifFlag(flag) revertIfNoReturn() internal view returns(S storage) {
+    function g(bool flag) ifFlag(flag) callAndRevert() internal view returns(S storage) {
         return s;
     }
 }
 // ----
-// TypeError 3464: (249-258): This variable is of storage pointer type and can be returned without prior assignment, which would lead to undefined behaviour.
+// TypeError 3464: (246-255): This variable is of storage pointer type and can be returned without prior assignment, which would lead to undefined behaviour.
-// TypeError 3464: (367-376): This variable is of storage pointer type and can be returned without prior assignment, which would lead to undefined behaviour.
+// TypeError 3464: (361-370): This variable is of storage pointer type and can be returned without prior assignment, which would lead to undefined behaviour.

test/libsolidity/syntaxTests/controlFlow/storageReturn/modifier_fine.sol

@@ -1,5 +1,5 @@
 contract C {
-    modifier revertIfNoReturn() {
+    modifier callAndRevert() {
         _;
         revert();
     }
@@ -9,10 +9,10 @@ contract C {
     }
     struct S { uint a; }
     S s;
-    function f(bool flag) revertIfNoReturn() internal view returns(S storage) {
+    function f(bool flag) callAndRevert() internal view returns(S storage) {
         if (flag) return s;
     }
-    function g(bool flag) revertIfNoReturn() ifFlag(flag) internal view returns(S storage) {
+    function g(bool flag) callAndRevert() ifFlag(flag) internal view returns(S storage) {
         return s;
     }