Detect circular references for library and free functions

Author: Marenz

Changelog.md

@@ -6,6 +6,7 @@ Language Features:
 Compiler Features:
  * Command Line Interface: Drop experimental support for ``--machine evm15``.
  * Optimizer: Try to move ``and`` with constant inside ``or`` to improve storage writes of small types.
+ * References Resolver: Detect circular references to other contracts across libraries and free functions
 
 
 Bugfixes:

libsolidity/analysis/FunctionCallGraph.cpp

@@ -212,7 +212,7 @@ bool FunctionCallGraphBuilder::visit(ModifierInvocation const& _modifierInvocati
 bool FunctionCallGraphBuilder::visit(NewExpression const& _newExpression)
 {
 	if (ContractType const* contractType = dynamic_cast<ContractType const*>(_newExpression.typeName().annotation().type))
-		m_graph.createdContracts.emplace(&contractType->contractDefinition());
+		m_graph.createdContracts.emplace(&contractType->contractDefinition(), &_newExpression);
 
 	return true;
 }

libsolidity/analysis/NameAndTypeResolver.cpp

@@ -379,17 +379,21 @@ void NameAndTypeResolver::linearizeBaseContracts(ContractDefinition& _contract)
 		// "push_front" has the effect that bases mentioned later can overwrite members of bases
 		// mentioned earlier
 		input.back().push_front(base);
+		_contract.annotation().contractDependencies.emplace(base, baseSpecifier.get());
+
 		vector<ContractDefinition const*> const& basesBases = base->annotation().linearizedBaseContracts;
 		if (basesBases.empty())
 			m_errorReporter.fatalTypeError(2449_error, baseName.location(), "Definition of base has to precede definition of derived contract");
 		input.push_front(list<ContractDefinition const*>(basesBases.begin(), basesBases.end()));
+
+		auto const& basesDependencies = base->annotation().contractDependencies;
+		_contract.annotation().contractDependencies.insert(basesDependencies.begin(), basesDependencies.end());
 	}
 	input.back().push_front(&_contract);
 	vector<ContractDefinition const*> result = cThreeMerge(input);
 	if (result.empty())
 		m_errorReporter.fatalTypeError(5005_error, _contract.location(), "Linearization of inheritance graph impossible");
 	_contract.annotation().linearizedBaseContracts = result;
-	_contract.annotation().contractDependencies.insert(result.begin() + 1, result.end());
 }
 
 template <class T>

libsolidity/analysis/PostTypeChecker.cpp

@@ -19,13 +19,15 @@
 #include <libsolidity/analysis/PostTypeChecker.h>
 
 #include <libsolidity/ast/AST.h>
+#include <libsolidity/ast/CallGraph.h>
 #include <libsolidity/interface/Version.h>
 #include <liblangutil/ErrorReporter.h>
 #include <liblangutil/SemVerHandler.h>
 #include <libsolutil/Algorithms.h>
 
 #include <boost/range/adaptor/map.hpp>
 #include <memory>
+#include <set>
 
 using namespace std;
 using namespace solidity;
@@ -362,6 +364,58 @@ struct NoVariablesInInterfaceChecker: public PostTypeChecker::Checker
 	/// Flag indicating whether we are currently inside a StructDefinition.
 	int m_insideStruct = 0;
 };
+
+struct CircularContractDependencies: public PostTypeChecker::Checker
+{
+	CircularContractDependencies(ErrorReporter& _errorReporter):
+		Checker(_errorReporter),
+		m_cycleDetector(m_cycleVisitor)
+	{}
+
+	void endVisit(ContractDefinition const& _contract) override
+	{
+		ContractDefinition const* cycleContract = m_cycleDetector.run(_contract);
+
+		if (cycleContract)
+			m_foundCycles.emplace(cycleContract, _contract.annotation().contractDependencies[cycleContract]);
+	}
+
+	void finalize() override
+	{
+		for (auto const& [cycle, referencee]: m_foundCycles)
+		{
+			SecondarySourceLocation secondaryLocation{};
+			secondaryLocation.append("Referenced contract"s, cycle->location());
+
+			m_errorReporter.typeError(
+				7813_error,
+				referencee->location(),
+				secondaryLocation,
+				"Circular reference found."
+			);
+		}
+
+		m_foundCycles.clear();
+		m_cycleDetector.reset();
+	}
+
+private:
+	/// Visitor for the cycle detector
+	util::CycleDetector<ContractDefinition>::Visitor const m_cycleVisitor = [&](ContractDefinition const& _contract, util::CycleDetector<ContractDefinition>& _cycleDetector, size_t _depth)
+	{
+		if (_depth >= 256)
+			m_errorReporter.fatalTypeError(7864_error, _contract.location(), "Contract dependencies exhausting cyclic dependency validator");
+
+		for (auto& [dependencyContract, referencee]: _contract.annotation().contractDependencies)
+			if (_cycleDetector.run(*dependencyContract))
+				return;
+	};
+
+	/// Cycles we found, used to avoid duplicate reports for the same dependency
+	map<ContractDefinition const*, ASTNode const*, ASTNode::CompareByID> m_foundCycles;
+	/// Cycle detector is a class member as it should remember already visited contracts
+	util::CycleDetector<ContractDefinition> m_cycleDetector;
+};
 }
 
 PostTypeChecker::PostTypeChecker(langutil::ErrorReporter& _errorReporter): m_errorReporter(_errorReporter)
@@ -371,4 +425,5 @@ PostTypeChecker::PostTypeChecker(langutil::ErrorReporter& _errorReporter): m_err
 	m_checkers.push_back(make_shared<ModifierContextChecker>(_errorReporter));
 	m_checkers.push_back(make_shared<EventOutsideEmitChecker>(_errorReporter));
 	m_checkers.push_back(make_shared<NoVariablesInInterfaceChecker>(_errorReporter));
+	m_checkers.push_back(make_shared<CircularContractDependencies>(_errorReporter));
 }

libsolidity/analysis/TypeChecker.cpp

@@ -329,6 +329,12 @@ void TypeChecker::endVisit(ModifierDefinition const& _modifier)
 
 bool TypeChecker::visit(FunctionDefinition const& _function)
 {
+	if (_function.isFree())
+	{
+		solAssert(m_currentFreeFunction == nullptr, "");
+		m_currentFreeFunction = &_function;
+	}
+
 	if (_function.markedVirtual())
 	{
 		if (_function.isFree())
@@ -496,6 +502,15 @@ bool TypeChecker::visit(FunctionDefinition const& _function)
 	return false;
 }
 
+void TypeChecker::endVisit(FunctionDefinition const& _function)
+{
+	if (_function.isFree())
+	{
+		solAssert(m_currentFreeFunction == &_function, "");
+		m_currentFreeFunction = nullptr;
+	}
+}
+
 bool TypeChecker::visit(VariableDeclaration const& _variable)
 {
 	_variable.typeName().accept(*this);
@@ -2627,22 +2642,14 @@ void TypeChecker::endVisit(NewExpression const& _newExpression)
 		if (contract->abstract())
 			m_errorReporter.typeError(4614_error, _newExpression.location(), "Cannot instantiate an abstract contract.");
 
-		if (m_currentContract)
+		if (m_currentContract || m_currentFreeFunction)
 		{
-			// TODO this is not properly detecting creation-cycles if they go through
-			// internal library functions or free functions. It will be caught at
-			// code generation time, but it would of course be better to catch it here.
-			m_currentContract->annotation().contractDependencies.insert(contract);
 			solAssert(
 				!contract->annotation().linearizedBaseContracts.empty(),
 				"Linearized base contracts not yet available."
 			);
-			if (contractDependenciesAreCyclic(*m_currentContract))
-				m_errorReporter.typeError(
-					4579_error,
-					_newExpression.location(),
-					"Circular reference for contract creation (cannot create instance of derived or same contract)."
-				);
+
+			currentDependencyAnnotation()->contractDependencies.emplace(contract, &_newExpression);
 		}
 
 		_newExpression.annotation().type = FunctionType::newExpressionType(*contract);
@@ -2918,21 +2925,9 @@ bool TypeChecker::visit(MemberAccess const& _memberAccess)
 					_memberAccess.location(),
 					"\"runtimeCode\" is not available for contracts containing immutable variables."
 				);
-			if (m_currentContract)
-			{
-				// TODO in the same way as with ``new``,
-				// this is not properly detecting creation-cycles if they go through
-				// internal library functions or free functions. It will be caught at
-				// code generation time, but it would of course be better to catch it here.
 
-				m_currentContract->annotation().contractDependencies.insert(&accessedContractType.contractDefinition());
-				if (contractDependenciesAreCyclic(*m_currentContract))
-					m_errorReporter.typeError(
-						4224_error,
-						_memberAccess.location(),
-						"Circular reference for contract code access."
-					);
-			}
+			if (m_currentContract || m_currentFreeFunction)
+				currentDependencyAnnotation()->contractDependencies.emplace(&accessedContractType.contractDefinition(), &_memberAccess);
 		}
 		else if (magicType->kind() == MagicType::Kind::MetaType && memberName == "name")
 			annotation.isPure = true;
@@ -2950,6 +2945,11 @@ bool TypeChecker::visit(MemberAccess const& _memberAccess)
 				"\"chainid\" is not supported by the VM version."
 			);
 	}
+	else if (m_currentContract || m_currentFreeFunction)
+		if (auto const* typeType = dynamic_cast<TypeType const*>(exprType))
+			if (auto const* contractType = dynamic_cast<ContractType const*>(typeType->actualType()))
+				if (&contractType->contractDefinition() != m_currentContract)
+					currentDependencyAnnotation()->contractDependencies.emplace(&contractType->contractDefinition(), &_memberAccess);
 
 	if (
 		_memberAccess.expression().annotation().type->category() == Type::Category::Address &&
@@ -3394,22 +3394,6 @@ void TypeChecker::endVisit(UsingForDirective const& _usingFor)
 		);
 }
 
-bool TypeChecker::contractDependenciesAreCyclic(
-	ContractDefinition const& _contract,
-	std::set<ContractDefinition const*> const& _seenContracts
-) const
-{
-	// Naive depth-first search that remembers nodes already seen.
-	if (_seenContracts.count(&_contract))
-		return true;
-	set<ContractDefinition const*> seen(_seenContracts);
-	seen.insert(&_contract);
-	for (auto const* c: _contract.annotation().contractDependencies)
-		if (contractDependenciesAreCyclic(*c, seen))
-			return true;
-	return false;
-}
-
 Declaration const& TypeChecker::dereference(Identifier const& _identifier) const
 {
 	solAssert(!!_identifier.annotation().referencedDeclaration, "Declaration not stored.");

libsolidity/analysis/TypeChecker.h

@@ -114,6 +114,7 @@ class TypeChecker: private ASTConstVisitor
 	void endVisit(InheritanceSpecifier const& _inheritance) override;
 	void endVisit(ModifierDefinition const& _modifier) override;
 	bool visit(FunctionDefinition const& _function) override;
+	void endVisit(FunctionDefinition const& _function) override;
 	bool visit(VariableDeclaration const& _variable) override;
 	/// We need to do this manually because we want to pass the bases of the current contract in
 	/// case this is a base constructor call.
@@ -147,11 +148,6 @@ class TypeChecker: private ASTConstVisitor
 	void endVisit(Literal const& _literal) override;
 	void endVisit(UsingForDirective const& _usingForDirective) override;
 
-	bool contractDependenciesAreCyclic(
-		ContractDefinition const& _contract,
-		std::set<ContractDefinition const*> const& _seenContracts = std::set<ContractDefinition const*>()
-	) const;
-
 	/// @returns the referenced declaration and throws on error.
 	Declaration const& dereference(Identifier const& _identifier) const;
 	/// @returns the referenced declaration and throws on error.
@@ -180,8 +176,21 @@ class TypeChecker: private ASTConstVisitor
 			return m_currentSourceUnit;
 	}
 
+	SharedContractDependenciesAnnotation* currentDependencyAnnotation()
+	{
+		solAssert(!m_currentFreeFunction || !m_currentContract, "Both variables are set?!");
+
+		if (m_currentFreeFunction)
+			return &m_currentFreeFunction->annotation();
+		else if (m_currentContract)
+			return &m_currentContract->annotation();
+
+		return nullptr;
+	}
+
 	SourceUnit const* m_currentSourceUnit = nullptr;
 	ContractDefinition const* m_currentContract = nullptr;
+	FunctionDefinition const* m_currentFreeFunction = nullptr;
 
 	langutil::EVMVersion m_evmVersion;
 

libsolidity/ast/ASTAnnotations.h

@@ -152,16 +152,22 @@ struct StructDeclarationAnnotation: TypeDeclarationAnnotation
 	std::optional<bool> containsNestedMapping;
 };
 
-struct ContractDefinitionAnnotation: TypeDeclarationAnnotation, StructurallyDocumentedAnnotation
+struct SharedContractDependenciesAnnotation
+{
+	/// List of contracts that this contract or function creates, i.e. contracts which need to be compiled first.
+	/// For ContractDefinitions: Also includes all contracts from linearizedBaseContracts.
+	/// Only relevant for contracts or free functions
+	/// The Value represents the ast node that referenced the contract
+	std::map<ContractDefinition const*, ASTNode const*> contractDependencies;
+};
+
+struct ContractDefinitionAnnotation: TypeDeclarationAnnotation, StructurallyDocumentedAnnotation, SharedContractDependenciesAnnotation
 {
 	/// List of functions and modifiers without a body. Can also contain functions from base classes.
 	std::optional<std::vector<Declaration const*>> unimplementedDeclarations;
 	/// List of all (direct and indirect) base contracts in order from derived to
 	/// base, including the contract itself.
 	std::vector<ContractDefinition const*> linearizedBaseContracts;
-	/// List of contracts this contract creates, i.e. which need to be compiled first.
-	/// Also includes all contracts from @a linearizedBaseContracts.
-	std::set<ContractDefinition const*> contractDependencies;
 	/// Mapping containing the nodes that define the arguments for base constructors.
 	/// These can either be inheritance specifiers or modifier invocations.
 	std::map<FunctionDefinition const*, ASTNode const*> baseConstructorArguments;
@@ -177,7 +183,7 @@ struct CallableDeclarationAnnotation: DeclarationAnnotation
 	std::set<CallableDeclaration const*> baseFunctions;
 };
 
-struct FunctionDefinitionAnnotation: CallableDeclarationAnnotation, StructurallyDocumentedAnnotation
+struct FunctionDefinitionAnnotation: CallableDeclarationAnnotation, StructurallyDocumentedAnnotation, SharedContractDependenciesAnnotation
 {
 };
 

libsolidity/ast/ASTJsonConverter.cpp

@@ -35,13 +35,16 @@
 
 #include <boost/algorithm/string/join.hpp>
 #include <boost/range/algorithm/sort.hpp>
+#include <boost/range/adaptors.hpp>
 
 #include <utility>
 #include <vector>
 #include <algorithm>
 #include <limits>
 #include <type_traits>
+#include <range/v3/view/map.hpp>
 
+using namespace ranges;
 using namespace std;
 using namespace solidity::langutil;
 
@@ -269,7 +272,10 @@ bool ASTJsonConverter::visit(ContractDefinition const& _node)
 		make_pair("contractKind", contractKind(_node.contractKind())),
 		make_pair("abstract", _node.abstract()),
 		make_pair("baseContracts", toJson(_node.baseContracts())),
-		make_pair("contractDependencies", getContainerIds(_node.annotation().contractDependencies, true)),
+		make_pair("contractDependencies", getContainerIds(
+			_node.annotation().contractDependencies | views::keys,
+			true
+		)),
 		make_pair("nodes", toJson(_node.subNodes())),
 		make_pair("scope", idOrNull(_node.scope()))
 	};

libsolidity/ast/CallGraph.h

@@ -62,7 +62,7 @@ struct CallGraph
 	std::map<Node, std::set<Node, CompareByID>, CompareByID> edges;
 
 	/// Contracts that may get created with `new` by functions present in the graph.
-	std::set<ContractDefinition const*, ASTNode::CompareByID> createdContracts;
+	std::map<ContractDefinition const*, NewExpression const*, ASTNode::CompareByID> createdContracts;
 
 	/// Events that may get emitted by functions present in the graph.
 	std::set<EventDefinition const*, ASTNode::CompareByID> emittedEvents;