Detect circular references for library and free functions #10228

Marenz · 2020-11-09T12:02:53Z

Fixes #9565
Fixes #10457
Fixes #11128

libsolidity/ast/ASTAnnotations.h

libsolidity/ast/AST.cpp

test/libsolidity/syntaxTests/freeFunctions/circular_reference.sol

hrkrshnn

Looks good. Some minor comments.

Changelog.md

libsolidity/ast/ASTAnnotations.h

libsolidity/ast/AST.cpp

cameel

Mostly style issues.

And yeah, now I see how it won't work without the call graph. It does not cover transitive dependencies where a function calls a function.

libsolidity/analysis/TypeChecker.cpp

libsolidity/ast/AST.cpp

libsolidity/ast/AST.h

libsolidity/analysis/TypeChecker.h

libsolidity/analysis/PostTypeChecker.cpp

libsolidity/analysis/TypeChecker.cpp

test/libsolidity/syntaxTests/library_function_circular_reference.sol

cameel · 2021-02-23T10:58:59Z

Function call graph (#10973) has been merged so I guess we can now revive this PR. @Marenz, will you be working it do you want someone else to take over?

Marenz · 2021-02-23T12:04:38Z

I can do it, looking forward to using the call graph!

libsolidity/analysis/NameAndTypeResolver.cpp

libsolidity/analysis/PostTypeChecker.cpp

cameel · 2021-03-04T10:58:06Z

Is this actually up for review now? I think @Marenz said yesterday that he's still working on something here so I decided to wait and watch and in the meantime I started reviewing keccak instead :)

leonardoalt · 2021-03-04T11:07:32Z

@cameel ah ok, not sure, I based the requests on Marenz marked this pull request as ready for review 18 hours ago above

Marenz · 2021-03-04T11:51:36Z

Is this actually up for review now?

No, this is ready now

Changelog.md

chriseth · 2021-03-04T16:59:33Z

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());


Do we actually still need this, or are the dependencies from the call graph enough?

If we have

contract A { function f() public { new B(); } } contract B is A {}

this will be caught through the call graph.
A special case that changes is if the base function is not executed when the derived is the "most derived":

contract A { function f() public virtual { new B(); } } contract B is A { function f() public override { } }

But in that case, it is actually possible for A to create B.

contractDependencies looks like it only stores information about the base contracts that the derived contract inherits from. The call graph does not have that info. At least not full info. The creation call graph does have edges from Entry to base constructors but only if these constructors are explicitly defined.

I also see that it contains contracts used in member access expressions. The call graph tracks function calls but completely ignores contracts used in such expressions.

And the annotation is used not only for detecting cycles. For example CompilerStack::compileContract() uses it to determine which contracts to compile.

But I think there are some cases where adding stuff to contractDependencies is now redundant. For example #10228 (comment).

Though maybe not. The call graph does not take into account stuff that is unreachable. For example if there's a creation cycle caused by unused internal functions, it won't be detected.

contract C { function foo() internal { new D(); } } contract D { function foo() internal { new C(); } }

Currently it causes an error:

Error: Circular reference for contract creation (cannot create instance of derived or same contract).

But the graph won't report C and D in createdContracts.

What do you mean by "used in member access expressions"?
The reason CompilerStack::compileContract uses contractDependencies is because new C needs to have C available, and that's the only reason.

I meant that the TypeChecker was adding contracts used in member access expressions to contractDependencies and function call graph does not add them to createdContracts so it could not be used as a replacement.

But well, I see your point now. It should be possible to generalize createdContracts to just store info about dependencies rather than just about new expressions.

I ran a test and if we don't add them here, our tests all still work, EXCEPT the astjson tests who know are missing their contract dependency information for their field contractDependencies. I'll look into the callgraph and check if we can make it do that somehow..

Alright, I misinterpreted a previous error wrongly and it seems this does indeed break tests involving libraries like this one:

library L { function f() internal { new C(); } } contract D { function f() public { L.f(); } } contract C { constructor() { new D(); } }

The problem is that we never detect the new C(); in L.f() and never fill the dependency C for L..

So the call graph does not properly handle internal library functions? I think there should not be a dependency from L to C but rather from D to C, because the code of L.f is included directly in D.

That would also explain the problem I am seeing. I'll see if I can fix my problem with that information..

libsolidity/ast/CallGraph.h

cameel · 2021-03-04T18:05:54Z

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());


contractDependencies looks like it only stores information about the base contracts that the derived contract inherits from. The call graph does not have that info. At least not full info. The creation call graph does have edges from Entry to base constructors but only if these constructors are explicitly defined.

I also see that it contains contracts used in member access expressions. The call graph tracks function calls but completely ignores contracts used in such expressions.

And the annotation is used not only for detecting cycles. For example CompilerStack::compileContract() uses it to determine which contracts to compile.

libsolidity/analysis/NameAndTypeResolver.cpp

cameel · 2021-03-04T18:26:36Z

libsolidity/analysis/PostTypeChecker.cpp

+		);
+
+		for (auto const& [contract, referencee]: joined)
+			if (contract != &_contract)


Is this actually possible? I don't think a contract can create its own instance? Maybe this should be an assert?

Hmm are you sure that's not possible? Why wouldn't it?

Doesn't new C syntax require the bytecode of C to be embedded in the bytecode of the contract that wants to create it? And we're still compiling C.

If I try to make C create its own instance I get and error that says:

Error: Circular reference for contract creation (cannot create instance of derived or same contract).

This is from:

contract C { function foo() public { new C(); } }

cameel · 2021-03-04T20:35:47Z

test/libsolidity/syntaxTests/metaTypes/codeAccessBase.sol

-// TypeError 4224: (165-188): Circular reference for contract code access.
-// TypeError 4224: (271-293): Circular reference for contract code access.
-// TypeError 4224: (381-404): Circular reference for contract code access.
-// TypeError 4224: (491-513): Circular reference for contract code access.
+// TypeError 7813: (165-188): Circular reference found.


I think this test should be refactored so that each function is in a separate contract. Otherwise it's not clear if all of these cases still report an error or just some.

Well, creationBase() and runtimeBase() don't create an error anymore here. However, even when in different contracts, this results in only one error, even though both Test calls would cause one

I am at this point not sure if that's okay

However, even when in different contracts, this results in only one error, even though both Test calls would cause one

I think this might actually be an example of #10228 (comment). I suspect that for both cycles the node added to m_foundCycles is Base and the second one overwrites the first.

I'll investigate that

I reversed the contract <-> referencee now but this contract still only gets one error, no matter in how many contracts it is split... I didn't investigate further yet.

cameel · 2021-03-04T21:02:54Z

test/libsolidity/syntaxTests/nameAndTypeResolution/236_cyclic_binary_dependency.sol

@@ -2,4 +2,6 @@ contract A { function f() public { new B(); } }
 contract B { function f() public { new C(); } }


Some more test ideas:

Multiple creation cycles sharing one node (to test the case I mentioned in one of the comments where the error is reported only for that shared node and not separately for each contract).

Creating the contract with new in its own constructor (I think we only have that for an ordinary function).

A situation where two contracts just reference each other's names without doing anything (i.e. f() public { C; }). Currently this is allowed.

Same but with type(C).

Two contracts calling each other without creating an actual dependency cycle (we might already have a test for this):
contract C { function foo(D _d) public { _d.foo(this); } } contract D { function foo(C _c) public { _c.foo(this); } }
Currently this works.

Same for libraries
library L1 { function foo() internal { L2.foo(); } } library L2 { function foo() internal { L1.foo(); } }
Currently this works.

Creation cycles involving both new and inheritance. For example:

C is D, E is D, E does new C

C does new A, A does new D, D is C.

Creation cycle caused by unused internal functions (which are ignored by the call graph):
contract C { function foo() internal { new D(); } } contract D { function foo() internal { new C(); } }
Currently this is an error.

I am wondering if the libraries case should work. In this new PR it is an error.

C is D, E is D, E does new C — This is currently no error. Should it be?

C does new A, A does new D, D is C. — This does produce an error.

I am wondering if the libraries case should work. In this new PR it is an error.

I think it should because you do not need to compile one first to be able to compile the other. Both libraries have their own copies of both functions compiled into them.

C is D, E is D, E does new C — This is currently no error. Should it be?

Right, I think it's fine that it compiles because if we compile C first, E can use its bytecode just fine.

I'd still add it as a test case but the situation I had in mind is actually this: C is D, E is D, C does new E

C does new A, A does new D, D is C. — This does produce an error.

Great. This is what I would expect.

libsolidity/ast/ASTAnnotations.h

cameel · 2021-03-04T21:07:11Z

libsolidity/ast/ASTAnnotations.h

+	/// 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
+	std::map<ContractDefinition const*, ASTNode const*> contractDependencies;


Can we be more specific than ASTNode? I think it can only be NewExpression or InheritanceSpecifier. Using a variant here might save us some assertions.

libsolidity/ast/ASTJsonConverter.cpp

Marenz · 2021-03-10T17:20:07Z

I pushed an update for this PR that takes care of ~80% of the feedback here. Still a WIP though. Everything marked as resolved should be fine though.

chriseth · 2021-03-10T17:49:20Z

libsolidity/analysis/TypeChecker.cpp

-						"Circular reference for contract code access."
-					);
-			}
+			if (m_currentContract || m_currentFreeFunction)


This has to be moved inside the call graph analysis.

Accessing another contract's code is the same as using new OtherContract.

so adding this to the _memberAccess visitor and removing the part here should be enough?

TypePointer exprType = _memberAccess.expression().annotation().type; ASTString const& memberName = _memberAccess.memberName(); if (auto magicType = dynamic_cast<MagicType const*>(exprType)) if (magicType->kind() == MagicType::Kind::MetaType && ( memberName == "creationCode" || memberName == "runtimeCode" )) { ContractType const& accessedContractType = dynamic_cast<ContractType const&>(*magicType->typeArgument()); m_graph.createdContracts.emplace(&accessedContractType, &_memberAccess); }

@cameel and here we already have the case where it's more than just a new expression..

Well, I'd personally just use a variant. It's not really that much more intrusive than ASTNode * because with it you just trade get<T> for dynamic_cast<T>. But that's just my preference. ASTNode * is still a valid choice so if you think it's better suited here I'm fine that that too. I'll just probably be suggesting extra assertions because of that :)

libsolutil/Common.h

libsolidity/ast/ASTAnnotations.h

chriseth · 2021-03-25T17:03:18Z

libsolidity/ast/ASTForward.cpp

@@ -0,0 +1,42 @@
+/*


it's from the "fix macos" commit which is temporary. will be properly integrated now

libsolidity/ast/ASTForward.h

libsolidity/interface/CompilerStack.cpp

chriseth · 2021-03-25T17:19:58Z

libsolidity/interface/CompilerStack.cpp

+
+		for (ContractDefinition const* contractDefinition: ASTNode::filteredNodes<ContractDefinition>(source->ast->nodes()))
+		{
+			// Ignore libraries as they are tested as part of the


I don't think we can do that, libraries can also be deployed in isolation.

In that case we have to change the function call graph to handle internal functions as well when it's a library, even if they are not called...

I would suggest to do that in an extra PR and leave it like this in this one.

Internal library functions that are not called will be ignored, but there are also public library functions.

The call graph is working perfectly fine. I'll just remove this condition here. Also added some more tests.

chriseth · 2021-03-25T17:21:46Z

libsolidity/interface/CompilerStack.cpp

+			if (!cycle)
+				continue;
+
+			ASTNode const* referencee = contractDefinition->annotation().contractDependencies.at(cycle);


What if we have a cycle of the form
a -> b -> c -> d -> c?
Then the return vaule of the cycleDetector will be c, which is not a dependency of a

This is okay (I also tested your example just now) as the cycle Detector returns only and always the vertex that has a distance of 1 (depth = 1) to the originating vertex

Interesting, this is not how CycleDetector should work. I will double-check, because I thought it works correctly with constants.

Yeah, the code in line 64 of Algorithms.h does that. Ok, let's just leave it like that.

chriseth · 2021-03-25T17:26:42Z

Can you add a semantic test that shows that having a "fake cyclic dependency" through a function in a base contract that is never called actually works?

Marenz · 2021-03-25T17:34:09Z

Can you add a semantic test that shows that having a "fake cyclic dependency" through a function in a base contract that is never called actually works?

You mean an internal function correct?

chriseth · 2021-03-25T17:42:41Z

Yes, we discussed such examples in the context of whether or not the base contract should be automatically added to the dependencies or not.

Marenz · 2021-03-29T09:26:54Z

Added requested test case:

contract C
{
	// Internal uncalled function should not cause an cyclic dep. error
	function foo() internal { new D(); }
}

contract D is C
{
}

chriseth · 2021-03-29T09:30:10Z

I think we should also add a more complicated case:
There is another public function in C that calls foo, but it is overwritten in D by an empty function.

Marenz · 2021-03-29T09:31:15Z

That would still cause an error though if C is not abstract, that is.

chriseth · 2021-03-30T13:41:55Z

Needs rebase.

chriseth · 2021-03-30T15:09:32Z

Changelog.md

@@ -9,6 +9,7 @@ Compiler Features:
 * SMTChecker: Report out of bounds index access for arrays and fixed bytes.
 * Standard JSON: Model checker option ``settings.modelChecker.targets`` also accepts ``outOfBounds``.
 * Yul Optimizer: Added a new step FunctionSpecializer, that specializes a function with its literal arguments.
+ * References Resolver: Detect circular references to other contracts across libraries and free functions


It is not really the references resolver. Cannot think of a better word now. We can just call it "Analysis" if we do not find a better word.

Also please use "code references". References is too general.

Marenz force-pushed the issue-9565 branch 2 times, most recently from 8f39a7e to f696907 Compare November 9, 2020 12:21

chriseth reviewed Nov 9, 2020

View reviewed changes

libsolidity/ast/ASTAnnotations.h Outdated Show resolved Hide resolved

chriseth reviewed Nov 9, 2020

View reviewed changes

libsolidity/ast/AST.cpp Outdated Show resolved Hide resolved

chriseth reviewed Nov 9, 2020

View reviewed changes

test/libsolidity/syntaxTests/freeFunctions/circular_reference.sol Outdated Show resolved Hide resolved

hrkrshnn reviewed Nov 23, 2020

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

libsolidity/ast/ASTAnnotations.h Outdated Show resolved Hide resolved

libsolidity/ast/ASTAnnotations.h Outdated Show resolved Hide resolved

libsolidity/ast/AST.cpp Outdated Show resolved Hide resolved

hrkrshnn marked this pull request as draft November 24, 2020 10:07

cameel reviewed Jan 25, 2021

View reviewed changes

Marenz force-pushed the issue-9565 branch 2 times, most recently from d569f78 to 75e7e77 Compare March 3, 2021 15:54

Marenz marked this pull request as ready for review March 3, 2021 16:38

leonardoalt requested review from cameel and hrkrshnn March 4, 2021 10:52

hrkrshnn reviewed Mar 4, 2021

View reviewed changes

libsolidity/analysis/NameAndTypeResolver.cpp Outdated Show resolved Hide resolved

libsolidity/analysis/PostTypeChecker.cpp Outdated Show resolved Hide resolved

Marenz force-pushed the issue-9565 branch 4 times, most recently from f8bd508 to 1945da9 Compare March 4, 2021 15:25

chriseth reviewed Mar 4, 2021

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

chriseth reviewed Mar 4, 2021

View reviewed changes

cameel reviewed Mar 4, 2021

View reviewed changes

Marenz force-pushed the issue-9565 branch from 1945da9 to 8cbdfb5 Compare March 10, 2021 17:19

chriseth reviewed Mar 10, 2021

View reviewed changes

Marenz force-pushed the issue-9565 branch from 8cbdfb5 to 6014021 Compare March 11, 2021 14:08