From 85d17c6f602aefbc903621c13040c11e84143ddf Mon Sep 17 00:00:00 2001 From: David Bar-On Date: Tue, 27 Jun 2023 10:39:36 +0300 Subject: [PATCH] Fix #11889 - private functions should not be overridden by other contracts (rebase) --- libsolidity/analysis/OverrideChecker.cpp | 2 +- libsolidity/analysis/TypeChecker.cpp | 1 + libsolidity/ast/ASTAnnotations.h | 3 +++ libsolidity/codegen/ExpressionCompiler.cpp | 11 +++++++++++ .../codegen/ir/IRGeneratorForStatements.cpp | 10 ++++++++++ .../private_function_vs_free_function.sol | 16 ++++++++++++++++ .../private_function_vs_inheritance.sol | 18 ++++++++++++++++++ .../private_function_vs_internal_contract.sol | 13 +++++++++++++ .../private_function_vs_library.sol | 18 ++++++++++++++++++ .../private_function_vs_overloading.sol | 18 ++++++++++++++++++ .../overloaded_private_function.sol | 13 +++++++++++++ ...ent_private_function_by_public_variable.sol | 2 +- .../override/override_private_function.sol | 9 +++++++++ .../inheritance/override/virtual_private.sol | 1 + .../shadowing_private_base_function.sol | 11 +++++++++++ .../double_private_function_declaration.sol | 7 +++++++ 16 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 test/libsolidity/semanticTests/functionScopting/private_function_vs_free_function.sol create mode 100644 test/libsolidity/semanticTests/functionScopting/private_function_vs_inheritance.sol create mode 100644 test/libsolidity/semanticTests/functionScopting/private_function_vs_internal_contract.sol create mode 100644 test/libsolidity/semanticTests/functionScopting/private_function_vs_library.sol create mode 100644 test/libsolidity/semanticTests/functionScopting/private_function_vs_overloading.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/overloaded_private_function.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/override/override_private_function.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/shadowing_private_base_function.sol create mode 100644 test/libsolidity/syntaxTests/scoping/double_private_function_declaration.sol diff --git a/libsolidity/analysis/OverrideChecker.cpp b/libsolidity/analysis/OverrideChecker.cpp index d342ab100..7d6456629 100644 --- a/libsolidity/analysis/OverrideChecker.cpp +++ b/libsolidity/analysis/OverrideChecker.cpp @@ -933,7 +933,7 @@ OverrideChecker::OverrideProxyBySignatureMultiSet const& OverrideChecker::inheri { set functionsInBase; for (FunctionDefinition const* fun: base->definedFunctions()) - if (!fun->isConstructor()) + if (!fun->isConstructor() && fun->visibility() != Visibility::Private) functionsInBase.emplace(OverrideProxy{fun}); for (VariableDeclaration const* var: base->stateVariables()) if (var->isPublic()) diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index e7721fd9e..3e3f7c270 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -2791,6 +2791,7 @@ bool TypeChecker::visit(FunctionCall const& _functionCall) case Type::Category::Function: functionType = dynamic_cast(expressionType); funcCallAnno.kind = FunctionCallKind::FunctionCall; + funcCallAnno.origCallingContract = dynamic_cast(currentDefinitionScope()); if (auto memberAccess = dynamic_cast(&_functionCall.expression())) { diff --git a/libsolidity/ast/ASTAnnotations.h b/libsolidity/ast/ASTAnnotations.h index 2573015c2..eed7b9dd3 100644 --- a/libsolidity/ast/ASTAnnotations.h +++ b/libsolidity/ast/ASTAnnotations.h @@ -339,6 +339,9 @@ struct FunctionCallAnnotation: ExpressionAnnotation util::SetOnce kind; /// If true, this is the external call of a try statement. bool tryCall = false; + /// The original contract calling the function - actual calling contract may be different, + /// e.g. in case is inlining Internal library function call. + ContractDefinition const* origCallingContract = nullptr; }; } diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index 25bd15d5a..968513d66 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -696,6 +696,17 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) // Calling convention: Caller pushes return address and arguments // Callee removes them and pushes return values + FunctionDefinition const* functionDef = ASTNode::resolveFunctionCall(_functionCall, &m_context.mostDerivedContract()); + if (functionDef && !functionDef->isVisibleInDerivedContracts()) + { + auto origCallingContract = _functionCall.annotation().origCallingContract; + solAssert (origCallingContract, "Original calling contract is not set"); + solAssert( + functionDef->annotation().contract == origCallingContract, + "Attempt to call private function from other contract." + ); + } + evmasm::AssemblyItem returnLabel = m_context.pushNewTag(); for (unsigned i = 0; i < arguments.size(); ++i) acceptAndConvert(*arguments[i], *function.parameterTypes()[i]); diff --git a/libsolidity/codegen/ir/IRGeneratorForStatements.cpp b/libsolidity/codegen/ir/IRGeneratorForStatements.cpp index 509cc28c7..e96b8bc0f 100644 --- a/libsolidity/codegen/ir/IRGeneratorForStatements.cpp +++ b/libsolidity/codegen/ir/IRGeneratorForStatements.cpp @@ -1012,6 +1012,16 @@ void IRGeneratorForStatements::endVisit(FunctionCall const& _functionCall) { solAssert(functionDef->isImplemented()); + if (!functionDef->isVisibleInDerivedContracts()) + { + auto origCallingContract = _functionCall.annotation().origCallingContract; + solAssert (origCallingContract, "Original calling contract is not set"); + solAssert( + functionDef->annotation().contract == origCallingContract, + "Attempt to call private function from other contract" + ); + } + define(_functionCall) << m_context.enqueueFunctionForCodeGeneration(*functionDef) << "(" << diff --git a/test/libsolidity/semanticTests/functionScopting/private_function_vs_free_function.sol b/test/libsolidity/semanticTests/functionScopting/private_function_vs_free_function.sol new file mode 100644 index 000000000..681d5d628 --- /dev/null +++ b/test/libsolidity/semanticTests/functionScopting/private_function_vs_free_function.sol @@ -0,0 +1,16 @@ +// Tests that private contract function does not shadow a free function, +// but that internal contract function does. +function foo(uint256 value) pure returns (uint256) { return 1; } +function bar(uint256 value) pure returns (uint256) { return 2; } +contract A { + function foo(uint256 value) private pure returns (uint256) { return 3; } + function bar(uint256 value) internal pure returns (uint256) { return 4; } +} +contract B is A { + using {foo} for uint256; + function test1(uint256 value) public pure returns (uint256) { return value.foo(); } + function test2(uint256 value) public pure returns (uint256) { return bar(value); } +} +// ---- +// test1(uint256): 0 -> 1 +// test2(uint256): 0 -> 4 diff --git a/test/libsolidity/semanticTests/functionScopting/private_function_vs_inheritance.sol b/test/libsolidity/semanticTests/functionScopting/private_function_vs_inheritance.sol new file mode 100644 index 000000000..f519481fe --- /dev/null +++ b/test/libsolidity/semanticTests/functionScopting/private_function_vs_inheritance.sol @@ -0,0 +1,18 @@ +// Tests that private contract functions are not overridden or visible by inheriting contracts, +// and that that functions functions do. +contract A { + function foo() private pure returns (uint256) { return 1; } + function bar() internal pure returns (uint256) { return foo(); } +} +contract B is A { + function foo() private pure returns (uint256) { return 2; } + function test1() public pure returns (uint256) { return bar(); } + function test2() public pure returns (uint256) { return foo(); } +} +contract C is B { + function foo() public pure returns (uint256) { return 3; } +} +// ---- +// test1() -> 1 +// test2() -> 2 +// foo() -> 3 diff --git a/test/libsolidity/semanticTests/functionScopting/private_function_vs_internal_contract.sol b/test/libsolidity/semanticTests/functionScopting/private_function_vs_internal_contract.sol new file mode 100644 index 000000000..e5bcba64a --- /dev/null +++ b/test/libsolidity/semanticTests/functionScopting/private_function_vs_internal_contract.sol @@ -0,0 +1,13 @@ +// Tests that private contract function is not inherited, but that internal function does. +contract A { + function f() private pure returns (uint) { return 7; } + function g() internal pure returns (uint) { return f(); } +} +contract B is A { + function f() private pure returns (uint) { return 42; } + function test() external pure returns (uint, uint) { + return (g(), f()); + } +} +// ---- +// test() -> 7, 0x2a diff --git a/test/libsolidity/semanticTests/functionScopting/private_function_vs_library.sol b/test/libsolidity/semanticTests/functionScopting/private_function_vs_library.sol new file mode 100644 index 000000000..796d4f2d8 --- /dev/null +++ b/test/libsolidity/semanticTests/functionScopting/private_function_vs_library.sol @@ -0,0 +1,18 @@ +// Tests that private library functions are not overridden or visible contracts, +// and that that internal library functions do. +library L { + function foo(uint256 value) private pure returns (uint256) { return 1; } + function bar(uint256 value) internal pure returns (uint256) { return foo(value); } +} +contract A { + function foo(uint256 value) private pure returns (uint256) { return 2; } +} +contract B is A { + using L for uint256; + function foo(uint256 value) private pure returns (uint256) { return 3; } + function test1(uint256 value) public pure returns (uint256) { return value.bar(); } + function test2(uint256 value) public pure returns (uint256) { return foo(0); } +} +// ---- +// library: L +// test1(uint256): 0 -> 1 diff --git a/test/libsolidity/semanticTests/functionScopting/private_function_vs_overloading.sol b/test/libsolidity/semanticTests/functionScopting/private_function_vs_overloading.sol new file mode 100644 index 000000000..ba745ff71 --- /dev/null +++ b/test/libsolidity/semanticTests/functionScopting/private_function_vs_overloading.sol @@ -0,0 +1,18 @@ +// Tests that private functions are not overridden by inheriting contract's functions. +contract A { + function foo() private pure returns (uint256) { return 1; } + function foo(uint256 value) private pure returns (uint256) { return 2; } + function test1() public pure returns (uint256) { return foo(); } + function test2() public pure returns (uint256) { return foo(0); } +} +contract B is A { + function foo() private pure returns (uint256) { return 3; } + function foo(uint256 value) private pure returns (uint256) { return 4; } + function test3() public pure returns (uint256) { return foo(); } + function test4() public pure returns (uint256) { return foo(0); } +} +// ---- +// test1() -> 1 +// test2() -> 2 +// test3() -> 3 +// test4() -> 4 diff --git a/test/libsolidity/syntaxTests/inheritance/overloaded_private_function.sol b/test/libsolidity/syntaxTests/inheritance/overloaded_private_function.sol new file mode 100644 index 000000000..020b0ea80 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/overloaded_private_function.sol @@ -0,0 +1,13 @@ +// Tests that private functions are not overridden by inheriting contracts, but that public functions does. +contract A { + function foo() private {} + function foo(uint128 value) private {} + function foo(uint256 value) public {} +} +contract B is A { + function foo(uint128 value) private {} + function foo(uint256 value) public {} +} +// ---- +// TypeError 9456: (303-340): Overriding function is missing "override" specifier. +// TypeError 4334: (198-235): Trying to override non-virtual function. Did you forget to add "virtual"? diff --git a/test/libsolidity/syntaxTests/inheritance/override/implement_private_function_by_public_variable.sol b/test/libsolidity/syntaxTests/inheritance/override/implement_private_function_by_public_variable.sol index f78403d15..a8b1acff5 100644 --- a/test/libsolidity/syntaxTests/inheritance/override/implement_private_function_by_public_variable.sol +++ b/test/libsolidity/syntaxTests/inheritance/override/implement_private_function_by_public_variable.sol @@ -6,5 +6,5 @@ contract T { constructor() { new Y(); } } // ---- -// TypeError 5225: (97-130): Public state variables can only override functions with external visibility. +// TypeError 7792: (112-120): Public state variable has override specified but does not override anything. // TypeError 3942: (22-72): "virtual" and "private" cannot be used together. diff --git a/test/libsolidity/syntaxTests/inheritance/override/override_private_function.sol b/test/libsolidity/syntaxTests/inheritance/override/override_private_function.sol new file mode 100644 index 000000000..7ad92c752 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/override/override_private_function.sol @@ -0,0 +1,9 @@ +// Tests that private functions are not overridden by inheriting contracts. +contract A { + function foo() private {} +} +contract B is A { + function foo() private override {} +} +// ---- +// TypeError 7792: (160-168): Function has override specified but does not override anything. diff --git a/test/libsolidity/syntaxTests/inheritance/override/virtual_private.sol b/test/libsolidity/syntaxTests/inheritance/override/virtual_private.sol index 4fa8167c1..2102af5c3 100644 --- a/test/libsolidity/syntaxTests/inheritance/override/virtual_private.sol +++ b/test/libsolidity/syntaxTests/inheritance/override/virtual_private.sol @@ -5,4 +5,5 @@ abstract contract X is A { function test() private override returns (uint256) {} } // ---- +// TypeError 7792: (128-136): Function has override specified but does not override anything. // TypeError 3942: (23-73): "virtual" and "private" cannot be used together. diff --git a/test/libsolidity/syntaxTests/inheritance/shadowing_private_base_function.sol b/test/libsolidity/syntaxTests/inheritance/shadowing_private_base_function.sol new file mode 100644 index 000000000..ee23974ce --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/shadowing_private_base_function.sol @@ -0,0 +1,11 @@ +// Tests that private functions do not shadow, and are not shadowed by, inheriting contract's functions. +contract A { + function foo() private {} +} +contract B is A { + function foo() private {} +} +contract C is B { + function foo() public {} +} +// ---- diff --git a/test/libsolidity/syntaxTests/scoping/double_private_function_declaration.sol b/test/libsolidity/syntaxTests/scoping/double_private_function_declaration.sol new file mode 100644 index 000000000..575bdde97 --- /dev/null +++ b/test/libsolidity/syntaxTests/scoping/double_private_function_declaration.sol @@ -0,0 +1,7 @@ +// Tests that two private functions cannot be defined with the same name. +contract A { + function foo() private {} + function foo() private {} +} +// ---- +// DeclarationError 1686: (88-113): Function with same name and parameter types defined twice.