Validate i32/i64.load/store alignment #310

axic · 2020-05-11T16:12:15Z

Based on section 3.3.4: https://webassembly.github.io/spec/core/_download/WebAssembly.pdf

axic · 2020-05-11T16:13:34Z

circle.yml

-          expected_passed: 4482
-          expected_failed: 8
+          expected_passed: 4481
+          expected_failed: 9


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

lib/fizzy/parser_expr.cpp

codecov · 2020-05-11T17:13:19Z

Codecov Report

Merging #310 into master will decrease coverage by 0.02%.
The diff coverage is 96.03%.

@@            Coverage Diff             @@
##           master     #310      +/-   ##
==========================================
- Coverage   99.32%   99.30%   -0.03%     
==========================================
  Files          42       43       +1     
  Lines       12834    12933      +99     
==========================================
+ Hits        12748    12843      +95     
- Misses         86       90       +4

lib/fizzy/parser_expr.cpp

lib/fizzy/instructions.hpp

gumb0

This needs a unit test

lib/fizzy/parser_expr.cpp

chfast · 2020-06-25T14:39:30Z

lib/fizzy/instructions.hpp

@@ -16,6 +16,18 @@ 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 memory width in bytes the instruction operates on or 0 in case not using the memory.
+    uint8_t memory_width;


I can see now that it would be simpler to keep this as max_align of {1,2,3,4}, as in your tests.

Maybe, but memory_width is also fitting and describes what it does.

I mean {0,1,2,3} so you don't need to pre-process it during validation:

if (align > metrics.max_align) throw validation_error{"alignment cannot exceed operand size"};

I see. Even if we change that I still wouldn't use instruction_metrics in the tests, otherwise the tests aren't that prone to mistakes :)

/// The largest acceptable alignment value satisfying `2 ** max_align < memory_width` where memory_width /// is the number of bytes the instruction operates on. max_align is 0 if it does not operate on memory. uint8_t max_align;

This is actually a bit weird because max_align = 0 is a valid value and with the current way memory_width = 0 would be invalid which is the default on non-memory instructions. It is only a nitpick. The difference in the parser is a single shift less.

Not an issue to me.

chfast · 2020-06-25T15:34:32Z

lib/fizzy/parser_expr.cpp

+            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 > 3) || (align > metrics.max_align))


Suggested change

if ((align > 3) || (align > metrics.max_align))

if (align > metrics.max_align)

I would still prefer the memory_width version of the code.

Why? To have nice looking table? It is mistake to keep in table data which you later need to modify by a constant expression.

gumb0 · 2020-06-25T18:24:45Z

lib/fizzy/instructions.hpp

+    /// 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.


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

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

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

It does contain invalid value for non-memory ops.

test/unittests/validation_test.cpp

Co-authored-by: Paweł Bylica <chfast@gmail.com>

axic commented May 11, 2020

View reviewed changes

lib/fizzy/parser_expr.cpp Outdated Show resolved Hide resolved

axic commented May 11, 2020

View reviewed changes

lib/fizzy/parser_expr.cpp Outdated Show resolved Hide resolved

axic commented May 11, 2020

View reviewed changes

lib/fizzy/parser_expr.cpp Outdated Show resolved Hide resolved

axic commented May 11, 2020

View reviewed changes

lib/fizzy/instructions.hpp Outdated Show resolved Hide resolved

axic force-pushed the validate-align branch 2 times, most recently from 1c9f923 to 9da335e Compare May 16, 2020 15:38

axic marked this pull request as ready for review May 21, 2020 13:59

chfast requested changes May 21, 2020

View reviewed changes

lib/fizzy/instructions.hpp Outdated Show resolved Hide resolved

axic force-pushed the validate-align branch 2 times, most recently from f30fa0c to 3e2d684 Compare May 25, 2020 22:16

axic force-pushed the validate-align branch from 3e2d684 to 7cc5620 Compare May 27, 2020 18:42

axic requested review from chfast and gumb0 May 27, 2020 18:42

chfast approved these changes May 27, 2020

View reviewed changes

lib/fizzy/instructions.hpp Outdated Show resolved Hide resolved

lib/fizzy/instructions.hpp Outdated Show resolved Hide resolved

gumb0 previously requested changes May 28, 2020

View reviewed changes

gumb0 reviewed May 28, 2020

View reviewed changes

lib/fizzy/parser_expr.cpp Outdated Show resolved Hide resolved

axic force-pushed the validate-align branch 2 times, most recently from 369e0f5 to 68494f7 Compare June 2, 2020 10:11

gumb0 reviewed Jun 2, 2020

View reviewed changes

lib/fizzy/parser_expr.cpp Outdated Show resolved Hide resolved

axic force-pushed the validate-align branch 3 times, most recently from 6b5c048 to 4a0edcb Compare June 2, 2020 17:28

axic requested review from gumb0 and chfast June 2, 2020 17:29

axic force-pushed the validate-align branch from 4a0edcb to 94564b7 Compare June 2, 2020 17:41

axic force-pushed the validate-align branch 3 times, most recently from 8afdf74 to ef93833 Compare June 25, 2020 13:00

axic requested review from gumb0 and chfast June 25, 2020 14:31

chfast reviewed Jun 25, 2020

View reviewed changes

axic force-pushed the validate-align branch 2 times, most recently from 36d58e1 to 7d10cc9 Compare June 25, 2020 15:34

chfast requested changes Jun 25, 2020

View reviewed changes

axic force-pushed the validate-align branch 2 times, most recently from 809617d to b9623c6 Compare June 25, 2020 16:01

gumb0 reviewed Jun 25, 2020

View reviewed changes

chfast approved these changes Jun 25, 2020

View reviewed changes

gumb0 reviewed Jun 25, 2020

View reviewed changes

test/unittests/validation_test.cpp Outdated Show resolved Hide resolved

gumb0 reviewed Jun 25, 2020

View reviewed changes

test/unittests/validation_test.cpp Outdated Show resolved Hide resolved

axic force-pushed the validate-align branch from b9623c6 to 10cd822 Compare June 25, 2020 23:31

axic requested a review from gumb0 June 25, 2020 23:32

gumb0 approved these changes Jun 26, 2020

View reviewed changes

test/unittests/validation_test.cpp Outdated Show resolved Hide resolved

test/unittests/validation_test.cpp Outdated Show resolved Hide resolved

gumb0 approved these changes Jun 26, 2020

View reviewed changes

chfast approved these changes Jun 26, 2020

View reviewed changes

axic force-pushed the validate-align branch from e247895 to 795fb07 Compare June 26, 2020 12:19

axic and others added 4 commits June 26, 2020 13:20

Add max_align to InstructionMetrics

e67c498

Co-authored-by: Paweł Bylica <chfast@gmail.com>

Validate i32/i64.load/store alignment

31987ed

ci: update expectations

963d729

test: add validation for invalid alignment

f8e939c

axic force-pushed the validate-align branch from 795fb07 to f8e939c Compare June 26, 2020 12:21

axic merged commit 508fb9e into master Jun 26, 2020

axic deleted the validate-align branch June 26, 2020 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate i32/i64.load/store alignment #310

Validate i32/i64.load/store alignment #310

axic commented May 11, 2020

axic May 11, 2020

axic May 21, 2020

codecov bot commented May 11, 2020 •

edited

Loading

gumb0 left a comment

chfast Jun 25, 2020

axic Jun 25, 2020

chfast Jun 25, 2020

axic Jun 25, 2020

chfast Jun 25, 2020

axic Jun 25, 2020

chfast Jun 25, 2020

chfast Jun 25, 2020

axic Jun 25, 2020

chfast Jun 25, 2020

gumb0 Jun 25, 2020

axic Jun 25, 2020

gumb0 Jun 25, 2020

axic Jun 25, 2020

	if ((align > 3) \|\| (align > metrics.max_align))
	if (align > metrics.max_align)

Validate i32/i64.load/store alignment #310

Validate i32/i64.load/store alignment #310

Conversation

axic commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 11, 2020 • edited Loading

Codecov Report

gumb0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 11, 2020 •

edited

Loading