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/analysis.cpp b/lib/evmone/analysis.cpp index 40303a7fe8..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. { @@ -90,9 +89,16 @@ 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 = 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; @@ -153,7 +159,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 +168,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..42ddcfc4be 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 @@ -84,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; @@ -99,7 +123,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; @@ -129,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"); @@ -159,22 +184,11 @@ 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. - int64_t gas_cost = 0; - - /// The stack height required to execute the block. - int stack_req = 0; - - /// The maximum stack height growth relative to the stack height at block start. - int stack_max_growth = 0; -}; +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/lib/evmone/limits.hpp b/lib/evmone/limits.hpp new file mode 100644 index 0000000000..b282d8aec1 --- /dev/null +++ b/lib/evmone/limits.hpp @@ -0,0 +1,16 @@ +// 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; + +/// 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; 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/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/unittests/evm_test.cpp b/test/unittests/evm_test.cpp index 596af4bdcc..091bd8c9c1 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, ""); @@ -1373,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); +} 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 {