EOF: less restricted stack validation #676

gumb0 · 2023-07-26T19:15:51Z

https://notes.ethereum.org/Fk1TksgjTxOotvQNL07mEQ#Less-restricted-validation-algorithm

codecov · 2023-12-04T17:54:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (534d4e4) 97.88% compared to head (9a8e160) 97.98%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #676      +/-   ##
==========================================
+ Coverage   97.88%   97.98%   +0.09%     
==========================================
  Files         113      113              
  Lines       10631    11155     +524     
==========================================
+ Hits        10406    10930     +524     
  Misses        225      225

Flag	Coverage Δ
blockchaintests	`59.70% <0.00%> (-0.18%)`	⬇️
statetests	`62.18% <96.87%> (+0.01%)`	⬆️
statetests-silkpre	`24.48% <0.00%> (-1.14%)`	⬇️
unittests	`96.07% <100.00%> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
lib/evmone/eof.cpp	`83.56% <100.00%> (-0.08%)`	⬇️
test/unittests/eof_stack_validation_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/eof_validation_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/evm_eof_rjump_test.cpp	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

gumb0 · 2024-02-17T12:29:26Z

lib/evmone/eof.cpp

+                return expected_stack_height.min == successor_stack_height.min &&
+                       expected_stack_height.max == successor_stack_height.max;


The spec could have been relaxed to

Suggested change

return expected_stack_height.min == successor_stack_height.min &&

expected_stack_height.max == successor_stack_height.max;

return expected_stack_height.min >= successor_stack_height.min &&

expected_stack_height.max <= successor_stack_height.max;

but it was decided to have strict equality for simlicity.

Leave this comment in the code.

Also, I'd swap the arguments: successor_stack_height.min <= required_stack_height.

pdobacz

I thought for a while what other test cases we could have. Some ideas, choose which make sense in your opinion:

RJUMP(IV) sequence of maximum possible length, doing hops from one to the other
RJUMPI(V) sequence of maximum possible length, all targeting same instruction
stack_height.min/max range maximally broad
maximum number of code sections
CALLF sequence of maximum possible length
[RJUMPI, RETF] sequence of maximum ...
[RJUMPI, JUMPF] sequence ...

For each of these situations test if stack validation behaves - one valid code, one invalid.

pdobacz · 2024-02-19T08:17:31Z

lib/evmone/eof.cpp

+        int32_t max = 0;
+    };
+
+    assert(!code.empty());

    // Stack height in the header is limited to uint16_t,


this comment should be moved above

lib/evmone/eof.cpp

chfast · 2024-02-19T12:29:10Z

test/unittests/eof_validation.cpp

        EXPECT_EQ(evmone::validate_eof(rev, test_case.container), test_case.error)
-            << test_case.name << "\n"
+            << "test case " << i << " " << test_case.name << "\n"


Maybe use Go convention test_name/index but too late now.

lib/evmone/eof.cpp

chfast · 2024-02-20T08:54:53Z

test/unittests/eof_stack_validation_test.cpp

+namespace
+{
+// code prologue that creates a segment starting with possible stack heights 1 and 3
+const auto varstack = push0() + rjumpi(2, 0) + OP_PUSH0 + OP_PUSH0;


Suggested change

const auto varstack = push0() + rjumpi(2, 0) + OP_PUSH0 + OP_PUSH0;

const auto varstack = push0() + rjumpi(2, 0) + push0() + push0();

chfast · 2024-02-20T08:56:08Z

test/unittests/eof_stack_validation_test.cpp

+
+    // STOP reachable only via backwards jump - invalid
+    add_test_case(
+        eof_bytecode(rjump(1) + OP_STOP + rjump(-4)), EOFValidationError::unreachable_instructions);


In this case the error may be confusing. Any ideas?

I think we wouldn't be able to distinguish really unreachable instructions from those reachable only via backwards jump (because we validate in a linear forwards pass)

So we could only make this error message more general, something like instruction_not_reachabe_via_forward_control_flow or even more general invalid_instruction_order. I don't have better ideas yet.

Let's leave it for later.

test/unittests/eof_stack_validation_test.cpp

chfast · 2024-02-20T09:10:17Z

lib/evmone/eof.cpp

-    {
-        const auto i = worklist.top();
-        worklist.pop();
+    std::vector<StackHeightRange> stack_heights(code.size(), {LOC_UNVISITED, LOC_UNVISITED});


Suggested change

std::vector<StackHeightRange> stack_heights(code.size(), {LOC_UNVISITED, LOC_UNVISITED});

std::vector<StackHeightRange> stack_heights(code.size());

chfast · 2024-02-20T09:21:04Z

lib/evmone/eof.cpp

+                return expected_stack_height.min == successor_stack_height.min &&
+                       expected_stack_height.max == successor_stack_height.max;


Leave this comment in the code.

Also, I'd swap the arguments: successor_stack_height.min <= required_stack_height.

chfast · 2024-02-20T09:25:17Z

lib/evmone/eof.cpp

            auto& successor_stack_height = stack_heights[successor_offset];
-            if (successor_stack_height == LOC_UNVISITED)
+            if (successor_offset <= current_offset)  // backwards jump


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.

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

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.

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)

chfast · 2024-02-20T09:32:28Z

lib/evmone/eof.cpp

        // 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 validate_successor = [&stack_heights](size_t current_offset,


Suggested change

const auto validate_successor = [&stack_heights](size_t current_offset,

const auto visit_successor = [&stack_heights](size_t current_offset,

The name "validate" suggests this a "constant" function - no update.

lib/evmone/eof.cpp

chfast · 2024-02-20T18:00:42Z

lib/evmone/eof.cpp

            auto& successor_stack_height = stack_heights[successor_offset];
-            if (successor_stack_height == LOC_UNVISITED)
+            if (successor_offset <= current_offset)  // backwards jump


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.

lib/evmone/eof.cpp

chfast · 2024-02-20T18:05:14Z

test/unittests/eof_stack_validation_test.cpp

+
+    // STOP reachable only via backwards jump - invalid
+    add_test_case(
+        eof_bytecode(rjump(1) + OP_STOP + rjump(-4)), EOFValidationError::unreachable_instructions);


Let's leave it for later.

lib/evmone/eof.cpp

chfast · 2024-02-20T20:20:29Z

lib/evmone/eof.cpp

            auto& successor_stack_height = stack_heights[successor_offset];
-            if (successor_stack_height == LOC_UNVISITED)
+            if (successor_offset <= current_offset)  // backwards jump


pdobacz

👍

gumb0 added the EOF label Jul 26, 2023

gumb0 force-pushed the eof-loosened-stack-validation branch from bd20842 to 39d3fef Compare July 27, 2023 13:55

gumb0 force-pushed the eof-loosened-stack-validation branch from 39d3fef to cd39111 Compare August 11, 2023 15:06

gumb0 force-pushed the eof-loosened-stack-validation branch from cd39111 to 6de389a Compare September 1, 2023 13:21

gumb0 mentioned this pull request Sep 20, 2023

EOF Implementers Call #23 ethereum/pm#864

Closed

gumb0 force-pushed the eof-loosened-stack-validation branch from 6de389a to 296ce1e Compare December 4, 2023 17:48

gumb0 force-pushed the eof-loosened-stack-validation branch 7 times, most recently from e86d2a5 to 84c5142 Compare December 11, 2023 15:18

gumb0 force-pushed the eof-loosened-stack-validation branch 12 times, most recently from 18dd783 to 7a33c80 Compare December 20, 2023 11:17

gumb0 force-pushed the eof-loosened-stack-validation branch 4 times, most recently from 63ad029 to ac9cf2b Compare December 21, 2023 19:55

gumb0 requested a review from rodiazet February 16, 2024 17:04

gumb0 force-pushed the eof-loosened-stack-validation branch from c1b4f82 to 7ec8abb Compare February 17, 2024 11:34

gumb0 commented Feb 17, 2024

View reviewed changes

pdobacz approved these changes Feb 19, 2024

View reviewed changes

chfast reviewed Feb 19, 2024

View reviewed changes

gumb0 force-pushed the eof-loosened-stack-validation branch from 7ec8abb to f424a76 Compare February 19, 2024 14:12

gumb0 requested a review from chfast February 19, 2024 14:21

chfast reviewed Feb 20, 2024

View reviewed changes

eof: Stack validation with sequential pass without worklist

5a113a2

gumb0 force-pushed the eof-loosened-stack-validation branch 3 times, most recently from d267377 to 64716ef Compare February 20, 2024 17:38

gumb0 requested a review from chfast February 20, 2024 17:41

chfast reviewed Feb 20, 2024

View reviewed changes

gumb0 added 4 commits February 20, 2024 21:17

eof: Loosen constant stack height requirement

2ac05ac

Add RJUMP*, RETF, JUMPF stack validation tests and underflow tests

6c0a06d

Add more stack overflow validation tests

ea26895

Add tests for unreachable code and no terminating instruction

fa3b4a6

gumb0 force-pushed the eof-loosened-stack-validation branch from 64716ef to 786c173 Compare February 20, 2024 20:18

chfast approved these changes Feb 20, 2024

View reviewed changes

pdobacz approved these changes Feb 21, 2024

View reviewed changes

gumb0 added 5 commits February 21, 2024 11:20

Fix tests cases with unreachable sections

25de43b

Add more JUMPF stack validation test cases

202e9a8

Add test case for RETF reached via different paths

ab19da5

Add test cases for maximally broad stack range and jumps to self

5887f02

Update state tests

9a8e160

gumb0 force-pushed the eof-loosened-stack-validation branch from 1837002 to 9a8e160 Compare February 21, 2024 10:21

chfast approved these changes Feb 21, 2024

View reviewed changes

gumb0 merged commit 34c0169 into master Feb 21, 2024
25 checks passed

gumb0 deleted the eof-loosened-stack-validation branch February 21, 2024 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EOF: less restricted stack validation #676

EOF: less restricted stack validation #676

gumb0 commented Jul 26, 2023 •

edited

Loading

codecov bot commented Dec 4, 2023 •

edited

Loading

gumb0 Feb 17, 2024 •

edited

Loading

chfast Feb 20, 2024

pdobacz left a comment

pdobacz Feb 19, 2024

chfast Feb 19, 2024

chfast Feb 20, 2024

chfast Feb 20, 2024

gumb0 Feb 20, 2024

chfast Feb 20, 2024

chfast Feb 20, 2024

chfast Feb 20, 2024

chfast Feb 20, 2024

gumb0 Feb 20, 2024

chfast Feb 20, 2024

gumb0 Feb 20, 2024

chfast Feb 20, 2024

chfast Feb 20, 2024

chfast Feb 20, 2024

chfast Feb 20, 2024

chfast Feb 20, 2024

pdobacz left a comment

		return expected_stack_height.min == successor_stack_height.min &&
		expected_stack_height.max == successor_stack_height.max;

	const auto varstack = push0() + rjumpi(2, 0) + OP_PUSH0 + OP_PUSH0;
	const auto varstack = push0() + rjumpi(2, 0) + push0() + push0();

	std::vector<StackHeightRange> stack_heights(code.size(), {LOC_UNVISITED, LOC_UNVISITED});
	std::vector<StackHeightRange> stack_heights(code.size());

	const auto validate_successor = [&stack_heights](size_t current_offset,
	const auto visit_successor = [&stack_heights](size_t current_offset,

EOF: less restricted stack validation #676

EOF: less restricted stack validation #676

Conversation

gumb0 commented Jul 26, 2023 • edited Loading

codecov bot commented Dec 4, 2023 • edited Loading

Codecov Report

gumb0 Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdobacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdobacz left a comment

Choose a reason for hiding this comment

gumb0 commented Jul 26, 2023 •

edited

Loading

codecov bot commented Dec 4, 2023 •

edited

Loading

gumb0 Feb 17, 2024 •

edited

Loading