Fix issue with emscripten memory out of range #698
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bjfish
commented
Aug 19, 2019
| Some(( | ||
| align_memory(*dynamic_base as u32 - 32), | ||
| align_memory(*dynamictop_ptr as u32 - 32), | ||
| )) |
There was a problem hiding this comment.
In development profile, there was a subtraction underflow before the index out of bounds error.
MarkMcCaskey
approved these changes
Aug 19, 2019
There was a problem hiding this comment.
Looks good from a quick look over;
General comment: I think we should move away from memory.view::<u32>()[(dynamictop_ptr / 4) as usize] in favor of the WasmPtr abstraction. WasmPtr handles a lot of the complexity and avoids us having to do division and manual alignment checks.
|
bors r+ |
| let dynamictop_ptr = globals.dynamictop_ptr; | ||
| let dynamic_base = globals.dynamic_base; | ||
|
|
||
| if (dynamictop_ptr / 4) as usize >= memory.view::<u32>().len() { | ||
| return Err("dynamictop_ptr beyond memory len".to_string()); | ||
| } | ||
| memory.view::<u32>()[(dynamictop_ptr / 4) as usize].set(dynamic_base); |
There was a problem hiding this comment.
I'm very tired, so I kind of missed this, but the bounds check here is something WasmPtr would have done for us and this is kind of new legacy code that we should probably just do right... I suppose I can replace it once this lands on master though
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #678