Validate local count in local.set/get/tee #363

axic · 2020-05-29T13:53:34Z

No description provided.

lib/fizzy/parser_expr.cpp

axic · 2020-05-29T13:55:11Z

lib/fizzy/parser.cpp

    uint64_t local_count = 0;
    for (const auto& l : locals_vec)
    {
        local_count += l.count;
        if (local_count > std::numeric_limits<uint32_t>::max())
            throw parser_error{"too many local variables"};
    }
+
+    auto [code, pos2] = parse_expr(pos1, end, static_cast<uint32_t>(local_count), func_idx, module);


First I tried to pass on locals_vec but realised it would need to do this counting again. We can refactor this when type checking is introduced, because then we'll need the types of locals.

axic · 2020-05-29T13:55:41Z

lib/fizzy/parser_expr.cpp

@@ -564,6 +566,13 @@ parser_result<Code> parse_expr(
        {
            uint32_t imm;
            std::tie(imm, pos) = leb128u_decode<uint32_t>(pos, end);
+
+            if (instr == Instr::local_get || instr == Instr::local_set)


Need to add local_tee here. However it seems that doesn't change the number of successful validation tests, which suggests they are not covered upstream.

lib/fizzy/parser_expr.cpp

codecov · 2020-06-02T22:03:48Z

Codecov Report

Merging #363 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master     #363    +/-   ##
========================================
  Coverage   99.32%   99.32%            
========================================
  Files          42       42            
  Lines       12692    12829   +137     
========================================
+ Hits        12606    12743   +137     
  Misses         86       86

lib/fizzy/parser_expr.cpp

lib/fizzy/parser.hpp

lib/fizzy/parser_expr.cpp

axic · 2020-06-03T09:34:12Z

lib/fizzy/parser_expr.cpp

@@ -148,6 +150,12 @@ parser_result<Code> parse_expr(
    // The function's implicit block.
    control_stack.emplace(Instr::block, function_arity, 0);

+    // This is needed for aiding validation.
+    assert((uint64_t{local_count} + module.typesec[func_type_idx].inputs.size()) <=


Probably should make this a proper parser error... or is this more like a missing case in the spec?!

chfast

Also validation tests are missing.

lib/fizzy/parser_expr.cpp

gumb0 · 2020-06-03T12:13:27Z

lib/fizzy/parser.hpp

-    const uint8_t* input, const uint8_t* end, FuncIdx func_idx, const Module& module);
+/// @param input       The beginning of the expr binary input.
+/// @param end         The end of the binary input.
+/// @param local_count The number of locals defined.


We are going to need types for type validation, maybe makes sense to pass const Locals& right away?

This was discussed in the an earlier comment. I started out with std::vector<Locals> but then local counting and validation of it has to be moved into parse_expr, which I think isn't the nicest.

I'd defer this work once we implement type checking.

Maybe create Code object in Module before parse_expr?

Maybe create Code object in Module before parse_expr?

I would advice against this, because it would make code confusingly both input and output parameter

It does not mean that parse_expr should return Code. Maybe it should return Expr. But is also bad to ask parse_expr to move the information about locals (here local_count, maybe type information later) from one place to another. That's unneeded and trivial responsibility.

lib/fizzy/parser.hpp

lib/fizzy/parser_expr.cpp

gumb0 · 2020-06-03T14:12:56Z

lib/fizzy/parser_expr.cpp

+    // This is needed for aiding validation.
+    assert((uint64_t{local_count} + module.typesec[func_type_idx].inputs.size()) <=
+           std::numeric_limits<uint32_t>::max());
+    const uint32_t max_local_index =


Well it's not max index, maybe total_local_arg_count or local_and_arg_count

Why isn't it max index? The immediate it is compared against is called "local index".

Because it's max index + 1

gumb0 · 2020-06-08T16:12:12Z

Looks ok, needs a test fro validation failure.

axic · 2020-06-08T17:34:11Z

@gumb0 added tests.

axic · 2020-06-08T17:37:58Z

lib/fizzy/parser_expr.cpp

    // The function's implicit block.
    control_stack.emplace(Instr::block, function_arity, 0);

+    // This is needed for aiding validation.
+    assert(
+        (uint64_t{local_count} + func_type.inputs.size()) <= std::numeric_limits<uint32_t>::max());


I think this case is also missing from the spectests.

… 'callee_'

axic commented May 29, 2020

View reviewed changes

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

axic commented May 29, 2020

View reviewed changes

axic force-pushed the validate-locals branch 3 times, most recently from 2320ef5 to 4d72375 Compare June 2, 2020 21:57

axic commented Jun 2, 2020

View reviewed changes

lib/fizzy/parser_expr.cpp Show resolved Hide resolved

axic commented Jun 2, 2020

View reviewed changes

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

axic commented Jun 2, 2020

View reviewed changes

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

axic requested review from chfast and gumb0 and removed request for chfast June 2, 2020 22:06

axic marked this pull request as ready for review June 2, 2020 22:08

chfast reviewed Jun 3, 2020

View reviewed changes

axic force-pushed the validate-locals branch 4 times, most recently from 327178a to 62a933e Compare June 3, 2020 09:31

axic requested a review from chfast June 3, 2020 09:32

axic commented Jun 3, 2020

View reviewed changes

axic mentioned this pull request Jun 3, 2020

refactor: use proper variable names instead of generic imm in parser_expr #369

Merged

chfast requested changes Jun 3, 2020

View reviewed changes

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

axic force-pushed the validate-locals branch 2 times, most recently from d04fa37 to cc6693e Compare June 3, 2020 11:26

gumb0 reviewed Jun 3, 2020

View reviewed changes

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

gumb0 reviewed Jun 3, 2020

View reviewed changes

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

gumb0 reviewed Jun 3, 2020

View reviewed changes

axic force-pushed the validate-locals branch 2 times, most recently from b79d3dd to 0488136 Compare June 8, 2020 09:05

axic force-pushed the validate-locals branch 2 times, most recently from 7ae75ca to ddf45be Compare June 8, 2020 17:20

axic commented Jun 8, 2020

View reviewed changes

gumb0 approved these changes Jun 8, 2020

View reviewed changes

chfast approved these changes Jun 8, 2020

View reviewed changes

gumb0 force-pushed the validate-locals branch from ddf45be to 59fdbd0 Compare June 22, 2020 12:21

axic force-pushed the validate-locals branch 2 times, most recently from 99a24b8 to 2813c84 Compare June 24, 2020 16:58

axic added 7 commits June 24, 2020 17:59

Count locals prior to parsing expressions

e06d127

refactor: retrieve func_type in parse_expr and prefix other uses with…

bee3d7d

… 'callee_'

Pass local_count to parse_expr() and craft the Code fully there

b9ec848

Validate local indexes in parser

a0ce2cf

ci: update spectests expectations

ba5fe95

test: fix parser test to contain proper local count

6cfc610

test: add validation tests for local_get/set/tee

88dbbcd

axic force-pushed the validate-locals branch from 2813c84 to 88dbbcd Compare June 24, 2020 16:59

gumb0 approved these changes Jun 24, 2020

View reviewed changes

axic merged commit e1625a7 into master Jun 24, 2020

axic deleted the validate-locals branch June 24, 2020 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate local count in local.set/get/tee #363

Validate local count in local.set/get/tee #363

axic commented May 29, 2020

axic May 29, 2020

axic May 29, 2020

codecov bot commented Jun 2, 2020 •

edited

Loading

axic Jun 3, 2020 •

edited

Loading

chfast left a comment

gumb0 Jun 3, 2020

axic Jun 3, 2020

chfast Jun 8, 2020

gumb0 Jun 8, 2020

chfast Jun 8, 2020

gumb0 Jun 3, 2020

axic Jun 7, 2020

gumb0 Jun 8, 2020

gumb0 commented Jun 8, 2020

axic commented Jun 8, 2020

axic Jun 8, 2020

Validate local count in local.set/get/tee #363

Validate local count in local.set/get/tee #363

Conversation

axic commented May 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 2, 2020 • edited Loading

Codecov Report

axic Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

chfast 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

gumb0 commented Jun 8, 2020

axic commented Jun 8, 2020

Choose a reason for hiding this comment

codecov bot commented Jun 2, 2020 •

edited

Loading

axic Jun 3, 2020 •

edited

Loading