From 97f8c9347d019f481aec4347098a158c162f9c04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Bylica?= Date: Tue, 27 Aug 2019 12:30:09 +0200 Subject: [PATCH 1/6] Add limits.hpp with EVM constants --- lib/evmone/CMakeLists.txt | 1 + lib/evmone/limits.hpp | 7 +++++++ test/unittests/CMakeLists.txt | 1 + test/unittests/evm_test.cpp | 3 +-- 4 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 lib/evmone/limits.hpp diff --git a/lib/evmone/CMakeLists.txt b/lib/evmone/CMakeLists.txt index 314806a719..e57eb523fe 100644 --- a/lib/evmone/CMakeLists.txt +++ b/lib/evmone/CMakeLists.txt @@ -15,6 +15,7 @@ add_library(evmone execution.cpp execution.hpp instructions.cpp + limits.hpp opcodes_helpers.h ) target_link_libraries(evmone PUBLIC evmc::evmc PRIVATE intx::intx evmc::instructions ethash::keccak) diff --git a/lib/evmone/limits.hpp b/lib/evmone/limits.hpp new file mode 100644 index 0000000000..88253749b7 --- /dev/null +++ b/lib/evmone/limits.hpp @@ -0,0 +1,7 @@ +// evmone: Fast Ethereum Virtual Machine implementation +// Copyright 2019 Pawel Bylica. +// Licensed under the Apache License, Version 2.0. +#pragma once + +/// The maximum EVM bytecode size allowed by the Ethereum spec. +constexpr auto max_code_size = 0x6000; diff --git a/test/unittests/CMakeLists.txt b/test/unittests/CMakeLists.txt index 40c9c28493..c50f96dea5 100644 --- a/test/unittests/CMakeLists.txt +++ b/test/unittests/CMakeLists.txt @@ -12,6 +12,7 @@ add_library(evm-unittests OBJECT evm_test.cpp ) target_link_libraries(evm-unittests PRIVATE testutils evmc::evmc GTest::gtest) +target_include_directories(evm-unittests PRIVATE ${evmone_private_include_dir}) # The internal evmone unit tests. The generic EVM ones are also built in. add_executable(evmone-unittests diff --git a/test/unittests/evm_test.cpp b/test/unittests/evm_test.cpp index 596af4bdcc..4a220cf7fc 100644 --- a/test/unittests/evm_test.cpp +++ b/test/unittests/evm_test.cpp @@ -4,6 +4,7 @@ #include "evm_fixture.hpp" #include +#include #include #include #include @@ -12,8 +13,6 @@ using namespace evmc::literals; using namespace intx; -constexpr auto max_code_size = 0x6000; - TEST_F(evm, empty) { execute(0, ""); From 9f41bd1d149cb145fdbe19c7691b966b36f23ded Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Bylica?= Date: Tue, 27 Aug 2019 12:38:09 +0200 Subject: [PATCH 2/6] Use int32_t for block gas cost --- lib/evmone/analysis.cpp | 4 ++-- lib/evmone/analysis.hpp | 10 ++++++++-- lib/evmone/limits.hpp | 4 ++++ 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/lib/evmone/analysis.cpp b/lib/evmone/analysis.cpp index 40303a7fe8..20ecee7c3e 100644 --- a/lib/evmone/analysis.cpp +++ b/lib/evmone/analysis.cpp @@ -153,7 +153,7 @@ code_analysis analyze( break; case OP_GAS: - instr.arg.p.number = static_cast(block->gas_cost); + instr.arg.p.number = block->gas_cost; break; case OP_CALL: @@ -162,7 +162,7 @@ code_analysis analyze( case OP_STATICCALL: case OP_CREATE: case OP_CREATE2: - instr.arg.p.number = static_cast(block->gas_cost); + instr.arg.p.number = block->gas_cost; instr.arg.p.call_kind = op2call_kind(opcode == OP_STATICCALL ? uint8_t{OP_CALL} : opcode); break; diff --git a/lib/evmone/analysis.hpp b/lib/evmone/analysis.hpp index 048ad28b5f..fdfbf0b787 100644 --- a/lib/evmone/analysis.hpp +++ b/lib/evmone/analysis.hpp @@ -3,6 +3,7 @@ // Licensed under the Apache License, Version 2.0. #pragma once +#include "limits.hpp" #include #include #include @@ -99,7 +100,7 @@ struct execution_state /// /// This is only needed to correctly calculate remaining gas for GAS instruction. /// TODO: Maybe this should be precomputed in analysis. - int64_t current_block_cost = 0; + int32_t current_block_cost = 0; struct code_analysis* analysis = nullptr; bytes return_data; @@ -162,7 +163,12 @@ struct instr_info struct block_info { /// The total base gas cost of all instructions in the block. - int64_t gas_cost = 0; + /// This cannot overflow, see the static_assert() below. + int32_t gas_cost = 0; + + static_assert( + max_code_size * max_instruction_base_cost < std::numeric_limits::max(), + "Potential block_info::gas_cost overflow"); /// The stack height required to execute the block. int stack_req = 0; diff --git a/lib/evmone/limits.hpp b/lib/evmone/limits.hpp index 88253749b7..db31d50627 100644 --- a/lib/evmone/limits.hpp +++ b/lib/evmone/limits.hpp @@ -5,3 +5,7 @@ /// The maximum EVM bytecode size allowed by the Ethereum spec. constexpr auto max_code_size = 0x6000; + +/// The maximum base cost of any EVM instruction. +/// The value comes from the cost of the SSTORE instruction. +constexpr auto max_instruction_base_cost = 20000; From e6d2079fe35e27b53216881af68d07807034f348 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Bylica?= Date: Tue, 27 Aug 2019 12:53:51 +0200 Subject: [PATCH 3/6] Use int16_t for block stack max growth --- lib/evmone/analysis.cpp | 3 ++- lib/evmone/analysis.hpp | 7 ++++++- lib/evmone/limits.hpp | 5 +++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/lib/evmone/analysis.cpp b/lib/evmone/analysis.cpp index 20ecee7c3e..ee433d9572 100644 --- a/lib/evmone/analysis.cpp +++ b/lib/evmone/analysis.cpp @@ -92,7 +92,8 @@ code_analysis analyze( block->stack_req = std::max(block->stack_req, instr_stack_req - block_stack_change); block_stack_change += instr_stack_change; - block->stack_max_growth = std::max(block->stack_max_growth, block_stack_change); + block->stack_max_growth = + static_cast(std::max(int{block->stack_max_growth}, block_stack_change)); if (metrics.gas_cost > 0) // can be -1 for undefined instruction block->gas_cost += metrics.gas_cost; diff --git a/lib/evmone/analysis.hpp b/lib/evmone/analysis.hpp index fdfbf0b787..dcf5a847cf 100644 --- a/lib/evmone/analysis.hpp +++ b/lib/evmone/analysis.hpp @@ -174,7 +174,12 @@ struct block_info int stack_req = 0; /// The maximum stack height growth relative to the stack height at block start. - int stack_max_growth = 0; + /// This cannot overflow, see the static_assert() below. + int16_t stack_max_growth = 0; + + static_assert(max_code_size * max_instruction_stack_increase < + std::numeric_limits::max(), + "Potential block_info::stack_max_growth overflow"); }; struct code_analysis diff --git a/lib/evmone/limits.hpp b/lib/evmone/limits.hpp index db31d50627..b282d8aec1 100644 --- a/lib/evmone/limits.hpp +++ b/lib/evmone/limits.hpp @@ -9,3 +9,8 @@ constexpr auto max_code_size = 0x6000; /// The maximum base cost of any EVM instruction. /// The value comes from the cost of the SSTORE instruction. constexpr auto max_instruction_base_cost = 20000; + +/// The maximum stack increase any instruction can cause. +/// Only instructions taking 0 arguments and pushing an item to the stack +/// can increase the stack height and only by 1. +constexpr auto max_instruction_stack_increase = 1; From 879eb330e190c8cc8719d1683cb03554a0aaa9f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Bylica?= Date: Tue, 27 Aug 2019 13:29:53 +0200 Subject: [PATCH 4/6] Use int16_t for block stack requirement --- lib/evmone/analysis.cpp | 8 +++++++- lib/evmone/analysis.hpp | 5 ++++- test/unittests/evm_test.cpp | 11 +++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/lib/evmone/analysis.cpp b/lib/evmone/analysis.cpp index ee433d9572..4b6c202aa7 100644 --- a/lib/evmone/analysis.cpp +++ b/lib/evmone/analysis.cpp @@ -90,7 +90,13 @@ code_analysis analyze( const auto instr_stack_req = metrics.num_stack_arguments; const auto instr_stack_change = metrics.num_stack_returned_items - instr_stack_req; - block->stack_req = std::max(block->stack_req, instr_stack_req - block_stack_change); + // TODO: Define a block_analysis struct with regular ints for analysis. + // Compress it when block is closed. + auto stack_req = instr_stack_req - block_stack_change; + if (stack_req > std::numeric_limitsstack_req)>::max()) + stack_req = std::numeric_limitsstack_req)>::max(); + + block->stack_req = std::max(block->stack_req, static_cast(stack_req)); block_stack_change += instr_stack_change; block->stack_max_growth = static_cast(std::max(int{block->stack_max_growth}, block_stack_change)); diff --git a/lib/evmone/analysis.hpp b/lib/evmone/analysis.hpp index dcf5a847cf..cbb8f25647 100644 --- a/lib/evmone/analysis.hpp +++ b/lib/evmone/analysis.hpp @@ -171,7 +171,8 @@ struct block_info "Potential block_info::gas_cost overflow"); /// The stack height required to execute the block. - int stack_req = 0; + /// This MAY overflow. + int16_t stack_req = 0; /// The maximum stack height growth relative to the stack height at block start. /// This cannot overflow, see the static_assert() below. @@ -182,6 +183,8 @@ struct block_info "Potential block_info::stack_max_growth overflow"); }; +static_assert(sizeof(block_info) == 8); + struct code_analysis { std::vector instrs; diff --git a/test/unittests/evm_test.cpp b/test/unittests/evm_test.cpp index 4a220cf7fc..091bd8c9c1 100644 --- a/test/unittests/evm_test.cpp +++ b/test/unittests/evm_test.cpp @@ -1372,3 +1372,14 @@ TEST_F(evm, memory_access) } } } + +TEST_F(evm, block_stack_req_overflow) +{ + // This tests constructs a code with single basic block which stack requirement is > int16 max. + // Such basic block can cause int16_t overflow during analysis. + // The CALL instruction is going to be used because it has -6 stack change. + + const auto code = push(1) + 10 * OP_DUP1 + 5463 * OP_CALL; + execute(code); + EXPECT_STATUS(EVMC_STACK_UNDERFLOW); +} From 421622f99a67833ebe3541a1a48b14afc973d764 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Bylica?= Date: Tue, 27 Aug 2019 14:02:02 +0200 Subject: [PATCH 5/6] Allow storing block info in instruction arguments --- lib/evmone/analysis.hpp | 47 +++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/lib/evmone/analysis.hpp b/lib/evmone/analysis.hpp index cbb8f25647..1cd1ca85eb 100644 --- a/lib/evmone/analysis.hpp +++ b/lib/evmone/analysis.hpp @@ -85,6 +85,29 @@ class evm_memory struct instr_info; +struct block_info +{ + /// The total base gas cost of all instructions in the block. + /// This cannot overflow, see the static_assert() below. + int32_t gas_cost = 0; + + static_assert( + max_code_size * max_instruction_base_cost < std::numeric_limits::max(), + "Potential block_info::gas_cost overflow"); + + /// The stack height required to execute the block. + /// This MAY overflow. + int16_t stack_req = 0; + + /// The maximum stack height growth relative to the stack height at block start. + /// This cannot overflow, see the static_assert() below. + int16_t stack_max_growth = 0; + + static_assert(max_code_size * max_instruction_stack_increase < + std::numeric_limits::max(), + "Potential block_info::stack_max_growth overflow"); +}; + struct execution_state { evmc_status_code status = EVMC_SUCCESS; @@ -130,6 +153,7 @@ union instr_argument const uint8_t* data; const intx::uint256* push_value; uint64_t small_push_value; + block_info block{}; }; static_assert(sizeof(instr_argument) == sizeof(void*), "Incorrect size of instr_argument"); @@ -160,29 +184,6 @@ struct instr_info explicit constexpr instr_info(exec_fn f) noexcept : fn{f}, arg{} {}; }; -struct block_info -{ - /// The total base gas cost of all instructions in the block. - /// This cannot overflow, see the static_assert() below. - int32_t gas_cost = 0; - - static_assert( - max_code_size * max_instruction_base_cost < std::numeric_limits::max(), - "Potential block_info::gas_cost overflow"); - - /// The stack height required to execute the block. - /// This MAY overflow. - int16_t stack_req = 0; - - /// The maximum stack height growth relative to the stack height at block start. - /// This cannot overflow, see the static_assert() below. - int16_t stack_max_growth = 0; - - static_assert(max_code_size * max_instruction_stack_increase < - std::numeric_limits::max(), - "Potential block_info::stack_max_growth overflow"); -}; - static_assert(sizeof(block_info) == 8); struct code_analysis From 926ba874bfc3e8c7a6ea66e278e79f12872c1130 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Bylica?= Date: Tue, 27 Aug 2019 14:09:13 +0200 Subject: [PATCH 6/6] Keep block info in instruction argument --- lib/evmone/analysis.cpp | 9 ++++----- lib/evmone/analysis.hpp | 1 - lib/evmone/instructions.cpp | 2 +- test/unittests/analysis_test.cpp | 20 ++++++-------------- test/utils/dump.cpp | 2 +- 5 files changed, 12 insertions(+), 22 deletions(-) diff --git a/lib/evmone/analysis.cpp b/lib/evmone/analysis.cpp index 4b6c202aa7..b58e8b9b84 100644 --- a/lib/evmone/analysis.cpp +++ b/lib/evmone/analysis.cpp @@ -66,14 +66,13 @@ code_analysis analyze( if (!block || jumpdest) { - // Create new block. - block = &analysis.blocks.emplace_back(); - block_stack_change = 0; - // Create BEGINBLOCK instruction which either replaces JUMPDEST or is injected // in case there is no JUMPDEST. auto& beginblock_instr = analysis.instrs.emplace_back(fns[OPX_BEGINBLOCK]); - beginblock_instr.arg.p.number = static_cast(analysis.blocks.size() - 1); + + // Start new block. + block = &beginblock_instr.arg.block; + block_stack_change = 0; if (jumpdest) // Add the jumpdest to the map. { diff --git a/lib/evmone/analysis.hpp b/lib/evmone/analysis.hpp index 1cd1ca85eb..42ddcfc4be 100644 --- a/lib/evmone/analysis.hpp +++ b/lib/evmone/analysis.hpp @@ -189,7 +189,6 @@ static_assert(sizeof(block_info) == 8); struct code_analysis { std::vector instrs; - std::vector blocks; /// Storage for large push values. std::vector push_values; diff --git a/lib/evmone/instructions.cpp b/lib/evmone/instructions.cpp index fd8a14c436..09e94fc852 100644 --- a/lib/evmone/instructions.cpp +++ b/lib/evmone/instructions.cpp @@ -1200,7 +1200,7 @@ const instr_info* op_selfdestruct(const instr_info*, execution_state& state) noe const instr_info* opx_beginblock(const instr_info* instr, execution_state& state) noexcept { - auto& block = state.analysis->blocks[static_cast(instr->arg.p.number)]; + auto& block = instr->arg.block; if ((state.gas_left -= block.gas_cost) < 0) return state.exit(EVMC_OUT_OF_GAS); diff --git a/test/unittests/analysis_test.cpp b/test/unittests/analysis_test.cpp index bd08854580..7bf63c3131 100644 --- a/test/unittests/analysis_test.cpp +++ b/test/unittests/analysis_test.cpp @@ -30,7 +30,6 @@ TEST(analysis, example1) ASSERT_EQ(analysis.instrs.size(), 8); EXPECT_EQ(analysis.instrs[0].fn, fake_fn_table[OPX_BEGINBLOCK]); - EXPECT_EQ(analysis.instrs[0].arg.p.number, 0); EXPECT_EQ(analysis.instrs[1].fn, fake_fn_table[OP_PUSH1]); EXPECT_EQ(analysis.instrs[2].fn, fake_fn_table[OP_PUSH1]); EXPECT_EQ(analysis.instrs[3].fn, fake_fn_table[OP_MSTORE8]); @@ -39,10 +38,9 @@ TEST(analysis, example1) EXPECT_EQ(analysis.instrs[6].fn, fake_fn_table[OP_SSTORE]); EXPECT_EQ(analysis.instrs[7].fn, fake_fn_table[OP_STOP]); - ASSERT_EQ(analysis.blocks.size(), 1); - EXPECT_EQ(analysis.blocks[0].gas_cost, 14); - EXPECT_EQ(analysis.blocks[0].stack_req, 0); - EXPECT_EQ(analysis.blocks[0].stack_max_growth, 2); + EXPECT_EQ(analysis.instrs[0].arg.block.gas_cost, 14); + EXPECT_EQ(analysis.instrs[0].arg.block.stack_req, 0); + EXPECT_EQ(analysis.instrs[0].arg.block.stack_max_growth, 2); } TEST(analysis, stack_up_and_down) @@ -52,16 +50,14 @@ TEST(analysis, stack_up_and_down) ASSERT_EQ(analysis.instrs.size(), 20); EXPECT_EQ(analysis.instrs[0].fn, fake_fn_table[OPX_BEGINBLOCK]); - EXPECT_EQ(analysis.instrs[0].arg.p.number, 0); EXPECT_EQ(analysis.instrs[1].fn, fake_fn_table[OP_DUP2]); EXPECT_EQ(analysis.instrs[2].fn, fake_fn_table[OP_DUP1]); EXPECT_EQ(analysis.instrs[8].fn, fake_fn_table[OP_POP]); EXPECT_EQ(analysis.instrs[18].fn, fake_fn_table[OP_PUSH1]); - ASSERT_EQ(analysis.blocks.size(), 1); - EXPECT_EQ(analysis.blocks[0].gas_cost, 7 * 3 + 10 * 2 + 3); - EXPECT_EQ(analysis.blocks[0].stack_req, 3); - EXPECT_EQ(analysis.blocks[0].stack_max_growth, 7); + EXPECT_EQ(analysis.instrs[0].arg.block.gas_cost, 7 * 3 + 10 * 2 + 3); + EXPECT_EQ(analysis.instrs[0].arg.block.stack_req, 3); + EXPECT_EQ(analysis.instrs[0].arg.block.stack_max_growth, 7); } TEST(analysis, push) @@ -82,7 +78,6 @@ TEST(analysis, jump1) const auto code = jump(add(4, 2)) + OP_JUMPDEST + mstore(0, 3) + ret(0, 0x20) + jump(6); const auto analysis = analyze(fake_fn_table, rev, &code[0], code.size()); - ASSERT_EQ(analysis.blocks.size(), 3); ASSERT_EQ(analysis.jumpdest_offsets.size(), 1); ASSERT_EQ(analysis.jumpdest_targets.size(), 1); EXPECT_EQ(analysis.jumpdest_offsets[0], 6); @@ -97,7 +92,6 @@ TEST(analysis, empty) bytes code; auto analysis = evmone::analyze(fake_fn_table, rev, &code[0], code.size()); - EXPECT_EQ(analysis.blocks.size(), 0); EXPECT_EQ(analysis.instrs.size(), 1); EXPECT_EQ(analysis.instrs.back().fn, fake_fn_table[OP_STOP]); } @@ -107,7 +101,6 @@ TEST(analysis, only_jumpdest) auto code = from_hex("5b"); auto analysis = evmone::analyze(fake_fn_table, rev, &code[0], code.size()); - ASSERT_EQ(analysis.blocks.size(), 1); ASSERT_EQ(analysis.jumpdest_offsets.size(), 1); ASSERT_EQ(analysis.jumpdest_targets.size(), 1); EXPECT_EQ(analysis.jumpdest_offsets[0], 0); @@ -119,6 +112,5 @@ TEST(analysis, jumpi_at_the_end) auto code = from_hex("57"); auto analysis = evmone::analyze(fake_fn_table, rev, &code[0], code.size()); - EXPECT_EQ(analysis.blocks.size(), 1); EXPECT_EQ(analysis.instrs.back().fn, fake_fn_table[OP_STOP]); } \ No newline at end of file diff --git a/test/utils/dump.cpp b/test/utils/dump.cpp index 0ac977841e..69ea4cdb39 100644 --- a/test/utils/dump.cpp +++ b/test/utils/dump.cpp @@ -28,7 +28,7 @@ void dump_analysis(const evmone::code_analysis& analysis) if (c == OPX_BEGINBLOCK) { - block = &analysis.blocks[size_t(instr.arg.p.number)]; + block = &instr.arg.block; const auto get_jumpdest_offset = [&analysis](size_t index) noexcept {