From db9df397e60a2de810b801fc25aec5e653bf1adc Mon Sep 17 00:00:00 2001 From: Andrei Maiboroda Date: Wed, 10 Jun 2020 20:14:42 +0200 Subject: [PATCH 1/3] Validate stack height in blocks with unreachable --- lib/fizzy/parser_expr.cpp | 75 ++++++++++++++++-------- test/unittests/validation_stack_test.cpp | 6 +- 2 files changed, 55 insertions(+), 26 deletions(-) diff --git a/lib/fizzy/parser_expr.cpp b/lib/fizzy/parser_expr.cpp index f641e4991..bc0eab2d4 100644 --- a/lib/fizzy/parser_expr.cpp +++ b/lib/fizzy/parser_expr.cpp @@ -93,28 +93,39 @@ void update_caller_frame(ControlFrame& frame, const FuncType& func_type) { const auto stack_height_required = static_cast(func_type.inputs.size()); - if (!frame.unreachable && - (frame.stack_height - frame.parent_stack_height) < stack_height_required) - throw validation_error{"call/call_indirect instruction stack underflow"}; - - const auto stack_height_change = - static_cast(func_type.outputs.size()) - stack_height_required; - frame.stack_height += stack_height_change; + if (frame.stack_height - frame.parent_stack_height < stack_height_required) + { + // Stack is polymorphic after unreachable instruction: underflow is ignored, + // but we need to count stack growth to detect extra values at the end of the block. + if (frame.unreachable) + { + frame.stack_height = + frame.parent_stack_height + static_cast(func_type.outputs.size()); + } + else + throw validation_error{"call/call_indirect instruction stack underflow"}; + } + else + { + const auto stack_height_change = + static_cast(func_type.outputs.size()) - stack_height_required; + frame.stack_height += stack_height_change; + } } void validate_result_count(const ControlFrame& frame) { - if (frame.unreachable) - return; - // This is checked by "stack underflow". assert(frame.stack_height >= frame.parent_stack_height); - if (frame.stack_height < frame.parent_stack_height + frame.arity) - throw validation_error{"missing result"}; - if (frame.stack_height > frame.parent_stack_height + frame.arity) throw validation_error{"too many results"}; + + if (frame.unreachable) + return; + + if (frame.stack_height < frame.parent_stack_height + frame.arity) + throw validation_error{"missing result"}; } inline uint8_t get_branch_arity(const ControlFrame& frame) noexcept @@ -134,6 +145,12 @@ void push_branch_immediates(const ControlFrame& frame, bytes& immediates) push(immediates, get_branch_arity(frame)); } +inline void mark_frame_unreachable(ControlFrame& frame) noexcept +{ + frame.unreachable = true; + frame.stack_height = frame.parent_stack_height; +} + } // namespace parser_result parse_expr( @@ -162,21 +179,32 @@ parser_result parse_expr( auto& frame = control_stack.top(); const auto& metrics = metrics_table[opcode]; - if (!frame.unreachable) + if ((frame.stack_height - frame.parent_stack_height) < metrics.stack_height_required) { - if ((frame.stack_height - frame.parent_stack_height) < metrics.stack_height_required) + // Stack is polymorphic after unreachable instruction: underflow is ignored, + // but we need to count stack growth to detect extra values at the end of the block. + if (frame.unreachable) + { + // Add instruction's output values only. + frame.stack_height = frame.parent_stack_height + metrics.stack_height_required + + metrics.stack_height_change; + } + else throw validation_error{"stack underflow"}; - + } + else + { // Update code's max_stack_height using frame.stack_height of the previous instruction. // At this point frame.stack_height includes additional changes to the stack height // if the previous instruction is a call/call_indirect. // This way the update is skipped for end/else instructions (because their frame is // already popped/reset), but it does not matter, as these instructions do not modify // stack height anyway. - code.max_stack_height = std::max(code.max_stack_height, frame.stack_height); - } + if (!frame.unreachable) + code.max_stack_height = std::max(code.max_stack_height, frame.stack_height); - frame.stack_height += metrics.stack_height_change; + frame.stack_height += metrics.stack_height_change; + } const auto instr = static_cast(opcode); switch (instr) @@ -264,9 +292,10 @@ parser_result parse_expr( case Instr::i64_reinterpret_f64: case Instr::f32_reinterpret_i32: case Instr::f64_reinterpret_i64: + break; case Instr::unreachable: - frame.unreachable = true; + mark_frame_unreachable(frame); break; case Instr::nop: @@ -463,7 +492,7 @@ parser_result parse_expr( push_branch_immediates(branch_frame, code.immediates); if (instr == Instr::br) - frame.unreachable = true; + mark_frame_unreachable(frame); break; } @@ -503,7 +532,7 @@ parser_result parse_expr( default_branch_frame.br_immediate_offsets.push_back(code.immediates.size()); push_branch_immediates(default_branch_frame, code.immediates); - frame.unreachable = true; + mark_frame_unreachable(frame); break; } @@ -519,7 +548,7 @@ parser_result parse_expr( push_branch_immediates(control_stack[label_idx], code.immediates); - frame.unreachable = true; + mark_frame_unreachable(frame); break; } diff --git a/test/unittests/validation_stack_test.cpp b/test/unittests/validation_stack_test.cpp index 0940ac154..2babee5b2 100644 --- a/test/unittests/validation_stack_test.cpp +++ b/test/unittests/validation_stack_test.cpp @@ -502,7 +502,7 @@ TEST(validation_stack, unreachable_call_indirect) parse(wasm); } -TEST(validation_stack, DISABLED_unreachable_too_many_results) +TEST(validation_stack, unreachable_too_many_results) { // TODO: These are actually examples of invalid wasm. // It is not allowed to have additional items in polymorphic stack (after unreachable). @@ -514,7 +514,7 @@ TEST(validation_stack, DISABLED_unreachable_too_many_results) ) */ const auto wasm = from_hex("0061736d01000000010401600000030201000a070105000041010b"); - EXPECT_THROW_MESSAGE(parse(wasm), validation_error, ""); + EXPECT_THROW_MESSAGE(parse(wasm), validation_error, "too many results"); /* wat2wasm --no-check (func (param i32) (result i32) @@ -526,7 +526,7 @@ TEST(validation_stack, DISABLED_unreachable_too_many_results) */ const auto wasm2 = from_hex("0061736d0100000001060160017f017f030201000a0c010a0020000c00410141020b"); - EXPECT_THROW_MESSAGE(parse(wasm2), validation_error, ""); + EXPECT_THROW_MESSAGE(parse(wasm2), validation_error, "too many results"); } TEST(validation_stack, br) From d07693c648947b51559c17782f573e3eda7c0188 Mon Sep 17 00:00:00 2001 From: Andrei Maiboroda Date: Wed, 10 Jun 2020 19:57:23 +0200 Subject: [PATCH 2/3] ci: update spectest expectations --- circle.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/circle.yml b/circle.yml index fc253c9fe..87b70e8bf 100644 --- a/circle.yml +++ b/circle.yml @@ -252,8 +252,8 @@ jobs: - benchmark: min_time: "0.01" - spectest: - expected_passed: 4997 - expected_failed: 435 + expected_passed: 5018 + expected_failed: 414 expected_skipped: 6381 sanitizers-macos: @@ -270,8 +270,8 @@ jobs: - benchmark: min_time: "0.01" - spectest: - expected_passed: 4997 - expected_failed: 435 + expected_passed: 5018 + expected_failed: 414 expected_skipped: 6381 benchmark: @@ -381,8 +381,8 @@ jobs: expected_failed: 8 expected_skipped: 7323 - spectest: - expected_passed: 4997 - expected_failed: 435 + expected_passed: 5018 + expected_failed: 414 expected_skipped: 6381 workflows: From 9b6d0452eaa9753305db7845eb1ec0f843c602e6 Mon Sep 17 00:00:00 2001 From: Andrei Maiboroda Date: Wed, 10 Jun 2020 13:00:07 +0200 Subject: [PATCH 3/3] test: Add more stack validation tests with unreachable --- test/unittests/validation_stack_test.cpp | 116 ++++++++++++++++++++++- 1 file changed, 114 insertions(+), 2 deletions(-) diff --git a/test/unittests/validation_stack_test.cpp b/test/unittests/validation_stack_test.cpp index 2babee5b2..d3b53698f 100644 --- a/test/unittests/validation_stack_test.cpp +++ b/test/unittests/validation_stack_test.cpp @@ -465,6 +465,47 @@ TEST(validation_stack, unreachable_2) EXPECT_THAT(module.codesec[0].max_stack_height, 0); } +TEST(validation_stack, unreachable_3) +{ + /* wat2wasm + (func (result i32) + (block (result i32) unreachable) + ) + */ + const auto wasm = from_hex("0061736d010000000105016000017f030201000a08010600027f000b0b"); + const auto module = parse(wasm); + EXPECT_THAT(module.codesec[0].max_stack_height, 1); +} + +TEST(validation_stack, unreachable_4) +{ + /* wat2wasm + (func (result i32) + unreachable + (if (result i32) (then i32.const 0) (else i32.const 1)) + ) + */ + const auto wasm = + from_hex("0061736d010000000105016000017f030201000a0d010b0000047f41000541010b0b"); + const auto module = parse(wasm); + EXPECT_THAT(module.codesec[0].max_stack_height, 1); +} + +TEST(validation_stack, unreachable_5) +{ + /* wat2wasm + (func (result i32) + i64.const 0 + unreachable + i32.eqz + ) + */ + const auto wasm = from_hex("0061736d010000000105016000017f030201000a08010600420000450b"); + const auto module = parse(wasm); + EXPECT_THAT(module.codesec[0].max_stack_height, 1); +} + + TEST(validation_stack, unreachable_call) { /* wat2wasm @@ -504,8 +545,7 @@ TEST(validation_stack, unreachable_call_indirect) TEST(validation_stack, unreachable_too_many_results) { - // TODO: These are actually examples of invalid wasm. - // It is not allowed to have additional items in polymorphic stack (after unreachable). + // It is not allowed to have additional items in polymorphic stack (after unreachable). /* wat2wasm --no-check (func @@ -527,6 +567,78 @@ TEST(validation_stack, unreachable_too_many_results) const auto wasm2 = from_hex("0061736d0100000001060160017f017f030201000a0c010a0020000c00410141020b"); EXPECT_THROW_MESSAGE(parse(wasm2), validation_error, "too many results"); + + /* wat2wasm --no-check + (func (result i32) + unreachable + drop + i32.const 0 + i32.const 0 + ) + */ + const auto wasm3 = from_hex("0061736d010000000105016000017f030201000a0a010800001a410041000b"); + EXPECT_THROW_MESSAGE(parse(wasm3), validation_error, "too many results"); + + /* wat2wasm --no-check + (func (result i32) + unreachable + i32.add + i32.const 0 + ) + */ + const auto wasm4 = from_hex("0061736d010000000105016000017f030201000a08010600006a41000b"); + EXPECT_THROW_MESSAGE(parse(wasm4), validation_error, "too many results"); + + + /* wat2wasm --no-check + (func (result i32) + i32.const 0 + (block (result i32) + unreachable + drop + i32.const 0 + i32.const 0 + ) + drop + ) + */ + const auto wasm5 = + from_hex("0061736d010000000105016000017f030201000a10010e004100027f001a410041000b1a0b"); + EXPECT_THROW_MESSAGE(parse(wasm5), validation_error, "too many results"); + + /* wat2wasm --no-check + (func (param i32) (param i32) (result i32) + unreachable + call 0 + i32.const 0 + ) + */ + const auto wasm6 = from_hex("0061736d0100000001070160027f7f017f030201000a0901070000100041000b"); + EXPECT_THROW_MESSAGE(parse(wasm6), validation_error, "too many results"); + + /* wat2wasm --no-check + (table anyfunc (elem 0)) + (func (param i32) (param i32) (result i32) + unreachable + (call_indirect (type 0)) + i32.const 0 + ) + */ + const auto wasm7 = from_hex( + "0061736d0100000001070160027f7f017f03020100040501700101010907010041000b01000a0a010800001100" + "0041000b"); + EXPECT_THROW_MESSAGE(parse(wasm7), validation_error, "too many results"); + + /* wat2wasm --no-check + (func (result i32) + unreachable + i32.const 0 + (block (result i32) i32.const 0) + ) + */ + const auto wasm8 = + from_hex("0061736d010000000105016000017f030201000a0c010a00004100027f41000b0b"); + EXPECT_THROW_MESSAGE(parse(wasm8), validation_error, "too many results"); } TEST(validation_stack, br)