diff --git a/Changelog.md b/Changelog.md index f85404b9d..f6a8147e0 100644 --- a/Changelog.md +++ b/Changelog.md @@ -11,6 +11,7 @@ Language Features: Compiler Features: * LSP: Add rudimentary support for semantic highlighting. + * Type Checker: Warn about assignments involving multiple pushes to storage ``bytes`` that may invalidate references. * Yul Optimizer: Improve inlining heuristics for via IR code generation and pure Yul compilation. diff --git a/libsolidity/analysis/ControlFlowBuilder.cpp b/libsolidity/analysis/ControlFlowBuilder.cpp index 7c568b18e..1d1e33f50 100644 --- a/libsolidity/analysis/ControlFlowBuilder.cpp +++ b/libsolidity/analysis/ControlFlowBuilder.cpp @@ -17,6 +17,7 @@ // SPDX-License-Identifier: GPL-3.0 #include +#include #include #include @@ -617,11 +618,7 @@ bool ControlFlowBuilder::visit(VariableDeclarationStatement const& _variableDecl solAssert(tupleExpression->components().size() > i, ""); expression = tupleExpression->components()[i].get(); } - while (auto tupleExpression = dynamic_cast(expression)) - if (tupleExpression->components().size() == 1) - expression = tupleExpression->components().front().get(); - else - break; + expression = resolveOuterUnaryTuples(expression); m_currentNode->variableOccurrences.emplace_back( *var, VariableOccurrence::Kind::Assignment, diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 7f31fb56f..2467125d7 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -40,9 +40,10 @@ #include #include -#include -#include #include +#include +#include +#include #include #include @@ -105,26 +106,71 @@ bool TypeChecker::visit(ContractDefinition const& _contract) void TypeChecker::checkDoubleStorageAssignment(Assignment const& _assignment) { - TupleType const& lhs = dynamic_cast(*type(_assignment.leftHandSide())); - TupleType const& rhs = dynamic_cast(*type(_assignment.rightHandSide())); - - if (lhs.components().size() != rhs.components().size()) - { - solAssert(m_errorReporter.hasErrors(), ""); - return; - } - size_t storageToStorageCopies = 0; size_t toStorageCopies = 0; - for (size_t i = 0; i < lhs.components().size(); ++i) - { - ReferenceType const* ref = dynamic_cast(lhs.components()[i]); - if (!ref || !ref->dataStoredIn(DataLocation::Storage) || ref->isPointer()) - continue; - toStorageCopies++; - if (rhs.components()[i]->dataStoredIn(DataLocation::Storage)) - storageToStorageCopies++; - } + size_t storageByteArrayPushes = 0; + size_t storageByteAccesses = 0; + auto count = [&](TupleExpression const& _lhs, TupleType const& _rhs, auto _recurse) -> void { + TupleType const& lhsType = dynamic_cast(*type(_lhs)); + + if (lhsType.components().size() != _rhs.components().size()) + { + solAssert(m_errorReporter.hasErrors(), ""); + return; + } + + for (auto&& [index, componentType]: lhsType.components() | ranges::views::enumerate) + { + if (ReferenceType const* ref = dynamic_cast(componentType)) + { + if (ref && ref->dataStoredIn(DataLocation::Storage) && !ref->isPointer()) + { + toStorageCopies++; + if (_rhs.components()[index]->dataStoredIn(DataLocation::Storage)) + storageToStorageCopies++; + } + } + else if (FixedBytesType const* bytesType = dynamic_cast(componentType)) + { + if (bytesType && bytesType->numBytes() == 1) + { + if (FunctionCall const* lhsCall = dynamic_cast(resolveOuterUnaryTuples(_lhs.components().at(index).get()))) + { + FunctionType const& callType = dynamic_cast(*type(lhsCall->expression())); + if (callType.kind() == FunctionType::Kind::ArrayPush) + { + ArrayType const& arrayType = dynamic_cast(*callType.selfType()); + if (arrayType.isByteArray() && arrayType.dataStoredIn(DataLocation::Storage)) + { + ++storageByteAccesses; + ++storageByteArrayPushes; + } + } + } + else if (IndexAccess const* indexAccess = dynamic_cast(resolveOuterUnaryTuples(_lhs.components().at(index).get()))) + { + if (ArrayType const* arrayType = dynamic_cast(type(indexAccess->baseExpression()))) + if (arrayType->isByteArray() && arrayType->dataStoredIn(DataLocation::Storage)) + ++storageByteAccesses; + } + } + } + else if (TupleType const* tupleType = dynamic_cast(componentType)) + if (auto const* lhsNested = dynamic_cast(_lhs.components().at(index).get())) + if (auto const* rhsNestedType = dynamic_cast(_rhs.components().at(index))) + _recurse( + *lhsNested, + *rhsNestedType, + _recurse + ); + } + }; + count( + dynamic_cast(_assignment.leftHandSide()), + dynamic_cast(*type(_assignment.rightHandSide())), + count + ); + if (storageToStorageCopies >= 1 && toStorageCopies >= 2) m_errorReporter.warning( 7238_error, @@ -134,6 +180,16 @@ void TypeChecker::checkDoubleStorageAssignment(Assignment const& _assignment) "is executed and thus may have unexpected effects. It is safer to perform the copies " "separately or assign to storage pointers first." ); + + if (storageByteArrayPushes >= 1 && storageByteAccesses >= 2) + m_errorReporter.warning( + 7239_error, + _assignment.location(), + "This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. " + "When a bytes array is enlarged, it may transition from short storage layout to long storage layout, " + "which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single " + "operation, one element at a time." + ); } TypePointers TypeChecker::typeCheckABIDecodeAndRetrieveReturnType(FunctionCall const& _functionCall, bool _abiEncoderV2) diff --git a/libsolidity/ast/ASTUtils.cpp b/libsolidity/ast/ASTUtils.cpp index 2b7ea7427..27c0833d2 100644 --- a/libsolidity/ast/ASTUtils.cpp +++ b/libsolidity/ast/ASTUtils.cpp @@ -85,4 +85,14 @@ VariableDeclaration const* rootConstVariableDeclaration(VariableDeclaration cons return rootDecl; } +Expression const* resolveOuterUnaryTuples(Expression const* _expr) +{ + while (auto const* tupleExpression = dynamic_cast(_expr)) + if (tupleExpression->components().size() == 1) + _expr = tupleExpression->components().front().get(); + else + break; + return _expr; +} + } diff --git a/libsolidity/ast/ASTUtils.h b/libsolidity/ast/ASTUtils.h index 2bfbb8fe9..f6e3f0641 100644 --- a/libsolidity/ast/ASTUtils.h +++ b/libsolidity/ast/ASTUtils.h @@ -38,4 +38,8 @@ bool isConstantVariableRecursive(VariableDeclaration const& _varDecl); /// Returns the innermost AST node that covers the given location or nullptr if not found. ASTNode const* locateInnermostASTNode(int _offsetInFile, SourceUnit const& _sourceUnit); +/// @returns @a _expr itself, in case it is not a unary tuple expression. Otherwise it descends recursively +/// into unary tuples and returns the contained expression. +Expression const* resolveOuterUnaryTuples(Expression const* _expr); + } diff --git a/test/libsolidity/syntaxTests/array/bytes1_array_push_assign_multi.sol b/test/libsolidity/syntaxTests/array/bytes1_array_push_assign_multi.sol new file mode 100644 index 000000000..dd7fcda97 --- /dev/null +++ b/test/libsolidity/syntaxTests/array/bytes1_array_push_assign_multi.sol @@ -0,0 +1,14 @@ +contract C { + bytes1[] x; + bytes1[] z; + function f() public { + (x.push(), x.push()) = (0, 0); + (((x.push())), (x.push())) = (0, 0); + ((x.push(), x.push()), x.push()) = ((0, 0), 0); + (x.push(), x[0]) = (0, 0); + bytes1[] storage y = x; + (x.push(), y.push()) = (0, 0); + (x.push(), z.push()) = (0, 0); + } +} +// ---- diff --git a/test/libsolidity/syntaxTests/array/bytes_assign_multi_fine.sol b/test/libsolidity/syntaxTests/array/bytes_assign_multi_fine.sol new file mode 100644 index 000000000..1e954d1fb --- /dev/null +++ b/test/libsolidity/syntaxTests/array/bytes_assign_multi_fine.sol @@ -0,0 +1,8 @@ +contract C { + bytes x; + function f() public { + (x[0], x[1]) = (0, 0); + (x[0], x[1]) = (x[1], x[0]); + } +} +// ---- diff --git a/test/libsolidity/syntaxTests/array/bytes_push_assign_multi.sol b/test/libsolidity/syntaxTests/array/bytes_push_assign_multi.sol new file mode 100644 index 000000000..320e93963 --- /dev/null +++ b/test/libsolidity/syntaxTests/array/bytes_push_assign_multi.sol @@ -0,0 +1,21 @@ +contract C { + bytes x; + bytes z; + function f() public { + (x.push(), x.push()) = (0, 0); + (((x.push())), (x.push())) = (0, 0); + ((x.push(), x.push()), x.push()) = ((0, 0), 0); + (x.push(), x[0]) = (0, 0); + bytes storage y = x; + (x.push(), y.push()) = (0, 0); + // The following is a false positive. + (x.push(), z.push()) = (0, 0); + } +} +// ---- +// Warning 7239: (73-102): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time. +// Warning 7239: (112-147): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time. +// Warning 7239: (157-203): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time. +// Warning 7239: (213-238): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time. +// Warning 7239: (277-306): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time. +// Warning 7239: (362-391): This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. When a bytes array is enlarged, it may transition from short storage layout to long storage layout, which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single operation, one element at a time. diff --git a/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol index 102bbea68..e35d361d2 100644 --- a/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol +++ b/test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol @@ -4,6 +4,11 @@ contract C { function f() public { (x, y) = (y, x); } + function g() public { + uint z; + ((x, y), z) = ((y, x), 0); + } } // ---- // Warning 7238: (79-94): This assignment performs two copies to storage. Since storage copies do not first copy to a temporary location, one of them might be overwritten before the second is executed and thus may have unexpected effects. It is safer to perform the copies separately or assign to storage pointers first. +// Warning 7238: (134-159): This assignment performs two copies to storage. Since storage copies do not first copy to a temporary location, one of them might be overwritten before the second is executed and thus may have unexpected effects. It is safer to perform the copies separately or assign to storage pointers first.