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.

Sign up for GitHub

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

Jump to bottom

EOF: less restricted stack validation #676

Merged
merged 10 commits into from
Feb 21, 2024
Merged

EOF: less restricted stack validation #676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5a113a2
eof: Stack validation with sequential pass without worklist
gumb0 Feb 20, 2024
2ac05ac
eof: Loosen constant stack height requirement
gumb0 Feb 20, 2024
6c0a06d
Add RJUMP*, RETF, JUMPF stack validation tests and underflow tests
gumb0 Feb 20, 2024
ea26895
Add more stack overflow validation tests
gumb0 Feb 16, 2024
fa3b4a6
Add tests for unreachable code and no terminating instruction
gumb0 Feb 13, 2024
25de43b
Fix tests cases with unreachable sections
gumb0 Feb 7, 2024
202e9a8
Add more JUMPF stack validation test cases
gumb0 Feb 16, 2024
ab19da5
Add test case for RETF reached via different paths
gumb0 Feb 16, 2024
5887f02
Add test cases for maximally broad stack range and jumps to self
gumb0 Feb 21, 2024
9a8e160
Update state tests
gumb0 Feb 21, 2024
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
2 changes: 1 addition & 1 deletion circle.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ jobs:
~/tests/EIPTests/BlockchainTests/
- download_execution_tests:
repo: ipsilon/tests
rev: eof-unreachable-cs-20240122
rev: eof-relaxed-stack-validation-20240221
legacy: false
- run:
name: "State tests (EOF)"
Expand Down
102 changes: 62 additions & 40 deletions lib/evmone/eof.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,40 +349,46 @@ bool validate_rjump_destinations(bytes_view code) noexcept
std::variant<EOFValidationError, int32_t> validate_max_stack_height(
bytes_view code, size_t func_index, const std::vector<EOFCodeType>& code_types)
{
assert(!code.empty());

// Special values used for detecting errors.
// Special value used for detecting errors.
static constexpr int32_t LOC_UNVISITED = -1; // Unvisited byte.
static constexpr int32_t LOC_IMMEDIATE = -2; // Immediate byte.

// Stack height in the header is limited to uint16_t,
// but keeping larger size for ease of calculation.
std::vector<int32_t> stack_heights(code.size(), LOC_UNVISITED);
stack_heights[0] = code_types[func_index].inputs;
struct StackHeightRange
{
int32_t min = LOC_UNVISITED;
int32_t max = LOC_UNVISITED;
chfast marked this conversation as resolved.
Show resolved Hide resolved

std::stack<size_t> worklist;
worklist.push(0);
[[nodiscard]] bool visited() const noexcept { return min != LOC_UNVISITED; }
};

while (!worklist.empty())
{
const auto i = worklist.top();
worklist.pop();
assert(!code.empty());

std::vector<StackHeightRange> stack_heights(code.size());
stack_heights[0] = {code_types[func_index].inputs, code_types[func_index].inputs};

for (size_t i = 0; i < code.size();)
{
const auto opcode = static_cast<Opcode>(code[i]);

int stack_height_required = instr::traits[opcode].stack_height_required;
auto stack_height_change = instr::traits[opcode].stack_height_change;

auto stack_height = stack_heights[i];
assert(stack_height != LOC_UNVISITED);
const auto stack_height = stack_heights[i];
if (!stack_height.visited())
{
// We reached the code that was neither referenced by previous forward jump,
// nor is part of sequential instruction flow. This is not allowed.
return EOFValidationError::unreachable_instructions;
chfast marked this conversation as resolved.
Show resolved Hide resolved
}

if (opcode == OP_CALLF)
{
const auto fid = read_uint16_be(&code[i + 1]);

stack_height_required = code_types[fid].inputs;

if (stack_height + code_types[fid].max_stack_height - stack_height_required >
if (stack_height.max + code_types[fid].max_stack_height - stack_height_required >
STACK_SIZE_LIMIT)
return EOFValidationError::stack_overflow;

Expand All @@ -395,7 +401,7 @@ std::variant<EOFValidationError, int32_t> validate_max_stack_height(
{
const auto fid = read_uint16_be(&code[i + 1]);

if (stack_height + code_types[fid].max_stack_height - code_types[fid].inputs >
if (stack_height.max + code_types[fid].max_stack_height - code_types[fid].inputs >
STACK_SIZE_LIMIT)
return EOFValidationError::stack_overflow;

Expand All @@ -410,46 +416,63 @@ std::variant<EOFValidationError, int32_t> validate_max_stack_height(

stack_height_required = code_types[func_index].outputs + code_types[fid].inputs -
code_types[fid].outputs;
if (stack_heights[i] > stack_height_required)

// JUMPF to returning function requires exact number of stack items
// and is allowed only in constant stack segment.
if (stack_height.max > stack_height_required)
return EOFValidationError::stack_higher_than_outputs_required;
}
}
else if (opcode == OP_RETF)
{
stack_height_required = code_types[func_index].outputs;
if (stack_height > code_types[func_index].outputs)
// RETF allowed only in constant stack segment
if (stack_height.max > stack_height_required)
return EOFValidationError::stack_higher_than_outputs_required;
}
else if (opcode == OP_DUPN)
stack_height_required = code[i + 1] + 1;
else if (opcode == OP_SWAPN)
stack_height_required = code[i + 1] + 2;

if (stack_height < stack_height_required)
if (stack_height.min < stack_height_required)
return EOFValidationError::stack_underflow;

stack_height += stack_height_change;
const StackHeightRange next_stack_height{
stack_height.min + stack_height_change, stack_height.max + stack_height_change};

// Determine size of immediate, including the special case of RJUMPV.
const size_t imm_size = (opcode == OP_RJUMPV) ?
(1 + /*count*/ (size_t{code[i + 1]} + 1) * REL_OFFSET_SIZE) :
instr::traits[opcode].immediate_size;

// Mark immediate locations.
std::fill_n(&stack_heights[i + 1], imm_size, LOC_IMMEDIATE);

// Validates the successor instruction and updates its stack height.
const auto validate_successor = [&stack_heights, &worklist](size_t successor_offset,
int32_t expected_stack_height) {
const auto visit_successor = [&stack_heights](size_t current_offset,
size_t successor_offset,
StackHeightRange required_stack_height) {
auto& successor_stack_height = stack_heights[successor_offset];
if (successor_stack_height == LOC_UNVISITED)
if (successor_offset <= current_offset) // backwards jump
Copy link
Member

Choose a reason for hiding this comment

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

Handling the successor_offset == current_offset case here is slightly confusing. Maybe just separate this case and handle it as no-op? Or at least a comment would be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not a no-op (good that tests detected this), I've added a comment why.

Copy link
Member

Choose a reason for hiding this comment

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

By no-op I mean that the validity condition is always true because in this case successor_stack_height is required_stack_height. Or I'm missing something.

We could handle this separately for clarity and test coverage visibility. As follows:

if (successor_offset == current_offset) // self-referencing jump
    return true;

But I'm insisting on this. I'm mostly verifying my understanding.

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, you're missing something as I tried to explain in the comment. The stack height still needs to be checked in this case.

Here's the code that would be broken: PUSH0 RJUMPI(-3) STOP - RJUMPI(-3) generates equality case, and without the check it would be valid, but it is invalid because stack after RJUMPI is not equal to stack before RJUMPI (requires 1 item)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, cool.

{
successor_stack_height = expected_stack_height;
worklist.push(successor_offset);
return true;
// successor_offset == current_offset case is possible only with jump into the same
// jump instruction, e.g. RJUMP(-3), so it is technically a backwards jump, too.
assert(successor_stack_height.visited());
// The spec could have been relaxed to
// return successor_stack_height.min >= required_stack_height.min &&
// successor_stack_height.max <= required_stack_height.max;
// but it was decided to have strict equality for simplicity.
return successor_stack_height.min == required_stack_height.min &&
successor_stack_height.max == required_stack_height.max;
}
else
return successor_stack_height == expected_stack_height;
else if (!successor_stack_height.visited()) // forwards jump, new target
successor_stack_height = required_stack_height;
else // forwards jump, target known
{
successor_stack_height.min =
std::min(required_stack_height.min, successor_stack_height.min);
successor_stack_height.max =
std::max(required_stack_height.max, successor_stack_height.max);
}
return true;
};

const auto next = i + imm_size + 1; // Offset of the next instruction (may be invalid).
Expand All @@ -459,7 +482,7 @@ std::variant<EOFValidationError, int32_t> validate_max_stack_height(
{
if (next >= code.size())
return EOFValidationError::no_terminating_instruction;
if (!validate_successor(next, stack_height))
if (!visit_successor(i, next, next_stack_height))
return EOFValidationError::stack_height_mismatch;
}

Expand All @@ -468,7 +491,7 @@ std::variant<EOFValidationError, int32_t> validate_max_stack_height(
{
const auto target_rel_offset = read_int16_be(&code[i + 1]);
const auto target = static_cast<int32_t>(i) + target_rel_offset + 3;
if (!validate_successor(static_cast<size_t>(target), stack_height))
if (!visit_successor(i, static_cast<size_t>(target), next_stack_height))
return EOFValidationError::stack_height_mismatch;
}
else if (opcode == OP_RJUMPV)
Expand All @@ -480,18 +503,17 @@ std::variant<EOFValidationError, int32_t> validate_max_stack_height(
{
const auto target_rel_offset = read_int16_be(&code[i + k * REL_OFFSET_SIZE + 2]);
const auto target = static_cast<int32_t>(next) + target_rel_offset;
if (!validate_successor(static_cast<size_t>(target), stack_height))
if (!visit_successor(i, static_cast<size_t>(target), next_stack_height))
return EOFValidationError::stack_height_mismatch;
}
}
}

const auto max_stack_height = *std::max_element(stack_heights.begin(), stack_heights.end());

if (std::find(stack_heights.begin(), stack_heights.end(), LOC_UNVISITED) != stack_heights.end())
return EOFValidationError::unreachable_instructions;
i = next;
}

return max_stack_height;
const auto max_stack_height_it = std::max_element(stack_heights.begin(), stack_heights.end(),
[](StackHeightRange lhs, StackHeightRange rhs) noexcept { return lhs.max < rhs.max; });
return max_stack_height_it->max;
}

std::variant<EOF1Header, EOFValidationError> validate_eof1(
Expand Down
Loading