Merge pull request #105 from chriseth/fix_calldataUnpacker

Bugfix in calldata unpacker.

4 files changed, 47 insertions, 34 deletions

diff --git a/libsolidity/Compiler.cpp b/libsolidity/Compiler.cpp
index cb74072d..969c8f74 100644
--- a/libsolidity/Compiler.cpp
+++ b/libsolidity/Compiler.cpp
@@ -210,13 +210,10 @@ void Compiler::appendConstructor(FunctionDefinition const& _constructor)
 		m_context << eth::Instruction::DUP1;
 		m_context.appendProgramSize();
 		m_context << eth::Instruction::DUP4 << eth::Instruction::CODECOPY;
-		m_context << eth::Instruction::ADD;
+		m_context << eth::Instruction::DUP2 << eth::Instruction::ADD;
 		CompilerUtils(m_context).storeFreeMemoryPointer();
-		appendCalldataUnpacker(
-			FunctionType(_constructor).parameterTypes(),
-			true,
-			CompilerUtils::freeMemoryPointer + 0x20
-		);
+		// stack: <memptr>
+		appendCalldataUnpacker(FunctionType(_constructor).parameterTypes(), true);
 	}
 	_constructor.accept(*this);
 }
@@ -267,6 +264,7 @@ void Compiler::appendFunctionSelector(ContractDefinition const& _contract)
 		CompilerContext::LocationSetter locationSetter(m_context, functionType->declaration());
 		m_context << callDataUnpackerEntryPoints.at(it.first);
 		eth::AssemblyItem returnTag = m_context.pushNewTag();
+		m_context << CompilerUtils::dataStartOffset;
 		appendCalldataUnpacker(functionType->parameterTypes());
 		m_context.appendJumpTo(m_context.functionEntryLabel(functionType->declaration()));
 		m_context << returnTag;
@@ -274,23 +272,17 @@ void Compiler::appendFunctionSelector(ContractDefinition const& _contract)
 	}
 }
 
-void Compiler::appendCalldataUnpacker(
-	TypePointers const& _typeParameters,
-	bool _fromMemory,
-	u256 _startOffset
-)
+void Compiler::appendCalldataUnpacker(TypePointers const& _typeParameters, bool _fromMemory)
 {
-	// We do not check the calldata size, everything is zero-paddedd
+	// We do not check the calldata size, everything is zero-padded
 
 	//@todo this does not yet support nested dynamic arrays
 
-	if (_startOffset == u256(-1))
-		_startOffset = u256(CompilerUtils::dataStartOffset);
-
-	m_context << _startOffset;
+	// Retain the offset pointer as base_offset, the point from which the data offsets are computed.
+	m_context << eth::Instruction::DUP1;
 	for (TypePointer const& type: _typeParameters)
 	{
-		// stack: v1 v2 ... v(k-1) mem_offset
+		// stack: v1 v2 ... v(k-1) base_offset current_offset
 		switch (type->category())
 		{
 		case Type::Category::Array:
@@ -309,9 +301,9 @@ void Compiler::appendCalldataUnpacker(
 				solAssert(arrayType.location() == DataLocation::Memory, "");
 				// compute data pointer
 				m_context << eth::Instruction::DUP1 << eth::Instruction::MLOAD;
-				//@todo once we support nested arrays, this offset needs to be dynamic.
-				m_context << _startOffset << eth::Instruction::ADD;
-				m_context << eth::Instruction::SWAP1 << u256(0x20) << eth::Instruction::ADD;
+				m_context << eth::Instruction::DUP3 << eth::Instruction::ADD;
+				m_context << eth::Instruction::SWAP2 << eth::Instruction::SWAP1;
+				m_context << u256(0x20) << eth::Instruction::ADD;
 			}
 			else
 			{
@@ -321,14 +313,14 @@ void Compiler::appendCalldataUnpacker(
 				{
 					// put on stack: data_pointer length
 					CompilerUtils(m_context).loadFromMemoryDynamic(IntegerType(256), !_fromMemory);
-					// stack: data_offset next_pointer
-					//@todo once we support nested arrays, this offset needs to be dynamic.
-					m_context << eth::Instruction::SWAP1 << _startOffset << eth::Instruction::ADD;
-					// stack: next_pointer data_pointer
+					// stack: base_offset data_offset next_pointer
+					m_context << eth::Instruction::SWAP1 << eth::Instruction::DUP3 << eth::Instruction::ADD;
+					// stack: base_offset next_pointer data_pointer
 					// retrieve length
 					CompilerUtils(m_context).loadFromMemoryDynamic(IntegerType(256), !_fromMemory, true);
-					// stack: next_pointer length data_pointer
+					// stack: base_offset next_pointer length data_pointer
 					m_context << eth::Instruction::SWAP2;
+					// stack: base_offset data_pointer length next_pointer
 				}
 				else
 				{
@@ -338,7 +330,7 @@ void Compiler::appendCalldataUnpacker(
 				}
 				if (arrayType.location() == DataLocation::Memory)
 				{
-					// stack: calldata_ref [length] next_calldata
+					// stack: base_offset calldata_ref [length] next_calldata
 					// copy to memory
 					// move calldata type up again
 					CompilerUtils(m_context).moveIntoStack(calldataType->sizeOnStack());
@@ -346,15 +338,21 @@ void Compiler::appendCalldataUnpacker(
 					// fetch next pointer again
 					CompilerUtils(m_context).moveToStackTop(arrayType.sizeOnStack());
 				}
+				// move base_offset up
+				CompilerUtils(m_context).moveToStackTop(1 + arrayType.sizeOnStack());
+				m_context << eth::Instruction::SWAP1;
 			}
 			break;
 		}
 		default:
 			solAssert(!type->isDynamicallySized(), "Unknown dynamically sized type: " + type->toString());
 			CompilerUtils(m_context).loadFromMemoryDynamic(*type, !_fromMemory, true);
+			CompilerUtils(m_context).moveToStackTop(1 + type->sizeOnStack());
+			m_context << eth::Instruction::SWAP1;
 		}
+		// stack: v1 v2 ... v(k-1) v(k) base_offset mem_offset
 	}
-	m_context << eth::Instruction::POP;
+	m_context << eth::Instruction::POP << eth::Instruction::POP;
 }
 
 void Compiler::appendReturnValuePacker(TypePointers const& _typeParameters)
diff --git a/libsolidity/Compiler.h b/libsolidity/Compiler.h
index 7b7cffcf..c3bb838a 100644
--- a/libsolidity/Compiler.h
+++ b/libsolidity/Compiler.h
@@ -85,12 +85,8 @@ private:
 	void appendFunctionSelector(ContractDefinition const& _contract);
 	/// Creates code that unpacks the arguments for the given function represented by a vector of TypePointers.
 	/// From memory if @a _fromMemory is true, otherwise from call data.
-	/// Expects source offset on the stack.
-	void appendCalldataUnpacker(
-		TypePointers const& _typeParameters,
-		bool _fromMemory = false,
-		u256 _startOffset = u256(-1)
-	);
+	/// Expects source offset on the stack, which is removed.
+	void appendCalldataUnpacker(TypePointers const& _typeParameters, bool _fromMemory = false);
 	void appendReturnValuePacker(TypePointers const& _typeParameters);
 
 	void registerStateVariables(ContractDefinition const& _contract);
diff --git a/test/libsolidity/Assembly.cpp b/test/libsolidity/Assembly.cpp
index b4678611..ca1e8980 100644
--- a/test/libsolidity/Assembly.cpp
+++ b/test/libsolidity/Assembly.cpp
@@ -108,7 +108,7 @@ BOOST_AUTO_TEST_CASE(location_test)
 	AssemblyItems items = compileContract(sourceCode);
 	vector<SourceLocation> locations =
 		vector<SourceLocation>(17, SourceLocation(2, 75, n)) +
-		vector<SourceLocation>(26, SourceLocation(20, 72, n)) +
+		vector<SourceLocation>(28, SourceLocation(20, 72, n)) +
 		vector<SourceLocation>{SourceLocation(42, 51, n), SourceLocation(65, 67, n)} +
 		vector<SourceLocation>(4, SourceLocation(58, 67, n)) +
 		vector<SourceLocation>(3, SourceLocation(20, 72, n));
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index 3124f9cf..0459c3ae 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -5354,6 +5354,25 @@ BOOST_AUTO_TEST_CASE(fixed_arrays_as_return_type)
 	);
 }
 
+BOOST_AUTO_TEST_CASE(calldata_offset)
+{
+	// This tests a specific bug that was caused by not using the correct memory offset in the
+	// calldata unpacker.
+	char const* sourceCode = R"(
+			contract CB
+			{
+				address[] _arr;
+				string public last = "nd";
+				function CB(address[] guardians)
+				{
+					_arr = guardians;
+				}
+			}
+			)";
+	compileAndRun(sourceCode, 0, "CB", encodeArgs(u256(0x20)));
+	BOOST_CHECK(callContractFunction("last()", encodeArgs()) == encodeDyn(string("nd")));
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 
 }