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 1005 panic sub overflow #1006

Merged

Conversation

pventuzelo
Copy link
Contributor

@pventuzelo pventuzelo commented Nov 22, 2019

Description

Fix issue #1005

Review

  • Add a short description of the the change to the CHANGELOG.md file

Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good to me, one small point of feedback though

}

let new_len = self.stack.len() - n;
Ok(&self.stack[new_len..])
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor style feedback:

⛳️

let new_len = self.stack.len().checked_sub(n).ok_or( /* error code goes here */ );
// do other stuff here

I think this is a bit more explicit and concise!

This can be combined into a singe statement, but either style is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you, it would has been more clear in your way but the code will more complicated at the end because either:

  • wasmparser::primitives:BinaryReaderError doesn't implement convert::From
  • warning: unreachable expression
  • unused varaible will be detected by the compiler somehow so we will need to rename new_len to _new_len.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, sorry I'm not following, I'll write out fully what I meant:

I think the entire function can just be:

pub fn peekn_extra(&self, n: usize) -> Result<&[(BasicValueEnum, ExtraInfo)], BinaryReaderError> {
   let new_len = self.stack.len().checked_sub(n)
            .ok_or(BinaryReaderError {
                message: "invalid value stack",
                offset: -1isize as usize,
            });
   Ok(&self.stack[new_len..])
}

Let me know if that doesn't make sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've written the same and got some error but my bad i forgot to propagate the Err ;)

@MarkMcCaskey MarkMcCaskey self-assigned this Nov 25, 2019
Copy link
Contributor

@MarkMcCaskey MarkMcCaskey left a comment

Choose a reason for hiding this comment

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

awesome, thanks!

@MarkMcCaskey
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Nov 27, 2019
1006: fix 1005 panic sub overflow r=MarkMcCaskey a=pventuzelo

# Description

Fix issue #1005

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Patrick Ventuzelo <ventuzelo.patrick@gmail.com>
Co-authored-by: Patrick Ventuzelo <9038181+pventuzelo@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Nov 27, 2019

Build succeeded

@bors bors bot merged commit bb16547 into wasmerio:master Nov 27, 2019
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