Skip to content

Fix the moves fuzz target.#97

Merged
cfallin merged 2 commits intobytecodealliance:mainfrom
cfallin:fix-move-fuzz-scratch
Oct 21, 2022
Merged

Fix the moves fuzz target.#97
cfallin merged 2 commits intobytecodealliance:mainfrom
cfallin:fix-move-fuzz-scratch

Conversation

@cfallin
Copy link
Copy Markdown
Member

@cfallin cfallin commented Oct 21, 2022

Two fixes uncovered by a warning noted in #96:

  • The iteration that was meant to be adding between 0 and 2 scratch registers was adding zero or one instead, because it was written as for i in u.int_in_range(0..=2) rather than for i in 0..u.int_in_range(0..=2)?. This is somewhat subtle but the Result is iterable so it was choosing zero if u's input ended (making it unable to provide an arbitrary int) or one otherwise. This still would give some interesting fuzz coverage but was clearly not the original code's intent.

  • Separately, it seems this fuzz target hadn't been run comprehensively in a while (we just run the toplevel ion_checker target on OSS-Fuzz, which checks the whole allocator; this target is meant to be a more focused way of testing the move resolver when hacking on it, and probably isn't worth the OSS-Fuzz time otherwise). It pretty quickly hit a panic in the fuzz target itself on a stack-to-stack move, because earlier it was written not to generate these; but now it does, and we handle them successfully in the move resolver, so there's no reason to panic about a bad testcase.

@cfallin cfallin requested a review from elliottt October 21, 2022 15:41
Copy link
Copy Markdown
Member

@elliottt elliottt left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for the write-up!

@cfallin cfallin merged commit b4eedf3 into bytecodealliance:main Oct 21, 2022
@cfallin cfallin deleted the fix-move-fuzz-scratch branch October 21, 2022 16:08
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.

2 participants