Merge pull request #3868 from ethereum/bytescleanup

Properly force-clean for shortening bytesXX conversions.

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.

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

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<FixedBytesType const&>(_targetType);
-			if (targetType.numBytes() > typeOnStack.numBytes() || _cleanupNeeded)
+			if (typeOnStack.numBytes() == 0 || targetType.numBytes() == 0)
-			{
-				if (typeOnStack.numBytes() == 0)
 				m_context << Instruction::POP << u256(0);
-				else
+			else if (targetType.numBytes() > typeOnStack.numBytes() || _cleanupNeeded)
 			{
-					m_context << ((u256(1) << (256 - typeOnStack.numBytes() * 8)) - 1);
+				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<FixedBytesType const&>(_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)
 		{

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: <memory pointer> <selector>
@@ -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;
 			}

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)