From d30b04674ea000c705749d0371e5086afdccec7a Mon Sep 17 00:00:00 2001 From: Daniel Kirchner Date: Tue, 14 Jun 2022 15:31:32 +0200 Subject: [PATCH 1/2] Add resolveUnaryTuples helper. --- libsolidity/analysis/ControlFlowBuilder.cpp | 7 ++----- libsolidity/ast/ASTUtils.cpp | 10 ++++++++++ libsolidity/ast/ASTUtils.h | 4 ++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/libsolidity/analysis/ControlFlowBuilder.cpp b/libsolidity/analysis/ControlFlowBuilder.cpp index 7c568b18e..06787a512 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 = resolveUnaryTuples(expression); m_currentNode->variableOccurrences.emplace_back( *var, VariableOccurrence::Kind::Assignment, diff --git a/libsolidity/ast/ASTUtils.cpp b/libsolidity/ast/ASTUtils.cpp index 2b7ea7427..b4e75bace 100644 --- a/libsolidity/ast/ASTUtils.cpp +++ b/libsolidity/ast/ASTUtils.cpp @@ -85,4 +85,14 @@ VariableDeclaration const* rootConstVariableDeclaration(VariableDeclaration cons return rootDecl; } +Expression const* resolveUnaryTuples(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..7ec79ef1b 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 an unary tuple expression. Otherwise it descends recursively +/// into unary tuples and returns the contained expression. +Expression const* resolveUnaryTuples(Expression const* _expr); + } From 16245f7b9b86c1b524f4c5479c5694e91ff933de Mon Sep 17 00:00:00 2001 From: Daniel Kirchner Date: Tue, 14 Jun 2022 14:55:06 +0200 Subject: [PATCH 2/2] Warn about multiple assignments to storage byte pushes and fix warnings about multiple storage to storage copies. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kamil Śliwak --- Changelog.md | 1 + libsolidity/analysis/ControlFlowBuilder.cpp | 2 +- libsolidity/analysis/TypeChecker.cpp | 96 +++++++++++++++---- libsolidity/ast/ASTUtils.cpp | 2 +- libsolidity/ast/ASTUtils.h | 4 +- .../array/bytes1_array_push_assign_multi.sol | 14 +++ .../array/bytes_assign_multi_fine.sol | 8 ++ .../array/bytes_push_assign_multi.sol | 21 ++++ .../warn_multiple_storage_storage_copies.sol | 5 + 9 files changed, 129 insertions(+), 24 deletions(-) create mode 100644 test/libsolidity/syntaxTests/array/bytes1_array_push_assign_multi.sol create mode 100644 test/libsolidity/syntaxTests/array/bytes_assign_multi_fine.sol create mode 100644 test/libsolidity/syntaxTests/array/bytes_push_assign_multi.sol 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 06787a512..1d1e33f50 100644 --- a/libsolidity/analysis/ControlFlowBuilder.cpp +++ b/libsolidity/analysis/ControlFlowBuilder.cpp @@ -618,7 +618,7 @@ bool ControlFlowBuilder::visit(VariableDeclarationStatement const& _variableDecl solAssert(tupleExpression->components().size() > i, ""); expression = tupleExpression->components()[i].get(); } - expression = resolveUnaryTuples(expression); + 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 b4e75bace..27c0833d2 100644 --- a/libsolidity/ast/ASTUtils.cpp +++ b/libsolidity/ast/ASTUtils.cpp @@ -85,7 +85,7 @@ VariableDeclaration const* rootConstVariableDeclaration(VariableDeclaration cons return rootDecl; } -Expression const* resolveUnaryTuples(Expression const* _expr) +Expression const* resolveOuterUnaryTuples(Expression const* _expr) { while (auto const* tupleExpression = dynamic_cast(_expr)) if (tupleExpression->components().size() == 1) diff --git a/libsolidity/ast/ASTUtils.h b/libsolidity/ast/ASTUtils.h index 7ec79ef1b..f6e3f0641 100644 --- a/libsolidity/ast/ASTUtils.h +++ b/libsolidity/ast/ASTUtils.h @@ -38,8 +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 an unary tuple expression. Otherwise it descends recursively +/// @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* resolveUnaryTuples(Expression const* _expr); +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.