From f2ae30f6204bbcf14e4b48e3f981c9fd54ac0faf Mon Sep 17 00:00:00 2001 From: chriseth Date: Wed, 15 May 2019 18:31:07 +0200 Subject: [PATCH] Fix handling of structs of dynamic size as constructor parameters. --- Changelog.md | 1 + docs/bugs.json | 11 ++++ docs/bugs_by_version.json | 26 +++++++- libsolidity/codegen/ContractCompiler.cpp | 31 +++------- test/libsolidity/ABIEncoderTests.cpp | 75 +++++++++++++++++++++++ test/libsolidity/GasMeter.cpp | 4 +- test/libsolidity/SolidityEndToEndTest.cpp | 2 +- 7 files changed, 123 insertions(+), 27 deletions(-) diff --git a/Changelog.md b/Changelog.md index f5c69c3c0..f378aab49 100644 --- a/Changelog.md +++ b/Changelog.md @@ -5,6 +5,7 @@ Language Features: Compiler Features: + * Code Generator: Fix handling of structs of dynamic size as constructor parameters. * Optimizer: Add rule to simplify SHL/SHR combinations. * SMTChecker: Support inherited state variables. * SMTChecker: Support tuples and function calls with multiple return values. diff --git a/docs/bugs.json b/docs/bugs.json index 2f13b13df..3eb5d1807 100644 --- a/docs/bugs.json +++ b/docs/bugs.json @@ -1,4 +1,15 @@ [ + { + "name": "DynamicConstructorArgumentsClippedABIV2", + "summary": "A contract's constructor that takes structs or arrays that contain dynamically-sized arrays reverts or decodes to invalid data.", + "description": "During construction of a contract, constructor parameters are copied from the code section to memory for decoding. The amount of bytes to copy was calculated incorrectly in case all parameters are statically-sized but contain dynamically-sized arrays as struct members or inner arrays. Such types are only available if ABIEncoderV2 is activated.", + "introduced": "0.4.16", + "fixed": "0.5.9", + "severity": "very low", + "conditions": { + "ABIEncoderV2": true + } + }, { "name": "UninitializedFunctionPointerInConstructor", "summary": "Calling uninitialized internal function pointers created in the constructor does not always revert and can cause unexpected behaviour.", diff --git a/docs/bugs_by_version.json b/docs/bugs_by_version.json index f0e5b95b6..dbcbd01d0 100644 --- a/docs/bugs_by_version.json +++ b/docs/bugs_by_version.json @@ -452,6 +452,7 @@ }, "0.4.16": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor_0.4.x", "IncorrectEventSignatureInLibraries_0.4.x", "ExpExponentCleanup", @@ -462,6 +463,7 @@ }, "0.4.17": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor_0.4.x", "IncorrectEventSignatureInLibraries_0.4.x", "ExpExponentCleanup", @@ -473,6 +475,7 @@ }, "0.4.18": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor_0.4.x", "IncorrectEventSignatureInLibraries_0.4.x", "ExpExponentCleanup", @@ -483,6 +486,7 @@ }, "0.4.19": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor_0.4.x", "IncorrectEventSignatureInLibraries_0.4.x", "ABIEncoderV2PackedStorage_0.4.x", @@ -510,6 +514,7 @@ }, "0.4.20": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor_0.4.x", "IncorrectEventSignatureInLibraries_0.4.x", "ABIEncoderV2PackedStorage_0.4.x", @@ -521,6 +526,7 @@ }, "0.4.21": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor_0.4.x", "IncorrectEventSignatureInLibraries_0.4.x", "ABIEncoderV2PackedStorage_0.4.x", @@ -532,6 +538,7 @@ }, "0.4.22": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor_0.4.x", "IncorrectEventSignatureInLibraries_0.4.x", "ABIEncoderV2PackedStorage_0.4.x", @@ -543,6 +550,7 @@ }, "0.4.23": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor_0.4.x", "IncorrectEventSignatureInLibraries_0.4.x", "ABIEncoderV2PackedStorage_0.4.x", @@ -553,6 +561,7 @@ }, "0.4.24": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor_0.4.x", "IncorrectEventSignatureInLibraries_0.4.x", "ABIEncoderV2PackedStorage_0.4.x", @@ -563,6 +572,7 @@ }, "0.4.25": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor_0.4.x", "IncorrectEventSignatureInLibraries_0.4.x", "ABIEncoderV2PackedStorage_0.4.x" @@ -570,7 +580,9 @@ "released": "2018-09-12" }, "0.4.26": { - "bugs": [], + "bugs": [ + "DynamicConstructorArgumentsClippedABIV2" + ], "released": "2019-04-29" }, "0.4.3": { @@ -677,6 +689,7 @@ }, "0.5.0": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage" @@ -685,6 +698,7 @@ }, "0.5.1": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage" @@ -693,6 +707,7 @@ }, "0.5.2": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage" @@ -701,6 +716,7 @@ }, "0.5.3": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage" @@ -709,6 +725,7 @@ }, "0.5.4": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage" @@ -717,6 +734,7 @@ }, "0.5.5": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage", @@ -727,6 +745,7 @@ }, "0.5.6": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries", "ABIEncoderV2PackedStorage", @@ -736,13 +755,16 @@ }, "0.5.7": { "bugs": [ + "DynamicConstructorArgumentsClippedABIV2", "UninitializedFunctionPointerInConstructor", "IncorrectEventSignatureInLibraries" ], "released": "2019-03-26" }, "0.5.8": { - "bugs": [], + "bugs": [ + "DynamicConstructorArgumentsClippedABIV2" + ], "released": "2019-04-30" } } \ No newline at end of file diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp index 52f81cc3b..d50e4f4d5 100644 --- a/libsolidity/codegen/ContractCompiler.cpp +++ b/libsolidity/codegen/ContractCompiler.cpp @@ -228,34 +228,21 @@ void ContractCompiler::appendConstructor(FunctionDefinition const& _constructor) // copy constructor arguments from code to memory and then to stack, they are supplied after the actual program if (!_constructor.parameters().empty()) { - unsigned argumentSize = 0; - for (ASTPointer const& var: _constructor.parameters()) - if (var->annotation().type->isDynamicallySized()) - { - argumentSize = 0; - break; - } - else - argumentSize += var->annotation().type->calldataEncodedSize(); - CompilerUtils(m_context).fetchFreeMemoryPointer(); - if (argumentSize == 0) - { - // argument size is dynamic, use CODESIZE to determine it - m_context.appendProgramSize(); // program itself - // CODESIZE is program plus manually added arguments - m_context << Instruction::CODESIZE << Instruction::SUB; - } - else - m_context << u256(argumentSize); + // CODESIZE returns the actual size of the code, + // which is the size of the generated code (``programSize``) + // plus the constructor arguments added to the transaction payload. + m_context.appendProgramSize(); + m_context << Instruction::CODESIZE << Instruction::SUB; // stack: m_context << Instruction::DUP1; m_context.appendProgramSize(); m_context << Instruction::DUP4 << Instruction::CODECOPY; - m_context << Instruction::DUP2 << Instruction::ADD; - m_context << Instruction::DUP1; + // stack: + m_context << Instruction::DUP2 << Instruction::DUP2 << Instruction::ADD; + // stack: CompilerUtils(m_context).storeFreeMemoryPointer(); - // stack: + // stack: CompilerUtils(m_context).abiDecode(FunctionType(_constructor).parameterTypes(), true); } _constructor.accept(*this); diff --git a/test/libsolidity/ABIEncoderTests.cpp b/test/libsolidity/ABIEncoderTests.cpp index ae96c3b30..db19181e9 100644 --- a/test/libsolidity/ABIEncoderTests.cpp +++ b/test/libsolidity/ABIEncoderTests.cpp @@ -712,6 +712,81 @@ BOOST_AUTO_TEST_CASE(packed_structs) ) } + +BOOST_AUTO_TEST_CASE(struct_in_constructor) +{ + string sourceCode = R"( + contract C { + struct S { + string a; + uint8 b; + string c; + } + S public x; + constructor(S memory s) public { x = s; } + } + )"; + + NEW_ENCODER( + compileAndRun(sourceCode, 0, "C", encodeArgs(0x20, 0x60, 0x03, 0x80, 0x00, 0x00)); + ABI_CHECK(callContractFunction("x()"), encodeArgs(0x60, 0x03, 0x80, 0x00, 0x00)); + ) +} + +BOOST_AUTO_TEST_CASE(struct_in_constructor_indirect) +{ + string sourceCode = R"( + contract C { + struct S { + string a; + uint8 b; + string c; + } + S public x; + constructor(S memory s) public { x = s; } + } + + contract D { + function f() public returns (string memory, uint8, string memory) { + C.S memory s; + s.a = "abc"; + s.b = 7; + s.c = "def"; + C c = new C(s); + return c.x(); + } + } + )"; + if (dev::test::Options::get().evmVersion().supportsReturndata()) + { + NEW_ENCODER( + compileAndRun(sourceCode, 0, "D"); + ABI_CHECK(callContractFunction("f()"), encodeArgs(0x60, 7, 0xa0, 3, "abc", 3, "def")); + ) + } +} + +BOOST_AUTO_TEST_CASE(struct_in_constructor_data_short) +{ + string sourceCode = R"( + contract C { + struct S { + string a; + uint8 b; + string c; + } + S public x; + constructor(S memory s) public { x = s; } + } + )"; + + NEW_ENCODER( + BOOST_CHECK( + compileAndRunWithoutCheck(sourceCode, 0, "C", encodeArgs(0x20, 0x60, 0x03, 0x80, 0x00)).empty() + ); + ) +} + BOOST_AUTO_TEST_SUITE_END() } diff --git a/test/libsolidity/GasMeter.cpp b/test/libsolidity/GasMeter.cpp index 7ef791c54..4b13172b1 100644 --- a/test/libsolidity/GasMeter.cpp +++ b/test/libsolidity/GasMeter.cpp @@ -176,8 +176,8 @@ BOOST_AUTO_TEST_CASE(store_keccak256) char const* sourceCode = R"( contract test { bytes32 public shaValue; - constructor(uint a) public { - shaValue = keccak256(abi.encodePacked(a)); + constructor() public { + shaValue = keccak256(abi.encodePacked(this)); } } )"; diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp index f50c3d2f1..145a61674 100644 --- a/test/libsolidity/SolidityEndToEndTest.cpp +++ b/test/libsolidity/SolidityEndToEndTest.cpp @@ -9771,7 +9771,7 @@ BOOST_AUTO_TEST_CASE(calldata_offset) } } )"; - compileAndRun(sourceCode, 0, "CB", encodeArgs(u256(0x20))); + compileAndRun(sourceCode, 0, "CB", encodeArgs(u256(0x20), u256(0x00))); ABI_CHECK(callContractFunction("last()", encodeArgs()), encodeDyn(string("nd"))); }