From 0aaa6d876d56e450ce907e5a8b1db6cfd220312f Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 22 Jun 2017 18:25:22 +0200 Subject: Use for loop for assembly memcopy. --- libsolidity/codegen/CompilerUtils.cpp | 38 ++++++++++++++--------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/libsolidity/codegen/CompilerUtils.cpp b/libsolidity/codegen/CompilerUtils.cpp index 7fed1975..26eadf9d 100644 --- a/libsolidity/codegen/CompilerUtils.cpp +++ b/libsolidity/codegen/CompilerUtils.cpp @@ -305,15 +305,9 @@ void CompilerUtils::memoryCopy32() m_context.appendInlineAssembly(R"( { - jumpi(end, eq(len, 0)) - start: - mstore(dst, mload(src)) - jumpi(end, iszero(gt(len, 32))) - dst := add(dst, 32) - src := add(src, 32) - len := sub(len, 32) - jump(start) - end: + for { let i := 0 } lt(i, len) { i := add(i, 32) } { + mstore(add(dst, i), mload(add(src, i))) + } } )", { "len", "dst", "src" } @@ -327,21 +321,19 @@ void CompilerUtils::memoryCopy() m_context.appendInlineAssembly(R"( { - // copy 32 bytes at once - start32: - jumpi(end32, lt(len, 32)) - mstore(dst, mload(src)) - dst := add(dst, 32) - src := add(src, 32) - len := sub(len, 32) - jump(start32) - end32: + // copy 32 bytes at once + for {} iszero(lt(len, 32)) { + dst := add(dst, 32) + src := add(src, 32) + len := sub(len, 32) + } + { mstore(dst, mload(src)) } - // copy the remainder (0 < len < 32) - let mask := sub(exp(256, sub(32, len)), 1) - let srcpart := and(mload(src), not(mask)) - let dstpart := and(mload(dst), mask) - mstore(dst, or(srcpart, dstpart)) + // copy the remainder (0 < len < 32) + let mask := sub(exp(256, sub(32, len)), 1) + let srcpart := and(mload(src), not(mask)) + let dstpart := and(mload(dst), mask) + mstore(dst, or(srcpart, dstpart)) } )", { "len", "dst", "src" } -- cgit v1.2.3 From d94a12a34c4c3846a40002a4e3a79c2b1f0b6972 Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 26 Jun 2017 09:19:11 +0200 Subject: Reformat. --- libsolidity/codegen/CompilerUtils.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/libsolidity/codegen/CompilerUtils.cpp b/libsolidity/codegen/CompilerUtils.cpp index 26eadf9d..4edec155 100644 --- a/libsolidity/codegen/CompilerUtils.cpp +++ b/libsolidity/codegen/CompilerUtils.cpp @@ -322,12 +322,15 @@ void CompilerUtils::memoryCopy() m_context.appendInlineAssembly(R"( { // copy 32 bytes at once - for {} iszero(lt(len, 32)) { - dst := add(dst, 32) - src := add(src, 32) - len := sub(len, 32) - } - { mstore(dst, mload(src)) } + for + {} + iszero(lt(len, 32)) + { + dst := add(dst, 32) + src := add(src, 32) + len := sub(len, 32) + } + { mstore(dst, mload(src)) } // copy the remainder (0 < len < 32) let mask := sub(exp(256, sub(32, len)), 1) -- cgit v1.2.3 From d2445dfdced2d4e1fccdd873963e02663c0116c1 Mon Sep 17 00:00:00 2001 From: chriseth Date: Fri, 23 Jun 2017 18:36:56 +0200 Subject: Tests for comparison of non-comparable types. --- test/libsolidity/SolidityNameAndTypeResolution.cpp | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index a6027812..0c56e585 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -554,6 +554,40 @@ BOOST_AUTO_TEST_CASE(comparison_bitop_precedence) CHECK_SUCCESS(text); } +BOOST_AUTO_TEST_CASE(comparison_of_function_types) +{ + char const* text = R"( + contract C { + function f() returns (bool ret) { + return this.f < this.f; + } + } + )"; + CHECK_ERROR(text, TypeError, "Operator < not compatible"); + text = R"( + contract C { + function f() returns (bool ret) { + return f < f; + } + } + )"; + CHECK_SUCCESS(text); +} + +BOOST_AUTO_TEST_CASE(comparison_of_mapping_types) +{ + char const* text = R"( + contract C { + mapping(uint => uint) x; + function f() returns (bool ret) { + var y = x; + return x == y; + } + } + )"; + CHECK_ERROR(text, TypeError, "Operator == not compatible"); +} + BOOST_AUTO_TEST_CASE(function_no_implementation) { ASTPointer sourceUnit; -- cgit v1.2.3 From f47e6e90fb55066ff602a1448fc2d3f650449559 Mon Sep 17 00:00:00 2001 From: chriseth Date: Fri, 23 Jun 2017 18:37:15 +0200 Subject: Disallow comparisons between some types. --- Changelog.md | 1 + libsolidity/ast/Types.cpp | 10 ++++++++++ libsolidity/ast/Types.h | 9 ++++----- libsolidity/codegen/ExpressionCompiler.cpp | 1 + 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/Changelog.md b/Changelog.md index cfedf1fc..8eaa8271 100644 --- a/Changelog.md +++ b/Changelog.md @@ -19,6 +19,7 @@ Bugfixes: * Type Checker: Fix address literals not being treated as compile-time constants. * Type Checker: Disallow invoking the same modifier multiple times. * Type Checker: Make UTF8-validation a bit more sloppy to include more valid sequences. + * Type Checker: Disallow comparisons between mapping and non-internal function types. * Type Checker: Do not treat strings that look like addresses as addresses. * Fixed crash concerning non-callable types. * Unused variable warnings no longer issued for variables used inside inline assembly. diff --git a/libsolidity/ast/Types.cpp b/libsolidity/ast/Types.cpp index bd3346f9..0234f842 100644 --- a/libsolidity/ast/Types.cpp +++ b/libsolidity/ast/Types.cpp @@ -2248,6 +2248,16 @@ TypePointer FunctionType::unaryOperatorResult(Token::Value _operator) const return TypePointer(); } +TypePointer FunctionType::binaryOperatorResult(Token::Value _operator, TypePointer const& _other) const +{ + if (_other->category() != category() || !Token::isCompareOp(_operator)) + return TypePointer(); + FunctionType const& other = dynamic_cast(*_other); + if (kind() == Kind::Internal && other.kind() == Kind::Internal && sizeOnStack() == 1 && other.sizeOnStack() == 1) + return commonType(shared_from_this(), _other); + return TypePointer(); +} + string FunctionType::canonicalName(bool) const { solAssert(m_kind == Kind::External, ""); diff --git a/libsolidity/ast/Types.h b/libsolidity/ast/Types.h index c4ffc44c..f7a73ab5 100644 --- a/libsolidity/ast/Types.h +++ b/libsolidity/ast/Types.h @@ -933,6 +933,7 @@ public: virtual bool operator==(Type const& _other) const override; virtual bool isExplicitlyConvertibleTo(Type const& _convertTo) const override; virtual TypePointer unaryOperatorResult(Token::Value _operator) const override; + virtual TypePointer binaryOperatorResult(Token::Value, TypePointer const&) const override; virtual std::string canonicalName(bool /*_addDataLocation*/) const override; virtual std::string toString(bool _short) const override; virtual unsigned calldataEncodedSize(bool _padded) const override; @@ -1038,6 +1039,7 @@ public: virtual std::string toString(bool _short) const override; virtual std::string canonicalName(bool _addDataLocation) const override; virtual bool canLiveOutsideStorage() const override { return false; } + virtual TypePointer binaryOperatorResult(Token::Value, TypePointer const&) const override { return TypePointer(); } virtual TypePointer encodingType() const override { return std::make_shared(256); @@ -1116,11 +1118,7 @@ public: explicit ModuleType(SourceUnit const& _source): m_sourceUnit(_source) {} - virtual TypePointer binaryOperatorResult(Token::Value, TypePointer const&) const override - { - return TypePointer(); - } - + virtual TypePointer binaryOperatorResult(Token::Value, TypePointer const&) const override { return TypePointer(); } virtual std::string identifier() const override; virtual bool operator==(Type const& _other) const override; virtual bool canBeStored() const override { return false; } @@ -1178,6 +1176,7 @@ public: virtual std::string identifier() const override { return "t_inaccessible"; } virtual bool isImplicitlyConvertibleTo(Type const&) const override { return false; } virtual bool isExplicitlyConvertibleTo(Type const&) const override { return false; } + virtual TypePointer binaryOperatorResult(Token::Value, TypePointer const&) const override { return TypePointer(); } virtual unsigned calldataEncodedSize(bool _padded) const override { (void)_padded; return 32; } virtual bool canBeStored() const override { return false; } virtual bool canLiveOutsideStorage() const override { return false; } diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index a7cfe4dc..a65549fd 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -1366,6 +1366,7 @@ void ExpressionCompiler::appendAndOrOperatorCode(BinaryOperation const& _binaryO void ExpressionCompiler::appendCompareOperatorCode(Token::Value _operator, Type const& _type) { + solAssert(_type.sizeOnStack() == 1, "Comparison of multi-slot types."); if (_operator == Token::Equal || _operator == Token::NotEqual) { if (FunctionType const* funType = dynamic_cast(&_type)) -- cgit v1.2.3 From 4407a13c1730bf9ed4bcaf00c3f72640f6ddb2a7 Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 26 Jun 2017 16:30:13 +0200 Subject: Only allow equality checks for internal function types. --- libsolidity/ast/Types.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libsolidity/ast/Types.cpp b/libsolidity/ast/Types.cpp index 0234f842..7dc6c4a6 100644 --- a/libsolidity/ast/Types.cpp +++ b/libsolidity/ast/Types.cpp @@ -2250,7 +2250,7 @@ TypePointer FunctionType::unaryOperatorResult(Token::Value _operator) const TypePointer FunctionType::binaryOperatorResult(Token::Value _operator, TypePointer const& _other) const { - if (_other->category() != category() || !Token::isCompareOp(_operator)) + if (_other->category() != category() || !(_operator == Token::Equal || _operator == Token::NotEqual)) return TypePointer(); FunctionType const& other = dynamic_cast(*_other); if (kind() == Kind::Internal && other.kind() == Kind::Internal && sizeOnStack() == 1 && other.sizeOnStack() == 1) -- cgit v1.2.3 From d0b6de0b346b319c85747fdbee76c1d204d6ced6 Mon Sep 17 00:00:00 2001 From: chriseth Date: Wed, 21 Jun 2017 19:32:56 +0200 Subject: Warn about copies in storage that might overwrite unexpectedly. --- Changelog.md | 1 + libsolidity/analysis/TypeChecker.cpp | 31 ++++++++++++++++ libsolidity/analysis/TypeChecker.h | 3 ++ test/libsolidity/SolidityNameAndTypeResolution.cpp | 42 ++++++++++++++++++++++ 4 files changed, 77 insertions(+) diff --git a/Changelog.md b/Changelog.md index cfedf1fc..183f79ab 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(*type(_assignment.leftHandSide())); + TupleType const& rhs = dynamic_cast(*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(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(&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(); 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/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index a6027812..6af5f503 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -5771,6 +5771,48 @@ 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(warn_unused_local) { char const* text = R"( -- cgit v1.2.3 From 1a3066c3a1a9a9cbd16cd43e2725a4b03fc547cf Mon Sep 17 00:00:00 2001 From: chriseth Date: Thu, 22 Jun 2017 17:24:19 +0200 Subject: Test about semantics of "swap" in storage. --- test/libsolidity/SolidityEndToEndTest.cpp | 32 +++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) 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"( -- cgit v1.2.3 From 336c9e8f321c9829340affbe57ae4f2c07fad4aa Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 26 Jun 2017 16:35:44 +0200 Subject: Some more tests. --- test/libsolidity/SolidityNameAndTypeResolution.cpp | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index 6af5f503..7961dbc4 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -5813,6 +5813,38 @@ BOOST_AUTO_TEST_CASE(warn_multiple_storage_storage_copies_fill_left) 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"( -- cgit v1.2.3 From ab15040caa0b2b401f9bebc8d8247e8e6b6d1540 Mon Sep 17 00:00:00 2001 From: chriseth Date: Mon, 26 Jun 2017 16:27:12 +0200 Subject: Comment about zero length. --- libsolidity/codegen/CompilerUtils.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libsolidity/codegen/CompilerUtils.h b/libsolidity/codegen/CompilerUtils.h index caf2cdc2..0ee053a9 100644 --- a/libsolidity/codegen/CompilerUtils.h +++ b/libsolidity/codegen/CompilerUtils.h @@ -110,10 +110,12 @@ public: void zeroInitialiseMemoryArray(ArrayType const& _type); /// Copies full 32 byte words in memory (regions cannot overlap), i.e. may copy more than length. + /// Length can be zero, in this case, it copies nothing. /// Stack pre: /// Stack post: void memoryCopy32(); /// Copies data in memory (regions cannot overlap). + /// Length can be zero, in this case, it copies nothing. /// Stack pre: /// Stack post: void memoryCopy(); -- cgit v1.2.3 From 6b05bbbbb42dafdbf38661fd9c2c3c3e88a425a2 Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Mon, 26 Jun 2017 22:01:35 +0100 Subject: Update tests for function type comparison --- test/libsolidity/SolidityNameAndTypeResolution.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/libsolidity/SolidityNameAndTypeResolution.cpp b/test/libsolidity/SolidityNameAndTypeResolution.cpp index 0c56e585..eb1cf0dc 100644 --- a/test/libsolidity/SolidityNameAndTypeResolution.cpp +++ b/test/libsolidity/SolidityNameAndTypeResolution.cpp @@ -571,6 +571,17 @@ BOOST_AUTO_TEST_CASE(comparison_of_function_types) } } )"; + CHECK_ERROR(text, TypeError, "Operator < not compatible"); + text = R"( + contract C { + function f() returns (bool ret) { + return f == f; + } + function g() returns (bool ret) { + return f != f; + } + } + )"; CHECK_SUCCESS(text); } -- cgit v1.2.3