-
Notifications
You must be signed in to change notification settings - Fork 821
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
Handle overflow errors in Bytes -> Pages conversion #1914
Conversation
afc879a
to
fd9e851
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the PR! Apart from the little comment, everything looks good to me!
lib/wasmer-types/src/units.rs
Outdated
impl From<Bytes> for Pages { | ||
fn from(bytes: Bytes) -> Self { | ||
Self((bytes.0 / WASM_PAGE_SIZE) as u32) | ||
const PAGES_EXCEED_UINT32: &'static str = "Number of pages exceeds uint32 range"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need a constant here. The constant is used only once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can inline it, no problem. Do you prefer that?
lib/wasmer-types/src/units.rs
Outdated
.try_into() | ||
.or(Err(PAGES_EXCEED_UINT32))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can write:
.try_into() | |
.or(Err(PAGES_EXCEED_UINT32))?; | |
.try_into() | |
.map_err(PAGES_EXCEED_UINT32)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map_err
takes a callback, which is what I wanted to avoid. It would have to be
.map_err(|_original_error| PAGES_EXCEED_UINT32)?;
However, we can use this as well. Which way do you like more?
fd9e851
to
59fb850
Compare
Thanks for the positive feedback @Hywan. After looking at the error again I thought we better do this properly and use an error type instead of a generic string. This shows the user's type system that there there is only one possible error reason that in many use cases can be handled by defauling to |
bors r+ |
Description
Before the Bytes to Pages conversion silently returned wrong results when the number of pages was too large.
Review