Skip to content

Commit 16245f7

Browse files
ekpyroncameel
ekpyron
and
cameel
committed
Warn about multiple assignments to storage byte pushes and fix warnings about multiple storage to storage copies.
Co-authored-by: Kamil Śliwak <[email protected]>
1 parent d30b046 commit 16245f7

File tree

9 files changed

+129
-24
lines changed

9 files changed

+129
-24
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: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -618,7 +618,7 @@ bool ControlFlowBuilder::visit(VariableDeclarationStatement const& _variableDecl
618618
solAssert(tupleExpression->components().size() > i, "");
619619
expression = tupleExpression->components()[i].get();
620620
}
621-
expression = resolveUnaryTuples(expression);
621+
expression = resolveOuterUnaryTuples(expression);
622622
m_currentNode->variableOccurrences.emplace_back(
623623
*var,
624624
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: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ VariableDeclaration const* rootConstVariableDeclaration(VariableDeclaration cons
8585
return rootDecl;
8686
}
8787

88-
Expression const* resolveUnaryTuples(Expression const* _expr)
88+
Expression const* resolveOuterUnaryTuples(Expression const* _expr)
8989
{
9090
while (auto const* tupleExpression = dynamic_cast<TupleExpression const*>(_expr))
9191
if (tupleExpression->components().size() == 1)

‎libsolidity/ast/ASTUtils.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +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 an unary tuple expression. Otherwise it descends recursively
41+
/// @returns @a _expr itself, in case it is not a unary tuple expression. Otherwise it descends recursively
4242
/// into unary tuples and returns the contained expression.
43-
Expression const* resolveUnaryTuples(Expression const* _expr);
43+
Expression const* resolveOuterUnaryTuples(Expression const* _expr);
4444

4545
}

‎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)