From e969aed7803270f412e43107b83f5d5c97c4bb4a Mon Sep 17 00:00:00 2001 From: Marenz Date: Thu, 30 Dec 2021 18:16:29 +0100 Subject: [PATCH] Properly resolve virtual modifiers --- Changelog.md | 1 + libsolidity/analysis/ControlFlowBuilder.cpp | 33 ++++++--- libsolidity/analysis/ControlFlowBuilder.h | 8 +- libsolidity/analysis/ControlFlowGraph.cpp | 2 +- libsolidity/analysis/ControlFlowGraph.h | 4 +- .../analysis/ControlFlowRevertPruner.cpp | 74 +++++++++---------- .../modifier_declaration_fine.sol | 6 +- .../modifier_different_functions.sol | 12 +++ .../modifiers/modifier_override.sol | 17 +++++ .../storageReturn/modifier_err.sol | 8 +- .../storageReturn/modifier_fine.sol | 6 +- 11 files changed, 109 insertions(+), 62 deletions(-) create mode 100644 test/libsolidity/syntaxTests/controlFlow/modifiers/modifier_different_functions.sol create mode 100644 test/libsolidity/syntaxTests/controlFlow/modifiers/modifier_override.sol diff --git a/Changelog.md b/Changelog.md index b88e1a971..7f985b300 100644 --- a/Changelog.md +++ b/Changelog.md @@ -9,6 +9,7 @@ Compiler Features: Bugfixes: + * Control Flow Graph: Perform proper virtual lookup for modifiers for uninitialized variable and unreachable code analysis. Solc-Js: diff --git a/libsolidity/analysis/ControlFlowBuilder.cpp b/libsolidity/analysis/ControlFlowBuilder.cpp index ff8e2a29a..3de1ecca5 100644 --- a/libsolidity/analysis/ControlFlowBuilder.cpp +++ b/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 ControlFlowBuilder::createFunctionFlow( CFG::NodeContainer& _nodeContainer, - FunctionDefinition const& _function + FunctionDefinition const& _function, + ContractDefinition const* _contract ) { auto functionFlow = make_unique(); @@ -45,7 +47,7 @@ unique_ptr 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( _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()) - appendControlFlow(*modifier); + for (auto const& modifierInvocation: _functionDefinition.modifiers()) + appendControlFlow(*modifierInvocation); appendControlFlow(_functionDefinition.body()); diff --git a/libsolidity/analysis/ControlFlowBuilder.h b/libsolidity/analysis/ControlFlowBuilder.h index 9cab99dd4..a150262f1 100644 --- a/libsolidity/analysis/ControlFlowBuilder.h +++ b/libsolidity/analysis/ControlFlowBuilder.h @@ -37,13 +37,15 @@ class ControlFlowBuilder: private ASTConstVisitor, private yul::ASTWalker public: static std::unique_ptr 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. diff --git a/libsolidity/analysis/ControlFlowGraph.cpp b/libsolidity/analysis/ControlFlowGraph.cpp index f4afe0149..ca36b421c 100644 --- a/libsolidity/analysis/ControlFlowGraph.cpp +++ b/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; } diff --git a/libsolidity/analysis/ControlFlowGraph.h b/libsolidity/analysis/ControlFlowGraph.h index 732548664..7383783fd 100644 --- a/libsolidity/analysis/ControlFlowGraph.h +++ b/libsolidity/analysis/ControlFlowGraph.h @@ -98,8 +98,8 @@ struct CFGNode std::vector entries; /// Exit nodes. All CFG nodes to which control flow may continue after this node. std::vector exits; - /// Function calls done by this node - std::vector functionCalls; + /// Function call done by this node + FunctionCall const* functionCall = nullptr; /// Variable occurrences in the node. std::vector variableOccurrences; diff --git a/libsolidity/analysis/ControlFlowRevertPruner.cpp b/libsolidity/analysis/ControlFlowRevertPruner.cpp index 6a8018355..9a44be57a 100644 --- a/libsolidity/analysis/ControlFlowRevertPruner.cpp +++ b/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()) - continue; - - CFG::FunctionContractTuple calledFunctionTuple{ - findScopeContract(*resolvedFunction, item.contract), - resolvedFunction - }; - switch (m_functions.at(calledFunctionTuple)) + if (resolvedFunction && resolvedFunction->isImplemented()) { - case RevertState::Unknown: - wakeUp[calledFunctionTuple].insert(item); - foundUnknown = true; - return; - case RevertState::AllPathsRevert: - return; - case RevertState::HasNonRevertingPath: - break; + CFG::FunctionContractTuple calledFunctionTuple{ + findScopeContract(*resolvedFunction, item.contract), + resolvedFunction + }; + switch (m_functions.at(calledFunctionTuple)) + { + case RevertState::Unknown: + 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{{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()) - continue; + if (resolvedFunction && resolvedFunction->isImplemented()) + 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})) - { - 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); - - _node->exits = {functionFlow.revert}; - functionFlow.revert->entries.push_back(_node); - return; - default: - break; - } + _node->exits = {functionFlow.revert}; + functionFlow.revert->entries.push_back(_node); + return; + default: + break; + } } for (CFGNode* exit: _node->exits) diff --git a/test/libsolidity/syntaxTests/controlFlow/localStorageVariables/modifier_declaration_fine.sol b/test/libsolidity/syntaxTests/controlFlow/localStorageVariables/modifier_declaration_fine.sol index 6df4939cf..260ae6b94 100644 --- a/test/libsolidity/syntaxTests/controlFlow/localStorageVariables/modifier_declaration_fine.sol +++ b/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; } diff --git a/test/libsolidity/syntaxTests/controlFlow/modifiers/modifier_different_functions.sol b/test/libsolidity/syntaxTests/controlFlow/modifiers/modifier_different_functions.sol new file mode 100644 index 000000000..63d915369 --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/controlFlow/modifiers/modifier_override.sol b/test/libsolidity/syntaxTests/controlFlow/modifiers/modifier_override.sol new file mode 100644 index 000000000..3c5eb80ee --- /dev/null +++ b/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. diff --git a/test/libsolidity/syntaxTests/controlFlow/storageReturn/modifier_err.sol b/test/libsolidity/syntaxTests/controlFlow/storageReturn/modifier_err.sol index bf896b86a..5dee487d1 100644 --- a/test/libsolidity/syntaxTests/controlFlow/storageReturn/modifier_err.sol +++ b/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: (367-376): 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: (361-370): This variable is of storage pointer type and can be returned without prior assignment, which would lead to undefined behaviour. diff --git a/test/libsolidity/syntaxTests/controlFlow/storageReturn/modifier_fine.sol b/test/libsolidity/syntaxTests/controlFlow/storageReturn/modifier_fine.sol index ee37f6d64..7b7b3ba0c 100644 --- a/test/libsolidity/syntaxTests/controlFlow/storageReturn/modifier_fine.sol +++ b/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; }