Merge pull request #663 from ethereum/dataloadn-truncated-imm

Fix DATALOADN truncated immediate validation
ethereum · Jun 18, 2023 · 367c51c · 367c51c
2 parents 6e2a589 + 6c08a08
commit 367c51c
Show file tree

Hide file tree

Showing 4 changed files with 18 additions and 8 deletions.
diff --git a/lib/evmone/eof.cpp b/lib/evmone/eof.cpp
@@ -212,15 +212,17 @@ EOFValidationError validate_instructions(
         if (cost_table[op] == instr::undefined)
             return EOFValidationError::undefined_instruction;
 
+        if (i + instr::traits[op].immediate_size >= code.size())
+            return EOFValidationError::truncated_instruction;
+
         if (op == OP_RJUMPV)
         {
-            if (i + 1 >= code.size())
-                return EOFValidationError::truncated_instruction;
-
             const auto count = code[i + 1];
             if (count < 1)
                 return EOFValidationError::invalid_rjumpv_count;
             i += static_cast<size_t>(1 /* count */ + count * 2 /* tbl */);
+            if (i >= code.size())
+                return EOFValidationError::truncated_instruction;
         }
         else if (op == OP_DATALOADN)
         {
@@ -231,9 +233,6 @@ EOFValidationError validate_instructions(
         }
         else
             i += instr::traits[op].immediate_size;
-
-        if (i >= code.size())
-            return EOFValidationError::truncated_instruction;
     }
 
     return EOFValidationError::success;
@@ -275,7 +274,7 @@ bool validate_rjump_destinations(bytes_view code) noexcept
         else if (op == OP_RJUMPV)
         {
             const auto count = code[i + 1];
-            imm_size += size_t{1} /* count */ + count * REL_OFFSET_SIZE /* tbl */;
+            imm_size += count * REL_OFFSET_SIZE /* tbl */;
             const size_t post_pos = i + 1 + imm_size;
 
             for (size_t k = 0; k < count * REL_OFFSET_SIZE; k += REL_OFFSET_SIZE)

diff --git a/lib/evmone/instructions_traits.hpp b/lib/evmone/instructions_traits.hpp
@@ -301,7 +301,7 @@ constexpr inline std::array<Traits, 256> traits = []() noexcept {
     table[OP_RJUMP] = {"RJUMP", 2, false, 0, 0, EVMC_CANCUN};
     table[OP_RJUMPI] = {"RJUMPI", 2, false, 1, -1, EVMC_CANCUN};
     table[OP_RJUMPV] = {
-        "RJUMPV", 0 /* WARNING: immediate_size is dynamic */, false, 1, -1, EVMC_CANCUN};
+        "RJUMPV", 1 /* 1 byte static immediate + dynamic immediate */, false, 1, -1, EVMC_CANCUN};
 
     table[OP_PUSH0] = {"PUSH0", 0, false, 0, 1, EVMC_SHANGHAI};
 

diff --git a/test/unittests/eof_validation_test.cpp b/test/unittests/eof_validation_test.cpp
@@ -1248,6 +1248,15 @@ TEST(eof_validation, too_many_code_sections)
     EXPECT_EQ(validate_eof(code), EOFValidationError::too_many_code_sections);
 }
 
+TEST(eof_validation, EOF1_dataloadn_truncated)
+{
+    EXPECT_EQ(validate_eof("EF0001 010004 0200010001 030000 00 00000000 B9"),
+        EOFValidationError::truncated_instruction);
+
+    EXPECT_EQ(validate_eof("EF0001 010004 0200010002 030000 00 00000000 B900"),
+        EOFValidationError::truncated_instruction);
+}
+
 TEST(eof_validation, dataloadn)
 {
     // DATALOADN{0}

diff --git a/test/unittests/instructions_test.cpp b/test/unittests/instructions_test.cpp
@@ -58,6 +58,8 @@ constexpr void validate_traits_of() noexcept
         static_assert(tr.immediate_size == Op - OP_PUSH1 + 1);
     else if constexpr (Op == OP_RJUMP || Op == OP_RJUMPI || Op == OP_CALLF)
         static_assert(tr.immediate_size == 2);
+    else if constexpr (Op == OP_RJUMPV)
+        static_assert(tr.immediate_size == 1);
     else if constexpr (Op == OP_DUPN || Op == OP_SWAPN)
         static_assert(tr.immediate_size == 1);
     else if constexpr (Op == OP_DATALOADN)