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

Validate stack height after unreachable #384

Merged
merged 3 commits into from
Jun 17, 2020
Merged

Validate stack height after unreachable #384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db9df39
Validate stack height in blocks with unreachable
gumb0 Jun 10, 2020
d07693c
ci: update spectest expectations
gumb0 Jun 10, 2020
9b6d045
test: Add more stack validation tests with unreachable
gumb0 Jun 10, 2020
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
12 changes: 6 additions & 6 deletions circle.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
75 changes: 52 additions & 23 deletions lib/fizzy/parser_expr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,28 +93,39 @@ void update_caller_frame(ControlFrame& frame, const FuncType& func_type)
{
const auto stack_height_required = static_cast<int>(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<int>(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<int>(func_type.outputs.size());
}
else
throw validation_error{"call/call_indirect instruction stack underflow"};
}
else
{
const auto stack_height_change =
static_cast<int>(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
Expand All @@ -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<Code> parse_expr(
Expand Down Expand Up @@ -162,21 +179,32 @@ parser_result<Code> 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)
Copy link
Collaborator Author

@gumb0 gumb0 Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If we pull this max_stack_height update out of outer if, it looks like we can unify stack height update/checks for instructions and for calls, not sure if it's worth it. The error message for stack underflow would not be specific for calls then...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can be done in separate PR to confirm coverage of this version is full.

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<Instr>(opcode);
switch (instr)
Expand Down Expand Up @@ -264,9 +292,10 @@ parser_result<Code> 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:
Expand Down Expand Up @@ -463,7 +492,7 @@ parser_result<Code> parse_expr(
push_branch_immediates(branch_frame, code.immediates);

if (instr == Instr::br)
frame.unreachable = true;
mark_frame_unreachable(frame);

break;
}
Expand Down Expand Up @@ -503,7 +532,7 @@ parser_result<Code> 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;
}
Expand All @@ -519,7 +548,7 @@ parser_result<Code> parse_expr(

push_branch_immediates(control_stack[label_idx], code.immediates);

frame.unreachable = true;
mark_frame_unreachable(frame);
break;
}

Expand Down
122 changes: 117 additions & 5 deletions test/unittests/validation_stack_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -502,10 +543,9 @@ 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).
// It is not allowed to have additional items in polymorphic stack (after unreachable).

/* wat2wasm --no-check
(func
Expand All @@ -514,7 +554,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)
Expand All @@ -526,7 +566,79 @@ 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");

/* 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)
Expand Down