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 i32/i64.load/store alignment #310

Merged
merged 4 commits into from
Jun 26, 2020
Merged

Validate i32/i64.load/store alignment #310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e67c498
Add max_align to InstructionMetrics
axic May 11, 2020
31987ed
Validate i32/i64.load/store alignment
axic May 11, 2020
963d729
ci: update expectations
axic May 11, 2020
f8e939c
test: add validation for invalid alignment
axic Jun 2, 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
16 changes: 8 additions & 8 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: 5106
expected_failed: 326
expected_passed: 5136
expected_failed: 296
expected_skipped: 6381

sanitizers-macos:
Expand All @@ -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:
Expand Down Expand Up @@ -377,12 +377,12 @@ jobs:
at: ~/build
- spectest:
skip_validation: true
expected_passed: 4482
expected_failed: 8
expected_passed: 4481
expected_failed: 9
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is due to another shortcoming in the spectest, will submit an issue.

expected_skipped: 7323
- spectest:
expected_passed: 5106
expected_failed: 326
expected_passed: 5136
expected_failed: 296
expected_skipped: 6381

workflows:
Expand Down
46 changes: 23 additions & 23 deletions lib/fizzy/instructions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},

Expand Down
19 changes: 19 additions & 0 deletions lib/fizzy/instructions.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#pragma once

#include <cassert>
#include <cstdint>

namespace fizzy
Expand All @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's default-initialized to 0, I wouldn't call it invalid

Copy link
Member Author

Choose a reason for hiding this comment

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

0 is invalid on non-memory ops, because the change preferred by @chfast makes 0 a valid value (1<<0 = 1).

Copy link
Collaborator

Choose a reason for hiding this comment

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

0 is valid value for memory instructions, and nonrelevant for non-memory instructions. To me it never contains "invalid value" like garbage.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does contain invalid value for non-memory ops.

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;
Expand Down
8 changes: 6 additions & 2 deletions lib/fizzy/parser_expr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,8 +678,12 @@ parser_result<Code> 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<uint32_t>(pos, end);
uint32_t align;
axic marked this conversation as resolved.
Show resolved Hide resolved
std::tie(align, pos) = leb128u_decode<uint32_t>(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<uint32_t>(pos, end);
Expand Down
95 changes: 95 additions & 0 deletions test/unittests/validation_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Instr, int> 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
<instr> align=<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<Instr, int> 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
<instr> align=<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
Expand Down