Skip to content

[spec] Parsing and validation in a single pass seems to be discouraged #1170

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.

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

Closed
axic opened this issue Mar 31, 2020 · 3 comments
Closed

[spec] Parsing and validation in a single pass seems to be discouraged #1170

axic opened this issue Mar 31, 2020 · 3 comments

Comments

@axic
Copy link
Contributor

axic commented Mar 31, 2020

This test case in binary.wast (https://github.com/WebAssembly/spec/blob/v1.1/test/core/binary.wast#L739-L759) tests for assert_malformed, e.g. parser error. However it also contains a validation error.

This turns out to pose a difficulty to engines, which would try to do parsing and (or a part of) validation in a single pass.

Is this intentional?

@rossberg
Copy link
Member

rossberg commented Mar 31, 2020

No, every negative test should contain only one mistake. But it's easy to miss cases where that's not the case. Happy to accept fixes. :)

@chfast
Copy link
Contributor

chfast commented Apr 16, 2020

Problem

The problematic test checks br_table "with inconsistent target count (2 declared, 1 given)". But the intention is quite distant from how binary parser works. I.e. it assumes the parser fails because there is not enough targets provided

0e02 00 (missing LEB128 encoded target)

but the parser will just consume the next byte being the end 0b.

0e02 00 0b

and will fail because of the missing final 0b later. Even the test error message suggests that.

The problem is that in the middle we have br_table with 2 targets: 00 and 0b, where 0b one is invalid. The module is considered invalid before we are able to realize it is malformed.

Solution

  1. We can just drop the test.
  2. Or to preserve more/less the current cause of being malformed we can add some workaround/hacks preventing the additionally consumed byte seen as invalid br_table target. The only idea I have to achieve that is to make 0b target valid by adding 10 more functions to the module.

@binji
Copy link
Member

binji commented Apr 16, 2020

The only idea I have to achieve that is to make 0b target valid by adding 10 more functions to the module.

Right, though you would need to add blocks, not functions. I think it should work, as long as you made the br_table target length long enough too, so it attempts to access outside of the code section.

binji added a commit that referenced this issue Jun 9, 2020
This test doesn't really test what it says: the br_table is consumed
correctly, and instead the `end` instruction is missing. This has caused
various issues in decoders (see issue #1170) as well as requiring
changing the error message in future proposals (e.g. bulk memory).
binji added a commit that referenced this issue Jun 9, 2020
This test doesn't really test what it says: the br_table is consumed
correctly, and instead the `end` instruction is missing. This has caused
various issues in decoders (see issue #1170) as well as requiring
changing the error message in future proposals (e.g. bulk memory).
@binji binji closed this as completed Jun 9, 2020
gumb0 pushed a commit to wasmx/wasm-spec that referenced this issue Sep 18, 2020
This test doesn't really test what it says: the br_table is consumed
correctly, and instead the `end` instruction is missing. This has caused
various issues in decoders (see issue WebAssembly#1170) as well as requiring
changing the error message in future proposals (e.g. bulk memory).
gumb0 pushed a commit to wasmx/wasm-spec that referenced this issue Sep 21, 2020
This test doesn't really test what it says: the br_table is consumed
correctly, and instead the `end` instruction is missing. This has caused
various issues in decoders (see issue WebAssembly#1170) as well as requiring
changing the error message in future proposals (e.g. bulk memory).
chfast added a commit to wasmx/wasm-spec that referenced this issue Sep 29, 2020
Remove the code section in tests for malformed element section.
Otherwise the code section id (0x0a) is taken as an element's table
index what is a validation error.

This is similar to the previously reported issue:
WebAssembly#1170.
chfast added a commit to wasmx/wasm-spec that referenced this issue Nov 6, 2020
Remove the code section in tests for malformed element section.
Otherwise the code section id (0x0a) is taken as an element's table
index what is a validation error.

This is similar to the previously reported issue:
WebAssembly#1170.
rossberg pushed a commit that referenced this issue Nov 10, 2020
Remove the code section in tests for malformed element section.
Otherwise the code section id (0x0a) is taken as an element's table
index what is a validation error.

This is similar to the previously reported issue:
#1170.
chfast added a commit to wasmx/wasm-spec that referenced this issue Nov 10, 2020
Remove the code section in tests for malformed element section.
Otherwise the code section id (0x0a) is taken as an element's table
index what is a validation error.

This is similar to the previously reported issue:
WebAssembly#1170.
dtig pushed a commit to WebAssembly/simd that referenced this issue Jan 11, 2021
* [spec] automate instruction index rebuild (#1259)

* [test] Add test for malformed functype (#1254)

* [test] Correct tests for missing elements (#1251)

Remove the code section in tests for malformed element section.
Otherwise the code section id (0x0a) is taken as an element's table
index what is a validation error.

This is similar to the previously reported issue:
WebAssembly/spec#1170.

* [test] Add tests for data segment with memidx 1 (#1249)

* [test] Correct i32.store alignment in a LEB128 test (#1261)

In the binary-leb128.wast, change the alignment of an i32.store
instruction from 3 (invalid) to 2 (the intention suggested by the
comment).

Co-authored-by: Andreas Rossberg <[email protected]>
Co-authored-by: Paweł Bylica <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants