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

fix 1005 panic sub overflow #1006

Merged
merged 6 commits into from
Nov 27, 2019
Merged

fix 1005 panic sub overflow #1006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5080040
check stack len before getting value
pventuzelo Nov 22, 2019
2261f8b
cargo fmt
pventuzelo Nov 22, 2019
cd0da74
add changelog entry
pventuzelo Nov 22, 2019
d376b89
Merge branch 'master' into ventuzelo/fix-1005-panic-sub-overflow
pventuzelo Nov 25, 2019
49665d5
use checked_sub for peekn_extra
pventuzelo Nov 26, 2019
bb16547
Merge branch 'ventuzelo/fix-1005-panic-sub-overflow' of github.com:pv…
pventuzelo Nov 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## **[Unreleased]**

- [#1006](https://github.com/wasmerio/wasmer/pull/1006) Fix minor panic issue when `wasmer::compile_with` called with llvm backend
- [#1009](https://github.com/wasmerio/wasmer/pull/1009) Enable LLVM verifier for all tests, add new llvm-backend-tests crate.

## 0.11.0 - 2019-11-22
Expand Down
12 changes: 6 additions & 6 deletions lib/llvm-backend/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,12 @@ impl State {
&self,
n: usize,
) -> Result<&[(BasicValueEnum, ExtraInfo)], BinaryReaderError> {
self.stack
.get(self.stack.len() - n..)
.ok_or(BinaryReaderError {
message: "invalid value stack",
offset: -1isize as usize,
})
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..])
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 ;)

}

pub fn popn_save_extra(
Expand Down