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.

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

Fix a panic in table-ops translation #2350

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

alexcrichton
Copy link
Member

This fixes an issue where ensure_inserted_block() wasn't called before
we do some block manipulation in the Wasmtime translation of some
table-related instructions. It looks like ensure_inserted_block() is
otherwise called on most instructions being added, so we just need to
call it explicitly it seems here.

Closes #2347

@alexcrichton
Copy link
Member Author

I'm a bit curious why the fuzzers haven't found this one yet, @fitzgen do you know if wasm-smith will create an if else end empty block like that?

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

wasm-smith definitely should be able to generate if; else; end sequences. It might be somewhat low probability, since all of the following need to happen:

  • We need to choose to generate a stack neutral if control frame (1 out of 400 chance to generate a stack neutral type; not sure on probability to generate any if in the program at all)
  • We need to immediately choose an else instruction (1 out of ~250 or so; not sure how many instructions we support these days)
  • We need to then immediately choose and end instruction (1 out of ~250 again)

so that's a 1 out of ~25,000,000 chance, ignoring running up against the entropy limit. Which seems like reasonable odds to hit fairly quickly, given that our fuzzers are running continuously...

So in conclusion: I'm not sure what's up, and I would have expected this to have been triggered earlier.

@fitzgen
Copy link
Member

fitzgen commented Nov 2, 2020

(Updated comment due to accidental early submit)

@fitzgen
Copy link
Member

fitzgen commented Nov 2, 2020

I think you need to split the new test file into one file for each module, which is why the tests are failing I think.

This fixes an issue where `ensure_inserted_block()` wasn't called before
we do some block manipulation in the Wasmtime translation of some
table-related instructions. It looks like `ensure_inserted_block()` is
otherwise called on most instructions being added, so we just need to
call it explicitly it seems here.

Closes bytecodealliance#2347
@alexcrichton
Copy link
Member Author

Bah I needed to touch build.rs to actually run the tests locally, I got the order of operands to table.set wrong by accident.

@alexcrichton alexcrichton merged commit 372ae2a into bytecodealliance:main Nov 2, 2020
@alexcrichton alexcrichton deleted the fix-panic branch November 2, 2020 23:53
cfallin pushed a commit that referenced this pull request Nov 30, 2020
This fixes an issue where `ensure_inserted_block()` wasn't called before
we do some block manipulation in the Wasmtime translation of some
table-related instructions. It looks like `ensure_inserted_block()` is
otherwise called on most instructions being added, so we just need to
call it explicitly it seems here.

Closes #2347
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

Successfully merging this pull request may close these issues.

Cranelift: Specifically formatted valid WASM panics with 'block Insertion point not in the layout'
2 participants