Skip to content

Commit 416c3dc

Browse files
ekpyronekpyron
authored
Merge pull request #13139 from ethereum/lvalueBytesPushWarning
Warn about multiple assignments to storage byte pushes.
2 parents 7f07497 + 16245f7 commit 416c3dc

File tree

9 files changed

+141
-25
lines changed

9 files changed

+141
-25
lines changed

‎Changelog.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ Language Features:
1111

1212
Compiler Features:
1313
* LSP: Add rudimentary support for semantic highlighting.
14+
* Type Checker: Warn about assignments involving multiple pushes to storage ``bytes`` that may invalidate references.
1415
* Yul Optimizer: Improve inlining heuristics for via IR code generation and pure Yul compilation.
1516

1617

‎libsolidity/analysis/ControlFlowBuilder.cpp

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
// SPDX-License-Identifier: GPL-3.0
1818

1919
#include <libsolidity/analysis/ControlFlowBuilder.h>
20+
#include <libsolidity/ast/ASTUtils.h>
2021
#include <libyul/AST.h>
2122
#include <libyul/backends/evm/EVMDialect.h>
2223

@@ -617,11 +618,7 @@ bool ControlFlowBuilder::visit(VariableDeclarationStatement const& _variableDecl
617618
solAssert(tupleExpression->components().size() > i, "");
618619
expression = tupleExpression->components()[i].get();
619620
}
620-
while (auto tupleExpression = dynamic_cast<TupleExpression const*>(expression))
621-
if (tupleExpression->components().size() == 1)
622-
expression = tupleExpression->components().front().get();
623-
else
624-
break;
621+
expression = resolveOuterUnaryTuples(expression);
625622
m_currentNode->variableOccurrences.emplace_back(
626623
*var,
627624
VariableOccurrence::Kind::Assignment,

‎libsolidity/analysis/TypeChecker.cpp

Lines changed: 76 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,10 @@
4040
#include <boost/algorithm/string/join.hpp>
4141
#include <boost/algorithm/string/predicate.hpp>
4242

43-
#include <range/v3/view/zip.hpp>
44-
#include <range/v3/view/drop_exactly.hpp>
4543
#include <range/v3/algorithm/count_if.hpp>
44+
#include <range/v3/view/drop_exactly.hpp>
45+
#include <range/v3/view/enumerate.hpp>
46+
#include <range/v3/view/zip.hpp>
4647

4748
#include <memory>
4849
#include <vector>
@@ -105,26 +106,71 @@ bool TypeChecker::visit(ContractDefinition const& _contract)
105106

106107
void TypeChecker::checkDoubleStorageAssignment(Assignment const& _assignment)
107108
{
108-
TupleType const& lhs = dynamic_cast<TupleType const&>(*type(_assignment.leftHandSide()));
109-
TupleType const& rhs = dynamic_cast<TupleType const&>(*type(_assignment.rightHandSide()));
110-
111-
if (lhs.components().size() != rhs.components().size())
112-
{
113-
solAssert(m_errorReporter.hasErrors(), "");
114-
return;
115-
}
116-
117109
size_t storageToStorageCopies = 0;
118110
size_t toStorageCopies = 0;
119-
for (size_t i = 0; i < lhs.components().size(); ++i)
120-
{
121-
ReferenceType const* ref = dynamic_cast<ReferenceType const*>(lhs.components()[i]);
122-
if (!ref || !ref->dataStoredIn(DataLocation::Storage) || ref->isPointer())
123-
continue;
124-
toStorageCopies++;
125-
if (rhs.components()[i]->dataStoredIn(DataLocation::Storage))
126-
storageToStorageCopies++;
127-
}
111+
size_t storageByteArrayPushes = 0;
112+
size_t storageByteAccesses = 0;
113+
auto count = [&](TupleExpression const& _lhs, TupleType const& _rhs, auto _recurse) -> void {
114+
TupleType const& lhsType = dynamic_cast<TupleType const&>(*type(_lhs));
115+
116+
if (lhsType.components().size() != _rhs.components().size())
117+
{
118+
solAssert(m_errorReporter.hasErrors(), "");
119+
return;
120+
}
121+
122+
for (auto&& [index, componentType]: lhsType.components() | ranges::views::enumerate)
123+
{
124+
if (ReferenceType const* ref = dynamic_cast<ReferenceType const*>(componentType))
125+
{
126+
if (ref && ref->dataStoredIn(DataLocation::Storage) && !ref->isPointer())
127+
{
128+
toStorageCopies++;
129+
if (_rhs.components()[index]->dataStoredIn(DataLocation::Storage))
130+
storageToStorageCopies++;
131+
}
132+
}
133+
else if (FixedBytesType const* bytesType = dynamic_cast<FixedBytesType const*>(componentType))
134+
{
135+
if (bytesType && bytesType->numBytes() == 1)
136+
{
137+
if (FunctionCall const* lhsCall = dynamic_cast<FunctionCall const*>(resolveOuterUnaryTuples(_lhs.components().at(index).get())))
138+
{
139+
FunctionType const& callType = dynamic_cast<FunctionType const&>(*type(lhsCall->expression()));
140+
if (callType.kind() == FunctionType::Kind::ArrayPush)
141+
{
142+
ArrayType const& arrayType = dynamic_cast<ArrayType const&>(*callType.selfType());
143+
if (arrayType.isByteArray() && arrayType.dataStoredIn(DataLocation::Storage))
144+
{
145+
++storageByteAccesses;
146+
++storageByteArrayPushes;
147+
}
148+
}
149+
}
150+
else if (IndexAccess const* indexAccess = dynamic_cast<IndexAccess const*>(resolveOuterUnaryTuples(_lhs.components().at(index).get())))
151+
{
152+
if (ArrayType const* arrayType = dynamic_cast<ArrayType const*>(type(indexAccess->baseExpression())))
153+
if (arrayType->isByteArray() && arrayType->dataStoredIn(DataLocation::Storage))
154+
++storageByteAccesses;
155+
}
156+
}
157+
}
158+
else if (TupleType const* tupleType = dynamic_cast<TupleType const*>(componentType))
159+
if (auto const* lhsNested = dynamic_cast<TupleExpression const*>(_lhs.components().at(index).get()))
160+
if (auto const* rhsNestedType = dynamic_cast<TupleType const*>(_rhs.components().at(index)))
161+
_recurse(
162+
*lhsNested,
163+
*rhsNestedType,
164+
_recurse
165+
);
166+
}
167+
};
168+
count(
169+
dynamic_cast<TupleExpression const&>(_assignment.leftHandSide()),
170+
dynamic_cast<TupleType const&>(*type(_assignment.rightHandSide())),
171+
count
172+
);
173+
128174
if (storageToStorageCopies >= 1 && toStorageCopies >= 2)
129175
m_errorReporter.warning(
130176
7238_error,
@@ -134,6 +180,16 @@ void TypeChecker::checkDoubleStorageAssignment(Assignment const& _assignment)
134180
"is executed and thus may have unexpected effects. It is safer to perform the copies "
135181
"separately or assign to storage pointers first."
136182
);
183+
184+
if (storageByteArrayPushes >= 1 && storageByteAccesses >= 2)
185+
m_errorReporter.warning(
186+
7239_error,
187+
_assignment.location(),
188+
"This assignment involves multiple accesses to a bytes array in storage while simultaneously enlarging it. "
189+
"When a bytes array is enlarged, it may transition from short storage layout to long storage layout, "
190+
"which invalidates all references to its elements. It is safer to only enlarge byte arrays in a single "
191+
"operation, one element at a time."
192+
);
137193
}
138194

139195
TypePointers TypeChecker::typeCheckABIDecodeAndRetrieveReturnType(FunctionCall const& _functionCall, bool _abiEncoderV2)

‎libsolidity/ast/ASTUtils.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,4 +85,14 @@ VariableDeclaration const* rootConstVariableDeclaration(VariableDeclaration cons
8585
return rootDecl;
8686
}
8787

88+
Expression const* resolveOuterUnaryTuples(Expression const* _expr)
89+
{
90+
while (auto const* tupleExpression = dynamic_cast<TupleExpression const*>(_expr))
91+
if (tupleExpression->components().size() == 1)
92+
_expr = tupleExpression->components().front().get();
93+
else
94+
break;
95+
return _expr;
96+
}
97+
8898
}

‎libsolidity/ast/ASTUtils.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,4 +38,8 @@ bool isConstantVariableRecursive(VariableDeclaration const& _varDecl);
3838
/// Returns the innermost AST node that covers the given location or nullptr if not found.
3939
ASTNode const* locateInnermostASTNode(int _offsetInFile, SourceUnit const& _sourceUnit);
4040

41+
/// @returns @a _expr itself, in case it is not a unary tuple expression. Otherwise it descends recursively
42+
/// into unary tuples and returns the contained expression.
43+
Expression const* resolveOuterUnaryTuples(Expression const* _expr);
44+
4145
}

‎test/libsolidity/syntaxTests/array/bytes1_array_push_assign_multi.sol

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
contract C {
2+
bytes1[] x;
3+
bytes1[] z;
4+
function f() public {
5+
(x.push(), x.push()) = (0, 0);
6+
(((x.push())), (x.push())) = (0, 0);
7+
((x.push(), x.push()), x.push()) = ((0, 0), 0);
8+
(x.push(), x[0]) = (0, 0);
9+
bytes1[] storage y = x;
10+
(x.push(), y.push()) = (0, 0);
11+
(x.push(), z.push()) = (0, 0);
12+
}
13+
}
14+
// ----

‎test/libsolidity/syntaxTests/array/bytes_assign_multi_fine.sol

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
contract C {
2+
bytes x;
3+
function f() public {
4+
(x[0], x[1]) = (0, 0);
5+
(x[0], x[1]) = (x[1], x[0]);
6+
}
7+
}
8+
// ----

‎test/libsolidity/syntaxTests/array/bytes_push_assign_multi.sol

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
contract C {
2+
bytes x;
3+
bytes z;
4+
function f() public {
5+
(x.push(), x.push()) = (0, 0);
6+
(((x.push())), (x.push())) = (0, 0);
7+
((x.push(), x.push()), x.push()) = ((0, 0), 0);
8+
(x.push(), x[0]) = (0, 0);
9+
bytes storage y = x;
10+
(x.push(), y.push()) = (0, 0);
11+
// The following is a false positive.
12+
(x.push(), z.push()) = (0, 0);
13+
}
14+
}
15+
// ----
16+
// 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.
17+
// 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.
18+
// 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.
19+
// 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.
20+
// 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.
21+
// 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.

‎test/libsolidity/syntaxTests/tupleAssignments/warn_multiple_storage_storage_copies.sol

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ contract C {
44
function f() public {
55
(x, y) = (y, x);
66
}
7+
function g() public {
8+
uint z;
9+
((x, y), z) = ((y, x), 0);
10+
}
711
}
812
// ----
913
// 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.
14+
// 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.

0 commit comments

Comments
 (0)