From c2e1273ff4a83bcdbf62c836504ecf505a77e95a Mon Sep 17 00:00:00 2001 From: a3d4 Date: Thu, 11 Jun 2020 15:52:36 +0200 Subject: [PATCH 1/2] Fixed recursive check in structureSizeEstimate --- libsolidity/analysis/StaticAnalyzer.cpp | 19 ++++++++----------- .../large_storage_array_fine.sol} | 0 .../large_storage_array_mapping.sol | 4 ++++ .../large_storage_array_simple.sol} | 0 .../large_storage_arrays_combined.sol | 4 ++++ .../large_storage_arrays_struct.sol} | 0 .../largeTypes/large_storage_structs.sol | 18 ++++++++++++++++++ .../508_large_storage_arrays_combined.sol | 5 ----- .../510_large_storage_array_mapping.sol | 5 ----- 9 files changed, 34 insertions(+), 21 deletions(-) rename test/libsolidity/syntaxTests/{nameAndTypeResolution/506_large_storage_array_fine.sol => largeTypes/large_storage_array_fine.sol} (100%) create mode 100644 test/libsolidity/syntaxTests/largeTypes/large_storage_array_mapping.sol rename test/libsolidity/syntaxTests/{nameAndTypeResolution/507_large_storage_array_simple.sol => largeTypes/large_storage_array_simple.sol} (100%) create mode 100644 test/libsolidity/syntaxTests/largeTypes/large_storage_arrays_combined.sol rename test/libsolidity/syntaxTests/{nameAndTypeResolution/509_large_storage_arrays_struct.sol => largeTypes/large_storage_arrays_struct.sol} (100%) create mode 100644 test/libsolidity/syntaxTests/largeTypes/large_storage_structs.sol delete mode 100644 test/libsolidity/syntaxTests/nameAndTypeResolution/508_large_storage_arrays_combined.sol delete mode 100644 test/libsolidity/syntaxTests/nameAndTypeResolution/510_large_storage_array_mapping.sol diff --git a/libsolidity/analysis/StaticAnalyzer.cpp b/libsolidity/analysis/StaticAnalyzer.cpp index 3cbfa4db4..5524b3a68 100644 --- a/libsolidity/analysis/StaticAnalyzer.cpp +++ b/libsolidity/analysis/StaticAnalyzer.cpp @@ -346,24 +346,21 @@ bigint StaticAnalyzer::structureSizeEstimate(Type const& _type, set(_type); - return structureSizeEstimate(*t.baseType(), _structsSeen) * (t.isDynamicallySized() ? 1 : t.length()); + if (!t.isDynamicallySized()) + return structureSizeEstimate(*t.baseType(), _structsSeen) * t.length(); + break; } case Type::Category::Struct: { auto const& t = dynamic_cast(_type); + solAssert(!_structsSeen.count(&t.structDefinition()), "Recursive struct."); bigint size = 1; - if (!_structsSeen.count(&t.structDefinition())) - { - _structsSeen.insert(&t.structDefinition()); - for (auto const& m: t.members(nullptr)) - size += structureSizeEstimate(*m.type, _structsSeen); - } + _structsSeen.insert(&t.structDefinition()); + for (auto const& m: t.members(nullptr)) + size += structureSizeEstimate(*m.type, _structsSeen); + _structsSeen.erase(&t.structDefinition()); return size; } - case Type::Category::Mapping: - { - return structureSizeEstimate(*dynamic_cast(_type).valueType(), _structsSeen); - } default: break; } diff --git a/test/libsolidity/syntaxTests/nameAndTypeResolution/506_large_storage_array_fine.sol b/test/libsolidity/syntaxTests/largeTypes/large_storage_array_fine.sol similarity index 100% rename from test/libsolidity/syntaxTests/nameAndTypeResolution/506_large_storage_array_fine.sol rename to test/libsolidity/syntaxTests/largeTypes/large_storage_array_fine.sol diff --git a/test/libsolidity/syntaxTests/largeTypes/large_storage_array_mapping.sol b/test/libsolidity/syntaxTests/largeTypes/large_storage_array_mapping.sol new file mode 100644 index 000000000..34132bfbe --- /dev/null +++ b/test/libsolidity/syntaxTests/largeTypes/large_storage_array_mapping.sol @@ -0,0 +1,4 @@ +contract C { + mapping(uint => uint[2**100]) x; +} +// ---- diff --git a/test/libsolidity/syntaxTests/nameAndTypeResolution/507_large_storage_array_simple.sol b/test/libsolidity/syntaxTests/largeTypes/large_storage_array_simple.sol similarity index 100% rename from test/libsolidity/syntaxTests/nameAndTypeResolution/507_large_storage_array_simple.sol rename to test/libsolidity/syntaxTests/largeTypes/large_storage_array_simple.sol diff --git a/test/libsolidity/syntaxTests/largeTypes/large_storage_arrays_combined.sol b/test/libsolidity/syntaxTests/largeTypes/large_storage_arrays_combined.sol new file mode 100644 index 000000000..eee56ce0f --- /dev/null +++ b/test/libsolidity/syntaxTests/largeTypes/large_storage_arrays_combined.sol @@ -0,0 +1,4 @@ +contract C { + uint[200][200][2**30][][2**30] x; +} +// ---- diff --git a/test/libsolidity/syntaxTests/nameAndTypeResolution/509_large_storage_arrays_struct.sol b/test/libsolidity/syntaxTests/largeTypes/large_storage_arrays_struct.sol similarity index 100% rename from test/libsolidity/syntaxTests/nameAndTypeResolution/509_large_storage_arrays_struct.sol rename to test/libsolidity/syntaxTests/largeTypes/large_storage_arrays_struct.sol diff --git a/test/libsolidity/syntaxTests/largeTypes/large_storage_structs.sol b/test/libsolidity/syntaxTests/largeTypes/large_storage_structs.sol new file mode 100644 index 000000000..e7057e7a6 --- /dev/null +++ b/test/libsolidity/syntaxTests/largeTypes/large_storage_structs.sol @@ -0,0 +1,18 @@ +contract C { + struct P0 { uint256[2**63] x; } + struct S0 { + P0[2**62] y; + P0 x; + } + S0 s0; + + struct P1 { uint256[2**63] x; } + struct S1 { + P1 x; + P1[2**62] y; + } + S1 s1; +} +// ---- +// Warning 3408: (110-115): Variable covers a large part of storage and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. +// Warning 3408: (215-220): Variable covers a large part of storage and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. diff --git a/test/libsolidity/syntaxTests/nameAndTypeResolution/508_large_storage_arrays_combined.sol b/test/libsolidity/syntaxTests/nameAndTypeResolution/508_large_storage_arrays_combined.sol deleted file mode 100644 index 366f35339..000000000 --- a/test/libsolidity/syntaxTests/nameAndTypeResolution/508_large_storage_arrays_combined.sol +++ /dev/null @@ -1,5 +0,0 @@ -contract C { - uint[200][200][2**30][][2**30] x; -} -// ---- -// Warning 3408: (17-49): Variable covers a large part of storage and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. diff --git a/test/libsolidity/syntaxTests/nameAndTypeResolution/510_large_storage_array_mapping.sol b/test/libsolidity/syntaxTests/nameAndTypeResolution/510_large_storage_array_mapping.sol deleted file mode 100644 index e639b58e9..000000000 --- a/test/libsolidity/syntaxTests/nameAndTypeResolution/510_large_storage_array_mapping.sol +++ /dev/null @@ -1,5 +0,0 @@ -contract C { - mapping(uint => uint[2**100]) x; -} -// ---- -// Warning 3408: (17-48): Variable covers a large part of storage and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. From 1c7a0dcbea3084f5b75eaa3b4e635abaa4218970 Mon Sep 17 00:00:00 2001 From: a3d4 Date: Thu, 2 Jul 2020 06:03:26 +0200 Subject: [PATCH 2/2] Add warnings for oversized subtypes --- libsolidity/analysis/StaticAnalyzer.cpp | 35 +++++++++-- libsolidity/analysis/StaticAnalyzer.h | 16 ++++- .../large_storage_array_mapping.sol | 1 + .../largeTypes/large_storage_structs.sol | 63 ++++++++++++++++--- 4 files changed, 101 insertions(+), 14 deletions(-) diff --git a/libsolidity/analysis/StaticAnalyzer.cpp b/libsolidity/analysis/StaticAnalyzer.cpp index 5524b3a68..bee1197b3 100644 --- a/libsolidity/analysis/StaticAnalyzer.cpp +++ b/libsolidity/analysis/StaticAnalyzer.cpp @@ -158,7 +158,8 @@ bool StaticAnalyzer::visit(VariableDeclaration const& _variable) else if (_variable.isStateVariable()) { set structsSeen; - if (structureSizeEstimate(*_variable.type(), structsSeen) >= bigint(1) << 64) + TypeSet oversizedSubTypes; + if (structureSizeEstimate(*_variable.type(), structsSeen, oversizedSubTypes) >= bigint(1) << 64) m_errorReporter.warning( 3408_error, _variable.location(), @@ -166,6 +167,14 @@ bool StaticAnalyzer::visit(VariableDeclaration const& _variable) "Either use mappings or dynamic arrays and allow their size to be increased only " "in small quantities per transaction." ); + for (Type const* type: oversizedSubTypes) + m_errorReporter.warning( + 7325_error, + _variable.location(), + "Type " + type->canonicalName() + " has large size and thus makes collisions likely. " + "Either use mappings or dynamic arrays and allow their size to be increased only " + "in small quantities per transaction." + ); } return true; } @@ -339,28 +348,44 @@ bool StaticAnalyzer::visit(FunctionCall const& _functionCall) return true; } -bigint StaticAnalyzer::structureSizeEstimate(Type const& _type, set& _structsSeen) +bigint StaticAnalyzer::structureSizeEstimate( + Type const& _type, + set& _structsSeen, + TypeSet& _oversizedSubTypes +) { switch (_type.category()) { case Type::Category::Array: { auto const& t = dynamic_cast(_type); + bigint baseTypeSize = structureSizeEstimate(*t.baseType(), _structsSeen, _oversizedSubTypes); + if (baseTypeSize >= bigint(1) << 64) + _oversizedSubTypes.insert(t.baseType()); if (!t.isDynamicallySized()) - return structureSizeEstimate(*t.baseType(), _structsSeen) * t.length(); + return structureSizeEstimate(*t.baseType(), _structsSeen, _oversizedSubTypes) * t.length(); break; } case Type::Category::Struct: { auto const& t = dynamic_cast(_type); - solAssert(!_structsSeen.count(&t.structDefinition()), "Recursive struct."); bigint size = 1; + if (_structsSeen.count(&t.structDefinition())) + return size; _structsSeen.insert(&t.structDefinition()); for (auto const& m: t.members(nullptr)) - size += structureSizeEstimate(*m.type, _structsSeen); + size += structureSizeEstimate(*m.type, _structsSeen, _oversizedSubTypes); _structsSeen.erase(&t.structDefinition()); return size; } + case Type::Category::Mapping: + { + auto const* valueType = dynamic_cast(_type).valueType(); + bigint valueTypeSize = structureSizeEstimate(*valueType, _structsSeen, _oversizedSubTypes); + if (valueTypeSize >= bigint(1) << 64) + _oversizedSubTypes.insert(valueType); + break; + } default: break; } diff --git a/libsolidity/analysis/StaticAnalyzer.h b/libsolidity/analysis/StaticAnalyzer.h index 57ac54fb1..8f7fbc597 100644 --- a/libsolidity/analysis/StaticAnalyzer.h +++ b/libsolidity/analysis/StaticAnalyzer.h @@ -73,8 +73,22 @@ private: bool visit(BinaryOperation const& _operation) override; bool visit(FunctionCall const& _functionCall) override; + struct TypeComp + { + bool operator()(Type const* lhs, Type const* rhs) const + { + solAssert(lhs && rhs, ""); + return lhs->richIdentifier() < rhs->richIdentifier(); + } + }; + using TypeSet = std::set; + /// @returns the size of this type in storage, including all sub-types. - static bigint structureSizeEstimate(Type const& _type, std::set& _structsSeen); + static bigint structureSizeEstimate( + Type const& _type, + std::set& _structsSeen, + TypeSet& _oversizedSubTypes + ); langutil::ErrorReporter& m_errorReporter; diff --git a/test/libsolidity/syntaxTests/largeTypes/large_storage_array_mapping.sol b/test/libsolidity/syntaxTests/largeTypes/large_storage_array_mapping.sol index 34132bfbe..0471307ef 100644 --- a/test/libsolidity/syntaxTests/largeTypes/large_storage_array_mapping.sol +++ b/test/libsolidity/syntaxTests/largeTypes/large_storage_array_mapping.sol @@ -2,3 +2,4 @@ contract C { mapping(uint => uint[2**100]) x; } // ---- +// Warning 7325: (17-48): Type uint256[1267650600228229401496703205376] has large size and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. diff --git a/test/libsolidity/syntaxTests/largeTypes/large_storage_structs.sol b/test/libsolidity/syntaxTests/largeTypes/large_storage_structs.sol index e7057e7a6..17113fc7b 100644 --- a/test/libsolidity/syntaxTests/largeTypes/large_storage_structs.sol +++ b/test/libsolidity/syntaxTests/largeTypes/large_storage_structs.sol @@ -1,18 +1,65 @@ contract C { - struct P0 { uint256[2**63] x; } + struct P { uint256[2**63] x; } + struct S0 { - P0[2**62] y; - P0 x; + P[2**62] x; + P y; } S0 s0; - struct P1 { uint256[2**63] x; } struct S1 { - P1 x; - P1[2**62] y; + P x; + P[2**62] y; } S1 s1; + + struct S2 { + mapping(uint => P[2**62]) x; + mapping(uint => P[2**62]) y; + mapping(uint => S2) z; + } + S2 s2; + + struct Q0 + { + uint[1][][2**65] x; + uint[2**65][][1] y; + uint[][2**65] z; + uint[2**65][] t; + } + Q0 q0; + + struct Q1 + { + uint[1][][2**65] x; + } + Q1 q1; + + struct Q2 + { + uint[2**65][][1] y; + } + Q2 q2; + + struct Q3 + { + uint[][2**65] z; + } + Q3 q3; + + struct Q4 + { + uint[2**65][] t; + } + Q4 q4; } // ---- -// Warning 3408: (110-115): Variable covers a large part of storage and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. -// Warning 3408: (215-220): Variable covers a large part of storage and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. +// Warning 3408: (108-113): Variable covers a large part of storage and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. +// Warning 3408: (175-180): Variable covers a large part of storage and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. +// Warning 7325: (314-319): Type C.P[4611686018427387904] has large size and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. +// Warning 3408: (458-463): Variable covers a large part of storage and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. +// Warning 7325: (458-463): Type uint256[36893488147419103232] has large size and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. +// Warning 3408: (524-529): Variable covers a large part of storage and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. +// Warning 7325: (590-595): Type uint256[36893488147419103232] has large size and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. +// Warning 3408: (653-658): Variable covers a large part of storage and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction. +// Warning 7325: (716-721): Type uint256[36893488147419103232] has large size and thus makes collisions likely. Either use mappings or dynamic arrays and allow their size to be increased only in small quantities per transaction.