From dfa0bcf760ddbebed102be227d906ebb066562f3 Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 24 Mar 2022 15:03:02 +0100 Subject: [PATCH] More strict override check for data locations. --- libsolidity/analysis/ContractLevelChecker.cpp | 4 +- libsolidity/analysis/OverrideChecker.cpp | 52 +++++++++++++++++-- libsolidity/analysis/OverrideChecker.h | 6 ++- libsolidity/ast/AST.cpp | 9 +++- .../dataLocation/external_public_calldata.sol | 18 +++++++ .../external_overriding_external.sol | 14 +++++ ..._data_location_change_illegal_internal.sol | 13 +++++ ..._change_calldata_memory_illegal_public.sol | 11 ++++ ..._data_location_change_illegal_internal.sol | 16 ++++++ ...er_data_location_change_illegal_public.sol | 16 ++++++ .../return_type_data_location.sol | 7 +++ ...turn_type_data_location_change_illegal.sol | 8 +++ tools/solidityUpgrade/Upgrade060.cpp | 2 +- 13 files changed, 165 insertions(+), 11 deletions(-) create mode 100644 test/libsolidity/semanticTests/inheritance/dataLocation/external_public_calldata.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/dataLocation/external_overriding_external.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/dataLocation/modifier_parameter_data_location_change_illegal_internal.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_calldata_memory_illegal_public.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_illegal_internal.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_illegal_public.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/dataLocation/return_type_data_location.sol create mode 100644 test/libsolidity/syntaxTests/inheritance/dataLocation/return_type_data_location_change_illegal.sol diff --git a/libsolidity/analysis/ContractLevelChecker.cpp b/libsolidity/analysis/ContractLevelChecker.cpp index c159b8357..9ecf76f13 100644 --- a/libsolidity/analysis/ContractLevelChecker.cpp +++ b/libsolidity/analysis/ContractLevelChecker.cpp @@ -39,7 +39,7 @@ namespace { template -bool hasEqualParameters(T const& _a, B const& _b) +bool hasEqualExternalCallableParameters(T const& _a, B const& _b) { return FunctionType(_a).asExternallyCallableFunction(false)->hasEqualParameterTypes( *FunctionType(_b).asExternallyCallableFunction(false) @@ -204,7 +204,7 @@ void ContractLevelChecker::findDuplicateDefinitions(map> const SecondarySourceLocation ssl; for (size_t j = i + 1; j < overloads.size(); ++j) - if (hasEqualParameters(*overloads[i], *overloads[j])) + if (hasEqualExternalCallableParameters(*overloads[i], *overloads[j])) { solAssert( ( diff --git a/libsolidity/analysis/OverrideChecker.cpp b/libsolidity/analysis/OverrideChecker.cpp index 2f83748fb..d342ab100 100644 --- a/libsolidity/analysis/OverrideChecker.cpp +++ b/libsolidity/analysis/OverrideChecker.cpp @@ -313,7 +313,7 @@ Token OverrideProxy::functionKind() const }, m_item); } -FunctionType const* OverrideProxy::functionType() const +FunctionType const* OverrideProxy::externalFunctionType() const { return std::visit(GenericVisitor{ [&](FunctionDefinition const* _item) { return FunctionType(*_item).asExternallyCallableFunction(false); }, @@ -322,6 +322,15 @@ FunctionType const* OverrideProxy::functionType() const }, m_item); } +FunctionType const* OverrideProxy::originalFunctionType() const +{ + return std::visit(GenericVisitor{ + [&](FunctionDefinition const* _item) { return TypeProvider::function(*_item); }, + [&](VariableDeclaration const*) -> FunctionType const* { solAssert(false, "Requested specific function type of variable."); return nullptr; }, + [&](ModifierDefinition const*) -> FunctionType const* { solAssert(false, "Requested specific function type of modifier."); return nullptr; } + }, m_item); +} + ModifierType const* OverrideProxy::modifierType() const { return std::visit(GenericVisitor{ @@ -413,7 +422,7 @@ OverrideProxy::OverrideComparator const& OverrideProxy::overrideComparator() con [&](FunctionDefinition const* _function) { vector paramTypes; - for (Type const* t: functionType()->parameterTypes()) + for (Type const* t: externalFunctionType()->parameterTypes()) paramTypes.emplace_back(t->richIdentifier()); return OverrideComparator{ _function->name(), @@ -424,7 +433,7 @@ OverrideProxy::OverrideComparator const& OverrideProxy::overrideComparator() con [&](VariableDeclaration const* _var) { vector paramTypes; - for (Type const* t: functionType()->parameterTypes()) + for (Type const* t: externalFunctionType()->parameterTypes()) paramTypes.emplace_back(t->richIdentifier()); return OverrideComparator{ _var->name(), @@ -589,14 +598,17 @@ void OverrideChecker::checkOverride(OverrideProxy const& _overriding, OverridePr if (_super.isFunction()) { - FunctionType const* functionType = _overriding.functionType(); - FunctionType const* superType = _super.functionType(); + FunctionType const* functionType = _overriding.externalFunctionType(); + FunctionType const* superType = _super.externalFunctionType(); + bool returnTypesDifferAlready = false; if (_overriding.functionKind() != Token::Fallback) { solAssert(functionType->hasEqualParameterTypes(*superType), "Override doesn't have equal parameters!"); if (!functionType->hasEqualReturnTypes(*superType)) + { + returnTypesDifferAlready = true; overrideError( _overriding, _super, @@ -604,6 +616,36 @@ void OverrideChecker::checkOverride(OverrideProxy const& _overriding, OverridePr "Overriding " + _overriding.astNodeName() + " return types differ.", "Overridden " + _overriding.astNodeName() + " is here:" ); + } + } + + // The override proxy considers calldata and memory the same data location. + // Here we do a more specific check: + // Data locations of parameters and return variables have to match + // unless we have a public function overriding an external one. + if ( + _overriding.isFunction() && + !returnTypesDifferAlready && + _super.visibility() != Visibility::External && + _overriding.functionKind() != Token::Fallback + ) + { + if (!_overriding.originalFunctionType()->hasEqualParameterTypes(*_super.originalFunctionType())) + overrideError( + _overriding, + _super, + 7723_error, + "Data locations of parameters have to be the same when overriding non-external functions, but they differ.", + "Overridden " + _overriding.astNodeName() + " is here:" + ); + if (!_overriding.originalFunctionType()->hasEqualReturnTypes(*_super.originalFunctionType())) + overrideError( + _overriding, + _super, + 1443_error, + "Data locations of return variables have to be the same when overriding non-external functions, but they differ.", + "Overridden " + _overriding.astNodeName() + " is here:" + ); } // Stricter mutability is always okay except when super is Payable diff --git a/libsolidity/analysis/OverrideChecker.h b/libsolidity/analysis/OverrideChecker.h index 9178d179d..824248325 100644 --- a/libsolidity/analysis/OverrideChecker.h +++ b/libsolidity/analysis/OverrideChecker.h @@ -84,7 +84,10 @@ public: /// @returns receive / fallback / function (only the latter for modifiers and variables); langutil::Token functionKind() const; - FunctionType const* functionType() const; + /// @returns the externally callable function type + FunctionType const* externalFunctionType() const; + /// @returns the (unmodified) function type + FunctionType const* originalFunctionType() const; ModifierType const* modifierType() const; Declaration const* declaration() const; @@ -101,6 +104,7 @@ public: /** * Struct to help comparing override items about whether they override each other. + * Compares functions based on their "externally callable" type. * Does not produce a total order. */ struct OverrideComparator diff --git a/libsolidity/ast/AST.cpp b/libsolidity/ast/AST.cpp index be6f2fe9b..04c47a155 100644 --- a/libsolidity/ast/AST.cpp +++ b/libsolidity/ast/AST.cpp @@ -480,7 +480,9 @@ FunctionDefinition const& FunctionDefinition::resolveVirtual( solAssert(isOrdinary(), ""); solAssert(!libraryFunction(), ""); - FunctionType const* functionType = TypeProvider::function(*this)->asExternallyCallableFunction(false); + // We actually do not want the externally callable function here. + // This is just to add an assertion since the comparison used to be less strict. + FunctionType const* externalFunctionType = TypeProvider::function(*this)->asExternallyCallableFunction(false); bool foundSearchStart = (_searchStart == nullptr); for (ContractDefinition const* c: _mostDerivedContract.annotation().linearizedBaseContracts) @@ -495,9 +497,12 @@ FunctionDefinition const& FunctionDefinition::resolveVirtual( // With super lookup analysis guarantees that there is an implemented function in the chain. // With virtual lookup there are valid cases where returning an unimplemented one is fine. (function->isImplemented() || _searchStart == nullptr) && - FunctionType(*function).asExternallyCallableFunction(false)->hasEqualParameterTypes(*functionType) + FunctionType(*function).asExternallyCallableFunction(false)->hasEqualParameterTypes(*externalFunctionType) ) + { + solAssert(FunctionType(*function).hasEqualParameterTypes(*TypeProvider::function(*this))); return *function; + } } solAssert(false, "Virtual function " + name() + " not found."); diff --git a/test/libsolidity/semanticTests/inheritance/dataLocation/external_public_calldata.sol b/test/libsolidity/semanticTests/inheritance/dataLocation/external_public_calldata.sol new file mode 100644 index 000000000..4de95b0f1 --- /dev/null +++ b/test/libsolidity/semanticTests/inheritance/dataLocation/external_public_calldata.sol @@ -0,0 +1,18 @@ +abstract contract A { + function f(uint256[] calldata a) external virtual returns (uint256[] calldata); +} + +contract B is A { + function f(uint256[] memory a) public override returns (uint256[] memory) { + return a; + } + + function g(uint[] calldata x) public returns (uint256[] memory) { + return f(x); + } +} +// ==== +// compileViaYul: also +// ---- +// f(uint256[]): 0x20, 2, 9, 8 -> 0x20, 2, 9, 8 +// g(uint256[]): 0x20, 2, 9, 8 -> 0x20, 2, 9, 8 diff --git a/test/libsolidity/syntaxTests/inheritance/dataLocation/external_overriding_external.sol b/test/libsolidity/syntaxTests/inheritance/dataLocation/external_overriding_external.sol new file mode 100644 index 000000000..6105a50a2 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/dataLocation/external_overriding_external.sol @@ -0,0 +1,14 @@ +abstract contract A { + function f(uint256[1] memory a) external virtual returns (uint256); +} +contract B is A { + function f(uint256[1] calldata a) external pure virtual override returns (uint256) { + return a[0]; + } +} +contract C is A, B { + function f(uint256[1] memory a) external pure override(B, A) returns (uint256) { + return a[0]; + } +} +// ---- diff --git a/test/libsolidity/syntaxTests/inheritance/dataLocation/modifier_parameter_data_location_change_illegal_internal.sol b/test/libsolidity/syntaxTests/inheritance/dataLocation/modifier_parameter_data_location_change_illegal_internal.sol new file mode 100644 index 000000000..a4856d9f0 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/dataLocation/modifier_parameter_data_location_change_illegal_internal.sol @@ -0,0 +1,13 @@ +abstract contract A { + modifier m(uint256[1] memory a) virtual; + function test(uint256[1] memory a) m(a) external { + } +} + +contract B is A { + modifier m(uint256[1] calldata a) override { + _; + } +} +// ---- +// TypeError 1078: (153-214): Override changes modifier signature. diff --git a/test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_calldata_memory_illegal_public.sol b/test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_calldata_memory_illegal_public.sol new file mode 100644 index 000000000..e3fb069af --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_calldata_memory_illegal_public.sol @@ -0,0 +1,11 @@ +abstract contract A { + function f(uint256[1] calldata a) public virtual returns (uint256); +} + +contract B is A { + function f(uint256[1] memory a) public override returns (uint256) { + return a[0]; + } +} +// ---- +// TypeError 7723: (119-213): Data locations of parameters have to be the same when overriding non-external functions, but they differ. diff --git a/test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_illegal_internal.sol b/test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_illegal_internal.sol new file mode 100644 index 000000000..55b5b5fe6 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_illegal_internal.sol @@ -0,0 +1,16 @@ +abstract contract A { + function f(uint256[1] memory a) internal virtual returns (uint256); + function test() external returns (uint) { + uint[1] memory t; + t[0] = 7; + return f(t); + } +} + +contract B is A { + function f(uint256[1] calldata a) internal override returns (uint256) { + return a[0]; + } +} +// ---- +// TypeError 7723: (236-334): Data locations of parameters have to be the same when overriding non-external functions, but they differ. diff --git a/test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_illegal_public.sol b/test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_illegal_public.sol new file mode 100644 index 000000000..52123f68d --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/dataLocation/parameter_data_location_change_illegal_public.sol @@ -0,0 +1,16 @@ +abstract contract A { + function f(uint256[1] memory a) public virtual returns (uint256); + function test() external returns (uint) { + uint[1] memory t; + t[0] = 7; + return f(t); + } +} + +contract B is A { + function f(uint256[1] calldata a) public override returns (uint256) { + return a[0]; + } +} +// ---- +// TypeError 7723: (234-330): Data locations of parameters have to be the same when overriding non-external functions, but they differ. diff --git a/test/libsolidity/syntaxTests/inheritance/dataLocation/return_type_data_location.sol b/test/libsolidity/syntaxTests/inheritance/dataLocation/return_type_data_location.sol new file mode 100644 index 000000000..1c7f4e926 --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/dataLocation/return_type_data_location.sol @@ -0,0 +1,7 @@ +abstract contract A { + function foo() external virtual view returns(uint[] calldata); +} +contract X is A { + function foo() public view override returns(uint[] memory) { } +} +// ---- diff --git a/test/libsolidity/syntaxTests/inheritance/dataLocation/return_type_data_location_change_illegal.sol b/test/libsolidity/syntaxTests/inheritance/dataLocation/return_type_data_location_change_illegal.sol new file mode 100644 index 000000000..e06cd43ae --- /dev/null +++ b/test/libsolidity/syntaxTests/inheritance/dataLocation/return_type_data_location_change_illegal.sol @@ -0,0 +1,8 @@ +abstract contract A { + function foo() public virtual view returns(uint[] calldata); +} +contract X is A { + function foo() public view override returns(uint[] memory) { } +} +// ---- +// TypeError 1443: (105-168): Data locations of return variables have to be the same when overriding non-external functions, but they differ. diff --git a/tools/solidityUpgrade/Upgrade060.cpp b/tools/solidityUpgrade/Upgrade060.cpp index 52010db39..8caff02c9 100644 --- a/tools/solidityUpgrade/Upgrade060.cpp +++ b/tools/solidityUpgrade/Upgrade060.cpp @@ -72,7 +72,7 @@ void OverridingFunction::endVisit(ContractDefinition const& _contract) { auto& super = (*begin); auto functionType = FunctionType(*function).asExternallyCallableFunction(false); - auto superType = super.functionType()->asExternallyCallableFunction(false); + auto superType = super.externalFunctionType(); if (functionType && functionType->hasEqualParameterTypes(*superType)) {