Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EOF code validation (EIP-3670) #366

Merged
merged 2 commits into from
Apr 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions lib/evmone/eof.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
// SPDX-License-Identifier: Apache-2.0

#include "eof.hpp"
#include "instructions_traits.hpp"

#include <array>
#include <cassert>
#include <limits>

namespace evmone
{
Expand Down Expand Up @@ -95,15 +97,46 @@ std::pair<EOFSectionHeaders, EOFValidationError> validate_eof_headers(bytes_view
return {section_headers, EOFValidationError::success};
}

std::pair<EOF1Header, EOFValidationError> validate_eof1(bytes_view container) noexcept
EOFValidationError validate_instructions(evmc_revision rev, bytes_view code) noexcept
{
const auto [section_headers, error] = validate_eof_headers(container);
if (error != EOFValidationError::success)
return {{}, error};
assert(code.size() > 0); // guaranteed by EOF headers validation

size_t i = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this an iterator instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because with an iterator loop end condition can be only it != code.end(), but loop can overrun end in case of truncated immediate.

uint8_t op = code[0];
while (i < code.size())
chfast marked this conversation as resolved.
Show resolved Hide resolved
{
op = code[i];
const auto& since = instr::traits[op].since;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is fine for now, but instr::traits has sub-optimal layout for runtime use.

if (!since.has_value() || *since > rev)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this doesn't cover the case when we want to remove an opcode (e.g.calldata) from EOF contracts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when that happens I guess we will need to add final revision to traits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This looks like a good solution if needed.

return EOFValidationError::undefined_instruction;

i += instr::traits[op].immediate_size;
++i;
}

if (!instr::traits[op].is_terminating)
chfast marked this conversation as resolved.
Show resolved Hide resolved
return EOFValidationError::missing_terminating_instruction;

return EOFValidationError::success;
}

std::pair<EOF1Header, EOFValidationError> validate_eof1(
evmc_revision rev, bytes_view container) noexcept
{
const auto [section_headers, error_header] = validate_eof_headers(container);
if (error_header != EOFValidationError::success)
return {{}, error_header};

EOF1Header header{section_headers[CODE_SECTION], section_headers[DATA_SECTION]};

const auto error_instr =
validate_instructions(rev, {&container[header.code_begin()], header.code_size});
if (error_instr != EOFValidationError::success)
return {{}, error_instr};

const EOF1Header header{section_headers[CODE_SECTION], section_headers[DATA_SECTION]};
return {header, EOFValidationError::success};
}

} // namespace

size_t EOF1Header::code_begin() const noexcept
Expand Down Expand Up @@ -154,7 +187,7 @@ EOFValidationError validate_eof(evmc_revision rev, bytes_view container) noexcep
{
if (rev < EVMC_SHANGHAI)
return EOFValidationError::eof_version_unknown;
return validate_eof1(container).second;
return validate_eof1(rev, container).second;
}
else
return EOFValidationError::eof_version_unknown;
Expand Down
2 changes: 2 additions & 0 deletions lib/evmone/eof.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ enum class EOFValidationError
zero_section_size,
section_headers_not_terminated,
invalid_section_bodies_size,
undefined_instruction,
missing_terminating_instruction,

impossible,
};
Expand Down
81 changes: 79 additions & 2 deletions test/unittests/eof_validation_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,20 @@
// SPDX-License-Identifier: Apache-2.0

#include <evmone/eof.hpp>
#include <evmone/instructions_traits.hpp>
#include <gtest/gtest.h>
#include <test/utils/bytecode.hpp>
#include <test/utils/utils.hpp>

using namespace evmone;

namespace
{
// Can be called as validate_eof(string_view hex, rev) or validate_eof(bytes_view cont, rev).
inline EOFValidationError validate_eof(
std::string_view code_hex, evmc_revision rev = EVMC_SHANGHAI) noexcept
const bytecode& container, evmc_revision rev = EVMC_SHANGHAI) noexcept
{
return ::validate_eof(rev, from_hex(code_hex));
return evmone::validate_eof(rev, container);
}
} // namespace

Expand Down Expand Up @@ -156,3 +159,77 @@ TEST(eof_validation, EOF1_trailing_bytes)
EXPECT_EQ(validate_eof("EF0001 010001 020002 00 FE AABB DEADBEEF"),
EOFValidationError::invalid_section_bodies_size);
}

TEST(eof_validation, EOF1_undefined_opcodes)
{
auto cont = from_hex("EF0001 010002 00 0000");

const auto& gas_table = evmone::instr::gas_costs[EVMC_SHANGHAI];

for (uint16_t opcode = 0; opcode <= 0xff; ++opcode)
{
// PUSH* require immediate argument to be valid, checked in a separate test
if (opcode >= OP_PUSH1 && opcode <= OP_PUSH32)
continue;

cont[cont.size() - 2] = static_cast<uint8_t>(opcode);

const auto expected = (gas_table[opcode] == evmone::instr::undefined ?
EOFValidationError::undefined_instruction :
EOFValidationError::success);
EXPECT_EQ(validate_eof(cont), expected) << hex(cont);
}

EXPECT_EQ(validate_eof(from_hex("EF0001 010001 00 FE")), EOFValidationError::success);
}

TEST(eof_validation, EOF1_truncated_push)
{
auto eof_header = from_hex("EF0001 010001 00");
auto& code_size_byte = eof_header[5];
for (uint8_t opcode = OP_PUSH1; opcode <= OP_PUSH32; ++opcode)
{
const auto required_bytes = static_cast<size_t>(opcode) - OP_PUSH1 + 1;
for (size_t i = 0; i < required_bytes; ++i)
{
const bytes code{opcode + bytes(i, 0)};
code_size_byte = static_cast<uint8_t>(code.size());
const auto container = eof_header + code;

EXPECT_EQ(validate_eof(container), EOFValidationError::missing_terminating_instruction)
<< hex(container);
}

const bytes code{opcode + bytes(required_bytes, 0) + uint8_t{OP_STOP}};
code_size_byte = static_cast<uint8_t>(code.size());
const auto container = eof_header + code;

EXPECT_EQ(validate_eof(container), EOFValidationError::success) << hex(container);
}
}

TEST(eof_validation, EOF1_terminating_instructions)
{
auto eof_header = from_hex("EF0001 010001 00");
auto& code_size_byte = eof_header[5];

const auto& traits = evmone::instr::traits;

for (uint16_t opcode = 0; opcode <= 0xff; ++opcode)
{
const auto& op_traits = traits[opcode];
// Skip undefined opcodes.
if (op_traits.name == nullptr)
continue;

bytes code{static_cast<uint8_t>(opcode) + bytes(op_traits.immediate_size, 0)};
code_size_byte = static_cast<uint8_t>(code.size());
const auto container = eof_header + code;

const auto expected = ((opcode == OP_STOP || opcode == OP_RETURN || opcode == OP_REVERT ||
opcode == OP_INVALID || opcode == OP_SELFDESTRUCT) ?
EOFValidationError::success :
EOFValidationError::missing_terminating_instruction);
EXPECT_EQ(validate_eof(container), expected) << hex(code);
}
}