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

[test] Add a test for non-treelike behavior of stack #961

Merged
merged 1 commit into from
May 4, 2020
Merged

[test] Add a test for non-treelike behavior of stack #961

merged 1 commit into from
May 4, 2020

Conversation

alexcrichton
Copy link
Contributor

We've recently found a bug in a WebAssembly library we've been working
with where we're mapping WebAssembly to a tree-like IR internally. The
way we parse into this representation, however, has a bug when the
function isn't itself tree-like but rather exibits properties that
exploit a stack machine. For example this isn't so straightforward to
represent in a tree-like fashion:

(import "" "a" (func $foo))
(import "" "b" (func $foo (result i32)))
(func (result i32)
  call $b
  call $b
  call $a
  i32.xor)

The extra call $a in the middle is valid WebAssembly but needs
special treatment when converting to a more tree-like IR format. I
figured it'd be good to ensure there's a spec test covering this case as
we currently pass the suite of spec tests but still contain this bug!

binji
binji reviewed Feb 1, 2019
View reviewed changes
test/core/call.wast Outdated Show resolved Hide resolved
test/core/call.wast Outdated
call $add_one_to_global
call $add_one_to_global
call $add_one_to_global_and_drop
i32.add
Copy link
Member

Choose a reason for hiding this comment

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

Can this be simplified to:

i32.const 0
i32.const 0
nop
i32.add

To exhibit the same behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idea here is to try and have something that has visible side effects and can't be trivially simplified to a tree like (i32.add (...) (...)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah the bug with our implementation at least was to reorder this accidentally to:

nop
i32.const 0
i32.const 0
i32.add

so I figured it'd be good to assert that the runtime behavior is as expected as well. I'm not sure how to best do that though and there may be an easier way to do so that's less convoluted!

@alexcrichton
Add a test for non-treelike behavior of stack
61170ba 
We've recently found a bug in a WebAssembly library we've been working
with where we're mapping WebAssembly to a tree-like IR internally. The
way we parse into this representation, however, has a bug when the
function isn't itself tree-like but rather exibits properties that
exploit a stack machine. For example this isn't so straightforward to
represent in a tree-like fashion:

    (import "" "a" (func $foo))
    (import "" "b" (func $foo (result i32)))
    (func (result i32)
      call $b
      call $b
      call $a
      i32.xor)

The extra `call $a` in the middle is valid `WebAssembly` but needs
special treatment when converting to a more tree-like IR format. I
figured it'd be good to ensure there's a spec test covering this case as
we currently pass the suite of spec tests but still contain this bug!
@rossberg
Copy link
Member

rossberg commented Feb 5, 2019

There are many tests like that for nop (see nop.wast). If we think it's worth having similar tests for call then perhaps it's worth being a bit more systematic than just a single test?

And what about other instructions with side effects?

@alexcrichton
Copy link
Contributor Author

Oh dear sorry for posting .... over a year later, this totally slipped my mind and I just got back to this from seeing this commit elsewhere. Sorry about that!

@rossberg FWIW this test is intended to exercise the side effects of executing instructions in order vs a test specifically for the call instruction. In that sense it's more just trying to guarantee that the side effects of various instructions are preserve and aren't reordered by accident.

I certainly don't mind adding more tests, though, if you feel that we should add more for specific instructions!

@rossberg rossberg changed the title Add a test for non-treelike behavior of stack [test] Add a test for non-treelike behavior of stack May 4, 2020
@rossberg rossberg merged commit 65a37e1 into WebAssembly:master May 4, 2020
gumb0 pushed a commit to wasmx/wasm-spec that referenced this pull request Sep 18, 2020
@alexcrichton @gumb0
Add a test for non-treelike behavior of stack (WebAssembly#961)
7475815 
We've recently found a bug in a WebAssembly library we've been working
with where we're mapping WebAssembly to a tree-like IR internally. The
way we parse into this representation, however, has a bug when the
function isn't itself tree-like but rather exibits properties that
exploit a stack machine. For example this isn't so straightforward to
represent in a tree-like fashion:

    (import "" "a" (func $foo))
    (import "" "b" (func $foo (result i32)))
    (func (result i32)
      call $b
      call $b
      call $a
      i32.xor)

The extra `call $a` in the middle is valid `WebAssembly` but needs
special treatment when converting to a more tree-like IR format. I
figured it'd be good to ensure there's a spec test covering this case as
we currently pass the suite of spec tests but still contain this bug!
gumb0 pushed a commit to wasmx/wasm-spec that referenced this pull request Sep 21, 2020
@alexcrichton @gumb0
Add a test for non-treelike behavior of stack (WebAssembly#961)
c3335bc 
We've recently found a bug in a WebAssembly library we've been working
with where we're mapping WebAssembly to a tree-like IR internally. The
way we parse into this representation, however, has a bug when the
function isn't itself tree-like but rather exibits properties that
exploit a stack machine. For example this isn't so straightforward to
represent in a tree-like fashion:

    (import "" "a" (func $foo))
    (import "" "b" (func $foo (result i32)))
    (func (result i32)
      call $b
      call $b
      call $a
      i32.xor)

The extra `call $a` in the middle is valid `WebAssembly` but needs
special treatment when converting to a more tree-like IR format. I
figured it'd be good to ensure there's a spec test covering this case as
we currently pass the suite of spec tests but still contain this bug!
rossberg added a commit that referenced this pull request Feb 11, 2021
@rossberg @ngzhian
@kripken @binji @Ms2ger @alexcrichton
Merge upstream (#89)
5567def 
* Upgrade to latest Sphinx release (2.4.4) (#1171)

Fixes #1157

* Support 4GB of memory both in initial and max.

* [interpreter] Strictify and specify .bin.wast format (#1173)

* Merge nontrapping-float-to-int proposal into spec (#1143)

See the non-trapping-float-to-int-conversions proposal here:

https://github.com/WebAssembly/nontrapping-float-to-int-conversions

* Merge sign-extension-ops proposal into spec (#1144)

See the sign-extension-ops proposal here:

https://github.com/WebAssembly/sign-extension-ops

This PR is built on top of #1143 (merge nontrapping-float-to-int).

* Merge multi-value proposal into spec (#1145)

See the multi-value proposal here:

https://github.com/WebAssembly/multi-value

This PR is built on top of the following PRs:

* #1143 (merge nontrapping-float-to-int)
* #1144 (merge sign-extension-ops)

* [interpreter] Remove junk in README

* [interpreter] Remove junk in README

* [spec] Fix grammar for fractions (#1178)

* [spec] Add missing i64.extend32_s syntax (#1179)

* [js-api][web-api] Fix some markup errors.

* Add a README to the proposals directory.

* Add more address overflow tests (#1188)

There are already tests for effective address overflow, but those have a
large value baked into the offset. These tests all use `1` as the
immediate offset, and use `-1` for the address on the stack, which may
be compiled differently.

* Add a test for non-treelike behavior of stack (#961)

We've recently found a bug in a WebAssembly library we've been working
with where we're mapping WebAssembly to a tree-like IR internally. The
way we parse into this representation, however, has a bug when the
function isn't itself tree-like but rather exibits properties that
exploit a stack machine. For example this isn't so straightforward to
represent in a tree-like fashion:

    (import "" "a" (func $foo))
    (import "" "b" (func $foo (result i32)))
    (func (result i32)
      call $b
      call $b
      call $a
      i32.xor)

The extra `call $a` in the middle is valid `WebAssembly` but needs
special treatment when converting to a more tree-like IR format. I
figured it'd be good to ensure there's a spec test covering this case as
we currently pass the suite of spec tests but still contain this bug!

* [js-api] Various editorial improvements.

* [js-api] Replace pseudo-ASCII characters by normal ones.

This also required disambiguating the references to "module", as there are now
two definitions by that name.

* [js-api] Improve prose in 'run a host function'.

* [js-api] Improve some of the multi-value prose.

* Synchronize js-api tests.

* Add script to synchronize js-api tests.

Co-authored-by: Ng Zhi An <ngzhian@gmail.com>
Co-authored-by: Alon Zakai <azakai@google.com>
Co-authored-by: Ben Smith <binji@chromium.org>
Co-authored-by: Ms2ger <Ms2ger@igalia.com>
Co-authored-by: Alex Crichton <alex@alexcrichton.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@binji binji binji left review comments

@fitzgen fitzgen fitzgen left review comments

@rossberg rossberg rossberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alexcrichton @rossberg @fitzgen @binji