diff --git a/Changelog.md b/Changelog.md index 9d1c835de..431a7db77 100644 --- a/Changelog.md +++ b/Changelog.md @@ -30,6 +30,7 @@ Bugfixes: * Code Generator: Bugfix in modifier lookup in libraries. * Code Generator: Implement packed encoding of external function types. * Code Generator: Treat empty base constructor argument list as not provided. + * Code Generator: Properly force-clean bytesXX types for shortening conversions. * Commandline interface: Fix error messages for imported files that do not exist. * Commandline interface: Support ``--evm-version constantinople`` properly. * DocString Parser: Fix error message for empty descriptions. diff --git a/docs/assembly.rst b/docs/assembly.rst index 705cd1b84..978e71e33 100644 --- a/docs/assembly.rst +++ b/docs/assembly.rst @@ -405,6 +405,16 @@ changes during the call, and thus references to local variables will be wrong. } } +.. note:: + If you access variables of a type that spans less than 256 bits + (for example ``uint64``, ``address``, ``bytes16`` or ``byte``), + you cannot make any assumptions about bits not part of the + encoding of the type. Especially, do not assume them to be zero. + To be safe, always clear the data properly before you use it + in a context where this is important: + ``uint32 x = f(); assembly { x := and(x, 0xffffffff) /* now use x */ }`` + To clean signed types, you can use the ``signextend`` opcode. + Labels ------ diff --git a/libsolidity/codegen/CompilerUtils.cpp b/libsolidity/codegen/CompilerUtils.cpp index b4550153e..4af7d9051 100644 --- a/libsolidity/codegen/CompilerUtils.cpp +++ b/libsolidity/codegen/CompilerUtils.cpp @@ -684,19 +684,17 @@ void CompilerUtils::convertType( // clear for conversion to longer bytes solAssert(targetTypeCategory == Type::Category::FixedBytes, "Invalid type conversion requested."); FixedBytesType const& targetType = dynamic_cast(_targetType); - if (targetType.numBytes() > typeOnStack.numBytes() || _cleanupNeeded) + if (typeOnStack.numBytes() == 0 || targetType.numBytes() == 0) + m_context << Instruction::POP << u256(0); + else if (targetType.numBytes() > typeOnStack.numBytes() || _cleanupNeeded) { - if (typeOnStack.numBytes() == 0) - m_context << Instruction::POP << u256(0); - else - { - m_context << ((u256(1) << (256 - typeOnStack.numBytes() * 8)) - 1); - m_context << Instruction::NOT << Instruction::AND; - } + int bytes = min(typeOnStack.numBytes(), targetType.numBytes()); + m_context << ((u256(1) << (256 - bytes * 8)) - 1); + m_context << Instruction::NOT << Instruction::AND; } } - } break; + } case Type::Category::Enum: solAssert(_targetType == _typeOnStack || targetTypeCategory == Type::Category::Integer, ""); if (enumOverflowCheckPending) @@ -798,8 +796,9 @@ void CompilerUtils::convertType( bytesConstRef data(value); if (targetTypeCategory == Type::Category::FixedBytes) { + int const numBytes = dynamic_cast(_targetType).numBytes(); solAssert(data.size() <= 32, ""); - m_context << h256::Arith(h256(data, h256::AlignLeft)); + m_context << (h256::Arith(h256(data, h256::AlignLeft)) & (~(u256(-1) >> (8 * numBytes)))); } else if (targetTypeCategory == Type::Category::Array) { diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index ed5af42ec..3cf46a9d0 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -1023,7 +1023,6 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) solAssert(function.kind() == FunctionType::Kind::ABIEncodeWithSelector, ""); } - // Cleanup actually does not clean on shrinking the type. utils().convertType(*dataOnStack, FixedBytesType(4), true); // stack: @@ -1034,7 +1033,7 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall) let data_start := add(mem_ptr, 0x20) let data := mload(data_start) let mask := )" + mask + R"( - mstore(data_start, or(and(data, mask), and(selector, not(mask)))) + mstore(data_start, or(and(data, mask), selector)) })", {"mem_ptr", "selector"}); m_context << Instruction::POP; } diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index 8440449cb..789b89d14 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -8830,6 +8830,24 @@ BOOST_AUTO_TEST_CASE(cleanup_bytes_types) ABI_CHECK(callContractFunction("f(bytes2,uint16)", string("abc"), u256(0x040102)), encodeArgs(0)); } +BOOST_AUTO_TEST_CASE(cleanup_bytes_types_shortening) +{ + char const* sourceCode = R"( + contract C { + function f() pure returns (bytes32 r) { + bytes4 x = 0xffffffff; + bytes2 y = bytes2(x); + assembly { r := y } + // At this point, r and y both store four bytes, but + // y is properly cleaned before the equality check + require(y == bytes2(0xffff)); + } + } + )"; + compileAndRun(sourceCode, 0, "C"); + ABI_CHECK(callContractFunction("f()"), encodeArgs("\xff\xff\xff\xff")); +} + BOOST_AUTO_TEST_CASE(skip_dynamic_types) { // The EVM cannot provide access to dynamically-sized return values, so we have to skip them. @@ -11345,6 +11363,10 @@ BOOST_AUTO_TEST_CASE(abi_encode) y[0] = "e"; require(y[0] == "e"); } + function f4() returns (bytes) { + bytes4 x = "abcd"; + return abi.encode(bytes2(x)); + } } )"; compileAndRun(sourceCode, 0, "C"); @@ -11352,6 +11374,7 @@ BOOST_AUTO_TEST_CASE(abi_encode) ABI_CHECK(callContractFunction("f1()"), encodeArgs(0x20, 0x40, 1, 2)); ABI_CHECK(callContractFunction("f2()"), encodeArgs(0x20, 0xa0, 1, 0x60, 2, 3, "abc")); ABI_CHECK(callContractFunction("f3()"), encodeArgs(0x20, 0xa0, 1, 0x60, 2, 3, "abc")); + ABI_CHECK(callContractFunction("f4()"), encodeArgs(0x20, 0x20, "ab")); } BOOST_AUTO_TEST_CASE(abi_encode_v2)