From e67c498ee8504ca8ec2adde6b5f7364891a2c91d Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Mon, 11 May 2020 18:04:58 +0100 Subject: [PATCH 1/4] Add max_align to InstructionMetrics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Paweł Bylica --- lib/fizzy/instructions.cpp | 46 +++++++++++++++++++------------------- lib/fizzy/instructions.hpp | 19 ++++++++++++++++ 2 files changed, 42 insertions(+), 23 deletions(-) diff --git a/lib/fizzy/instructions.cpp b/lib/fizzy/instructions.cpp index a6c7e10df..0bcdc57dd 100644 --- a/lib/fizzy/instructions.cpp +++ b/lib/fizzy/instructions.cpp @@ -63,29 +63,29 @@ constexpr InstructionMetrics instruction_metrics_table[256] = { /* 0x27 */ {}, // 5.4.4 Memory instructions - /* i32_load = 0x28 */ {1, 0}, - /* i64_load = 0x29 */ {1, 0}, - /* f32_load = 0x2a */ {1, 0}, - /* f64_load = 0x2b */ {1, 0}, - /* i32_load8_s = 0x2c */ {1, 0}, - /* i32_load8_u = 0x2d */ {1, 0}, - /* i32_load16_s = 0x2e */ {1, 0}, - /* i32_load16_u = 0x2f */ {1, 0}, - /* i64_load8_s = 0x30 */ {1, 0}, - /* i64_load8_u = 0x31 */ {1, 0}, - /* i64_load16_s = 0x32 */ {1, 0}, - /* i64_load16_u = 0x33 */ {1, 0}, - /* i64_load32_s = 0x34 */ {1, 0}, - /* i64_load32_u = 0x35 */ {1, 0}, - /* i32_store = 0x36 */ {2, -2}, - /* i64_store = 0x37 */ {2, -2}, - /* f32_store = 0x38 */ {2, -2}, - /* f64_store = 0x39 */ {2, -2}, - /* i32_store8 = 0x3a */ {2, -2}, - /* i32_store16 = 0x3b */ {2, -2}, - /* i64_store8 = 0x3c */ {2, -2}, - /* i64_store16 = 0x3d */ {2, -2}, - /* i64_store32 = 0x3e */ {2, -2}, + /* i32_load = 0x28 */ {1, 0, 2}, + /* i64_load = 0x29 */ {1, 0, 3}, + /* f32_load = 0x2a */ {1, 0, 2}, + /* f64_load = 0x2b */ {1, 0, 3}, + /* i32_load8_s = 0x2c */ {1, 0, 0}, + /* i32_load8_u = 0x2d */ {1, 0, 0}, + /* i32_load16_s = 0x2e */ {1, 0, 1}, + /* i32_load16_u = 0x2f */ {1, 0, 1}, + /* i64_load8_s = 0x30 */ {1, 0, 0}, + /* i64_load8_u = 0x31 */ {1, 0, 0}, + /* i64_load16_s = 0x32 */ {1, 0, 1}, + /* i64_load16_u = 0x33 */ {1, 0, 1}, + /* i64_load32_s = 0x34 */ {1, 0, 2}, + /* i64_load32_u = 0x35 */ {1, 0, 2}, + /* i32_store = 0x36 */ {2, -2, 2}, + /* i64_store = 0x37 */ {2, -2, 3}, + /* f32_store = 0x38 */ {2, -2, 2}, + /* f64_store = 0x39 */ {2, -2, 3}, + /* i32_store8 = 0x3a */ {2, -2, 0}, + /* i32_store16 = 0x3b */ {2, -2, 1}, + /* i64_store8 = 0x3c */ {2, -2, 0}, + /* i64_store16 = 0x3d */ {2, -2, 1}, + /* i64_store32 = 0x3e */ {2, -2, 2}, /* memory_size = 0x3f */ {0, 1}, /* memory_grow = 0x40 */ {1, 0}, diff --git a/lib/fizzy/instructions.hpp b/lib/fizzy/instructions.hpp index f718eae81..207dce42e 100644 --- a/lib/fizzy/instructions.hpp +++ b/lib/fizzy/instructions.hpp @@ -4,6 +4,7 @@ #pragma once +#include #include namespace fizzy @@ -16,6 +17,24 @@ struct InstructionMetrics /// The stack height change caused by the instruction execution, /// i.e. stack height _after_ execution - stack height _before_ execution. int8_t stack_height_change; + + /// The largest acceptable alignment value satisfying `2 ** max_align < memory_width` where + /// memory_width is the number of bytes the instruction operates on. + /// + /// This field may contain invalid value for instructions not needing it. + uint8_t max_align; + + InstructionMetrics() = default; + + constexpr InstructionMetrics( + int8_t _stack_height_required, int8_t _stack_height_change, uint8_t _max_align = 0) noexcept + : stack_height_required(_stack_height_required), + stack_height_change(_stack_height_change), + max_align(_max_align) + { + // The valid range is between 0 and 3. + assert(max_align <= 3); + } }; const InstructionMetrics* get_instruction_metrics_table() noexcept; From 31987ed2a607877e112a58356b22611209de6bcf Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Mon, 11 May 2020 18:06:20 +0100 Subject: [PATCH 2/4] Validate i32/i64.load/store alignment --- lib/fizzy/parser_expr.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/fizzy/parser_expr.cpp b/lib/fizzy/parser_expr.cpp index 525b460b9..af56a44ae 100644 --- a/lib/fizzy/parser_expr.cpp +++ b/lib/fizzy/parser_expr.cpp @@ -678,8 +678,12 @@ parser_result parse_expr(const uint8_t* pos, const uint8_t* end, uint32_t case Instr::i64_store16: case Instr::i64_store32: { - // alignment - std::tie(std::ignore, pos) = leb128u_decode(pos, end); + uint32_t align; + std::tie(align, pos) = leb128u_decode(pos, end); + // NOTE: [0, 3] is the correct range (the hard limit is log2(64 / 8)) and checking it to + // avoid overflows + if (align > metrics.max_align) + throw validation_error{"alignment cannot exceed operand size"}; uint32_t offset; std::tie(offset, pos) = leb128u_decode(pos, end); From 963d729259e11c25b5b5bdfce7d7f1b713579e06 Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Mon, 11 May 2020 17:13:09 +0100 Subject: [PATCH 3/4] ci: update expectations --- circle.yml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/circle.yml b/circle.yml index 92eb400fe..6764d5d52 100644 --- a/circle.yml +++ b/circle.yml @@ -252,8 +252,8 @@ jobs: - benchmark: min_time: "0.01" - spectest: - expected_passed: 5106 - expected_failed: 326 + expected_passed: 5136 + expected_failed: 296 expected_skipped: 6381 sanitizers-macos: @@ -270,8 +270,8 @@ jobs: - benchmark: min_time: "0.01" - spectest: - expected_passed: 5106 - expected_failed: 326 + expected_passed: 5136 + expected_failed: 296 expected_skipped: 6381 benchmark: @@ -377,12 +377,12 @@ jobs: at: ~/build - spectest: skip_validation: true - expected_passed: 4482 - expected_failed: 8 + expected_passed: 4481 + expected_failed: 9 expected_skipped: 7323 - spectest: - expected_passed: 5106 - expected_failed: 326 + expected_passed: 5136 + expected_failed: 296 expected_skipped: 6381 workflows: From f8e939c18831a9b1c5f7ae07e157914902c22d75 Mon Sep 17 00:00:00 2001 From: Alex Beregszaszi Date: Tue, 2 Jun 2020 10:54:53 +0100 Subject: [PATCH 4/4] test: add validation for invalid alignment --- test/unittests/validation_test.cpp | 95 ++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/test/unittests/validation_test.cpp b/test/unittests/validation_test.cpp index 7f0c70126..c261b9a93 100644 --- a/test/unittests/validation_test.cpp +++ b/test/unittests/validation_test.cpp @@ -129,6 +129,101 @@ TEST(validation, memory_size_no_memory) parse(wasm), validation_error, "memory instructions require imported or defined memory"); } +TEST(validation, store_alignment) +{ + // NOTE: could use instruction_metrics here, but better to have two sources of truth for testing + const std::map test_cases{ + {Instr::i32_store8, 0}, {Instr::i32_store16, 1}, {Instr::i32_store, 2}, + // TODO: split these out once we have type validation + {Instr::i64_store8, 0}, {Instr::i64_store16, 1}, {Instr::i64_store32, 2}, + {Instr::i64_store, 3}, + // TODO: include floating point + }; + + for (const auto test_case : test_cases) + { + const auto instr = test_case.first; + const auto max_align = test_case.second; + // TODO: consider using leb128_encode and test 2^32-1 + for (auto align : {0, 1, 2, 3, 4, 0x7f}) + { + /* + (func (param i32) + get_local 0 + i32.const 0 + align= + */ + const auto type_section = make_vec({"60017f00"_bytes}); + const auto function_section = make_vec({"00"_bytes}); + // NOTE: this depends on align < 0x80 + const auto code_bin = bytes{0, // vec(locals) + uint8_t(Instr::local_get), 0, uint8_t(Instr::i32_const), 0, uint8_t(instr), + uint8_t(align), 0, uint8_t(Instr::end)}; + const auto code_section = make_vec({add_size_prefix(code_bin)}); + const auto memory_section = "01007f"_bytes; + const auto bin = bytes{wasm_prefix} + make_section(1, type_section) + + make_section(3, function_section) + make_section(5, memory_section) + + make_section(10, code_section); + if (align <= max_align) + { + EXPECT_NO_THROW(parse(bin)); + } + else + { + EXPECT_THROW_MESSAGE( + parse(bin), validation_error, "alignment cannot exceed operand size"); + } + } + } +} + +TEST(validation, load_alignment) +{ + // NOTE: could use instruction_metrics here, but better to have two sources of truth for testing + const std::map test_cases{ + {Instr::i32_load8_s, 0}, {Instr::i32_load8_u, 0}, {Instr::i32_load16_s, 1}, + {Instr::i32_load16_u, 1}, {Instr::i32_load, 2}, {Instr::i64_load8_s, 0}, + {Instr::i64_load8_u, 0}, {Instr::i64_load16_s, 1}, {Instr::i64_load16_u, 1}, + {Instr::i64_load32_s, 2}, {Instr::i64_load32_u, 2}, {Instr::i64_load, 3}, + // TODO: include floating point + }; + + for (const auto test_case : test_cases) + { + const auto instr = test_case.first; + const auto max_align = test_case.second; + // TODO: consider using leb128_encode and test 2^32-1 + for (auto align : {0, 1, 2, 3, 4, 0x7f}) + { + /* + (func (param i32) + get_local 0 + align= + */ + const auto type_section = make_vec({"60017f00"_bytes}); + const auto function_section = make_vec({"00"_bytes}); + // NOTE: this depends on align < 0x80 + const auto code_bin = bytes{0, // vec(locals) + uint8_t(Instr::local_get), 0, uint8_t(instr), uint8_t(align), 0, + uint8_t(Instr::drop), uint8_t(Instr::end)}; + const auto code_section = make_vec({add_size_prefix(code_bin)}); + const auto memory_section = "01007f"_bytes; + const auto bin = bytes{wasm_prefix} + make_section(1, type_section) + + make_section(3, function_section) + make_section(5, memory_section) + + make_section(10, code_section); + if (align <= max_align) + { + EXPECT_NO_THROW(parse(bin)); + } + else + { + EXPECT_THROW_MESSAGE( + parse(bin), validation_error, "alignment cannot exceed operand size"); + } + } + } +} + TEST(validation, br_invalid_label_index) { /* wat2wasm --no-check