diff options
| -rw-r--r-- | Changelog.md | 1 | ||||
| -rw-r--r-- | libsolidity/analysis/TypeChecker.cpp | 31 | ||||
| -rw-r--r-- | libsolidity/analysis/TypeChecker.h | 3 | ||||
| -rw-r--r-- | test/libsolidity/SolidityEndToEndTest.cpp | 32 | ||||
| -rw-r--r-- | test/libsolidity/SolidityNameAndTypeResolution.cpp | 74 |
5 files changed, 141 insertions, 0 deletions
diff --git a/Changelog.md b/Changelog.md index 8eaa8271..6d9fe477 100644 --- a/Changelog.md +++ b/Changelog.md @@ -11,6 +11,7 @@ Features: * Inline Assembly: introduce ``keccak256`` as an opcode. ``sha3`` is still a valid alias. * Inline Assembly: ``for`` and ``switch`` statements. * Inline Assembly: function definitions and function calls. + * Type Checker: Warn about copies in storage that might overwrite unexpectedly. * Code Generator: Added the Whiskers template system. * Remove obsolete Why3 output. diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 40ff59f6..ef8a9345 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -364,6 +364,35 @@ void TypeChecker::checkLibraryRequirements(ContractDefinition const& _contract) m_errorReporter.typeError(var->location(), "Library cannot have non-constant state variables"); } +void TypeChecker::checkDoubleStorageAssignment(Assignment const& _assignment) +{ + TupleType const& lhs = dynamic_cast<TupleType const&>(*type(_assignment.leftHandSide())); + TupleType const& rhs = dynamic_cast<TupleType const&>(*type(_assignment.rightHandSide())); + + bool fillRight = !lhs.components().empty() && (!lhs.components().back() || lhs.components().front()); + size_t storageToStorageCopies = 0; + size_t toStorageCopies = 0; + for (size_t i = 0; i < lhs.components().size(); ++i) + { + ReferenceType const* ref = dynamic_cast<ReferenceType const*>(lhs.components()[i].get()); + if (!ref || !ref->dataStoredIn(DataLocation::Storage) || ref->isPointer()) + continue; + size_t rhsPos = fillRight ? i : rhs.components().size() - (lhs.components().size() - i); + solAssert(rhsPos < rhs.components().size(), ""); + toStorageCopies++; + if (rhs.components()[rhsPos]->dataStoredIn(DataLocation::Storage)) + storageToStorageCopies++; + } + if (storageToStorageCopies >= 1 && toStorageCopies >= 2) + m_errorReporter.warning( + _assignment.location(), + "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." + ); +} + void TypeChecker::endVisit(InheritanceSpecifier const& _inheritance) { auto base = dynamic_cast<ContractDefinition const*>(&dereference(_inheritance.name())); @@ -1047,6 +1076,8 @@ bool TypeChecker::visit(Assignment const& _assignment) // Sequenced assignments of tuples is not valid, make the result a "void" type. _assignment.annotation().type = make_shared<TupleType>(); expectType(_assignment.rightHandSide(), *tupleType); + + checkDoubleStorageAssignment(_assignment); } else if (t->category() == Type::Category::Mapping) { diff --git a/libsolidity/analysis/TypeChecker.h b/libsolidity/analysis/TypeChecker.h index 2fa66f97..ee43d13a 100644 --- a/libsolidity/analysis/TypeChecker.h +++ b/libsolidity/analysis/TypeChecker.h @@ -69,6 +69,9 @@ private: void checkContractExternalTypeClashes(ContractDefinition const& _contract); /// Checks that all requirements for a library are fulfilled if this is a library. void checkLibraryRequirements(ContractDefinition const& _contract); + /// Checks (and warns) if a tuple assignment might cause unexpected overwrites in storage. + /// Should only be called if the left hand side is tuple-typed. + void checkDoubleStorageAssignment(Assignment const& _assignment); virtual void endVisit(InheritanceSpecifier const& _inheritance) override; virtual void endVisit(UsingForDirective const& _usingFor) override; diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index 823a8eda..ffee5e36 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -4483,6 +4483,38 @@ BOOST_AUTO_TEST_CASE(array_copy_including_mapping) BOOST_CHECK(storageEmpty(m_contractAddress)); } +BOOST_AUTO_TEST_CASE(swap_in_storage_overwrite) +{ + // This tests a swap in storage which does not work as one + // might expect because we do not have temporary storage. + // (x, y) = (y, x) is the same as + // y = x; + // x = y; + char const* sourceCode = R"( + contract c { + struct S { uint a; uint b; } + S public x; + S public y; + function set() { + x.a = 1; x.b = 2; + y.a = 3; y.b = 4; + } + function swap() { + (x, y) = (y, x); + } + } + )"; + compileAndRun(sourceCode); + BOOST_CHECK(callContractFunction("x()") == encodeArgs(u256(0), u256(0))); + BOOST_CHECK(callContractFunction("y()") == encodeArgs(u256(0), u256(0))); + BOOST_CHECK(callContractFunction("set()") == encodeArgs()); + BOOST_CHECK(callContractFunction("x()") == encodeArgs(u256(1), u256(2))); + BOOST_CHECK(callContractFunction("y()") == encodeArgs(u256(3), u256(4))); + BOOST_CHECK(callContractFunction("swap()") == encodeArgs()); + BOOST_CHECK(callContractFunction("x()") == encodeArgs(u256(1), u256(2))); + BOOST_CHECK(callContractFunction("y()") == encodeArgs(u256(1), u256(2))); +} + BOOST_AUTO_TEST_CASE(pass_dynamic_arguments_to_the_base) { char const* sourceCode = R"( diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index eb1cf0dc..b489be22 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -5816,6 +5816,80 @@ BOOST_AUTO_TEST_CASE(pure_statement_check_for_regular_for_loop) success(text); } +BOOST_AUTO_TEST_CASE(warn_multiple_storage_storage_copies) +{ + char const* text = R"( + contract C { + struct S { uint a; uint b; } + S x; S y; + function f() { + (x, y) = (y, x); + } + } + )"; + CHECK_WARNING(text, "This assignment performs two copies to storage."); +} + +BOOST_AUTO_TEST_CASE(warn_multiple_storage_storage_copies_fill_right) +{ + char const* text = R"( + contract C { + struct S { uint a; uint b; } + S x; S y; + function f() { + (x, y, ) = (y, x, 1, 2); + } + } + )"; + CHECK_WARNING(text, "This assignment performs two copies to storage."); +} + +BOOST_AUTO_TEST_CASE(warn_multiple_storage_storage_copies_fill_left) +{ + char const* text = R"( + contract C { + struct S { uint a; uint b; } + S x; S y; + function f() { + (,x, y) = (1, 2, y, x); + } + } + )"; + CHECK_WARNING(text, "This assignment performs two copies to storage."); +} + +BOOST_AUTO_TEST_CASE(nowarn_swap_memory) +{ + char const* text = R"( + contract C { + struct S { uint a; uint b; } + function f() { + S memory x; + S memory y; + (x, y) = (y, x); + } + } + )"; + CHECK_SUCCESS_NO_WARNINGS(text); +} + +BOOST_AUTO_TEST_CASE(nowarn_swap_storage_pointers) +{ + char const* text = R"( + contract C { + struct S { uint a; uint b; } + S x; S y; + function f() { + S storage x_local = x; + S storage y_local = y; + S storage z_local = x; + (x, y_local, x_local, z_local) = (y, x_local, y_local, y); + } + } + )"; + CHECK_SUCCESS_NO_WARNINGS(text); +} + BOOST_AUTO_TEST_CASE(warn_unused_local) { char const* text = R"( |