From 5dc3a971cba7dca5bb95db60d877790d235ee036 Mon Sep 17 00:00:00 2001 From: chriseth Date: Tue, 13 Oct 2020 18:14:08 +0200 Subject: [PATCH] Use revert for out-of-bounds array index access in getter. --- Changelog.md | 1 + libsolidity/codegen/ExpressionCompiler.cpp | 11 ++++++++++- libsolidity/codegen/ir/IRGenerator.cpp | 17 ++++++++++++----- test/libsolidity/gasTests/abiv2.sol | 6 +++--- test/libsolidity/gasTests/abiv2_optimised.sol | 4 ++-- test/libsolidity/gasTests/dispatch_large.sol | 4 ++-- .../gasTests/dispatch_large_optimised.sol | 4 ++-- test/libsolidity/gasTests/dispatch_medium.sol | 4 ++-- .../gasTests/dispatch_medium_optimised.sol | 4 ++-- test/libsolidity/gasTests/dispatch_small.sol | 4 ++-- .../gasTests/dispatch_small_optimised.sol | 4 ++-- 11 files changed, 40 insertions(+), 23 deletions(-) diff --git a/Changelog.md b/Changelog.md index b47a66ad8..022d47a5d 100644 --- a/Changelog.md +++ b/Changelog.md @@ -12,6 +12,7 @@ Compiler Features: Bugfixes: * Code generator: Fix internal compiler error when referencing members via module name but not using the reference. * Code generator: Fix ``ABIEncoderV2`` pragma from the current module affecting inherited functions and applied modifiers. + * Code generator: Use revert instead of invalid opcode for out-of-bounds array index access in getter. * Type Checker: Fix internal compiler error caused by storage parameters with nested mappings in libraries. * Name Resolver: Fix shadowing/same-name warnings for later declarations. * Contract Level Checker: Add missing check against inheriting functions with ABIEncoderV2 return types in ABIEncoderV1 contracts. diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp index e4fd66ac9..9dff56afc 100644 --- a/libsolidity/codegen/ExpressionCompiler.cpp +++ b/libsolidity/codegen/ExpressionCompiler.cpp @@ -170,7 +170,16 @@ void ExpressionCompiler::appendStateVariableAccessor(VariableDeclaration const& // pop offset m_context << Instruction::POP; utils().copyToStackTop(paramTypes.size() - i + 1, 1); - ArrayUtils(m_context).accessIndex(*arrayType); + + ArrayUtils(m_context).retrieveLength(*arrayType, 1); + // Stack: ref [length] index length + // check out-of-bounds access + m_context << Instruction::DUP2 << Instruction::LT; + auto tag = m_context.appendConditionalJump(); + m_context << u256(0) << Instruction::DUP1 << Instruction::REVERT; + m_context << tag; + + ArrayUtils(m_context).accessIndex(*arrayType, false); returnType = arrayType->baseType(); } else diff --git a/libsolidity/codegen/ir/IRGenerator.cpp b/libsolidity/codegen/ir/IRGenerator.cpp index b1ab9dda4..fc7516dd9 100644 --- a/libsolidity/codegen/ir/IRGenerator.cpp +++ b/libsolidity/codegen/ir/IRGenerator.cpp @@ -348,18 +348,25 @@ string IRGenerator::generateGetter(VariableDeclaration const& _varDecl) mappingType ? *mappingType->keyType() : *TypeProvider::uint256() ).stackSlots(); parameters += keys; - code += Whiskers(R"( + + Whiskers templ(R"( + + if iszero(lt(, (slot))) { revert(0, 0) } + slot, offset := (slot, ) - )") - ( + )"); + templ( "indexAccess", mappingType ? m_utils.mappingIndexAccessFunction(*mappingType, *mappingType->keyType()) : m_utils.storageArrayIndexAccessFunction(*arrayType) ) ("array", arrayType != nullptr) - ("keys", joinHumanReadable(keys)) - .render(); + ("keys", joinHumanReadable(keys)); + if (arrayType) + templ("length", m_utils.arrayLengthFunction(*arrayType)); + + code += templ.render(); currentType = mappingType ? mappingType->valueType() : arrayType->baseType(); } diff --git a/test/libsolidity/gasTests/abiv2.sol b/test/libsolidity/gasTests/abiv2.sol index 9042c8a51..8d4569947 100644 --- a/test/libsolidity/gasTests/abiv2.sol +++ b/test/libsolidity/gasTests/abiv2.sol @@ -14,9 +14,9 @@ contract C { } // ---- // creation: -// codeDepositCost: 1106800 -// executionCost: 1147 -// totalCost: 1107947 +// codeDepositCost: 1107400 +// executionCost: 1154 +// totalCost: 1108554 // external: // a(): 1130 // b(uint256): infinite diff --git a/test/libsolidity/gasTests/abiv2_optimised.sol b/test/libsolidity/gasTests/abiv2_optimised.sol index 1069a6bdf..ada98be00 100644 --- a/test/libsolidity/gasTests/abiv2_optimised.sol +++ b/test/libsolidity/gasTests/abiv2_optimised.sol @@ -17,9 +17,9 @@ contract C { // optimize-yul: true // ---- // creation: -// codeDepositCost: 604400 +// codeDepositCost: 605000 // executionCost: 638 -// totalCost: 605038 +// totalCost: 605638 // external: // a(): 1029 // b(uint256): 2084 diff --git a/test/libsolidity/gasTests/dispatch_large.sol b/test/libsolidity/gasTests/dispatch_large.sol index 3fcd62eab..c76492c93 100644 --- a/test/libsolidity/gasTests/dispatch_large.sol +++ b/test/libsolidity/gasTests/dispatch_large.sol @@ -24,9 +24,9 @@ contract Large { } // ---- // creation: -// codeDepositCost: 637000 +// codeDepositCost: 637600 // executionCost: 670 -// totalCost: 637670 +// totalCost: 638270 // external: // a(): 1051 // b(uint256): 2046 diff --git a/test/libsolidity/gasTests/dispatch_large_optimised.sol b/test/libsolidity/gasTests/dispatch_large_optimised.sol index 49073a37b..30b46a3c0 100644 --- a/test/libsolidity/gasTests/dispatch_large_optimised.sol +++ b/test/libsolidity/gasTests/dispatch_large_optimised.sol @@ -27,9 +27,9 @@ contract Large { // optimize-runs: 2 // ---- // creation: -// codeDepositCost: 260600 +// codeDepositCost: 261200 // executionCost: 300 -// totalCost: 260900 +// totalCost: 261500 // external: // a(): 998 // b(uint256): 2305 diff --git a/test/libsolidity/gasTests/dispatch_medium.sol b/test/libsolidity/gasTests/dispatch_medium.sol index 5d1fb1b87..715a3e97f 100644 --- a/test/libsolidity/gasTests/dispatch_medium.sol +++ b/test/libsolidity/gasTests/dispatch_medium.sol @@ -11,9 +11,9 @@ contract Medium { } // ---- // creation: -// codeDepositCost: 253200 +// codeDepositCost: 253800 // executionCost: 294 -// totalCost: 253494 +// totalCost: 254094 // external: // a(): 1028 // b(uint256): 2046 diff --git a/test/libsolidity/gasTests/dispatch_medium_optimised.sol b/test/libsolidity/gasTests/dispatch_medium_optimised.sol index 7fe4f8897..b0741e8be 100644 --- a/test/libsolidity/gasTests/dispatch_medium_optimised.sol +++ b/test/libsolidity/gasTests/dispatch_medium_optimised.sol @@ -14,9 +14,9 @@ contract Medium { // optimize-runs: 2 // ---- // creation: -// codeDepositCost: 141000 +// codeDepositCost: 141600 // executionCost: 190 -// totalCost: 141190 +// totalCost: 141790 // external: // a(): 998 // b(uint256): 2063 diff --git a/test/libsolidity/gasTests/dispatch_small.sol b/test/libsolidity/gasTests/dispatch_small.sol index 52581c5fa..9b44e5937 100644 --- a/test/libsolidity/gasTests/dispatch_small.sol +++ b/test/libsolidity/gasTests/dispatch_small.sol @@ -6,9 +6,9 @@ contract Small { } // ---- // creation: -// codeDepositCost: 84800 +// codeDepositCost: 85400 // executionCost: 135 -// totalCost: 84935 +// totalCost: 85535 // external: // fallback: 129 // a(): 983 diff --git a/test/libsolidity/gasTests/dispatch_small_optimised.sol b/test/libsolidity/gasTests/dispatch_small_optimised.sol index cd967ea19..ed93fab71 100644 --- a/test/libsolidity/gasTests/dispatch_small_optimised.sol +++ b/test/libsolidity/gasTests/dispatch_small_optimised.sol @@ -9,9 +9,9 @@ contract Small { // optimize-runs: 2 // ---- // creation: -// codeDepositCost: 60600 +// codeDepositCost: 61200 // executionCost: 111 -// totalCost: 60711 +// totalCost: 61311 // external: // fallback: 118 // a(): 976