Create CommunicationError::DerefErr to avoid panics #418
Conversation
| DerefErr { | ||
| /// the position in a Wasm linear memory | ||
| offset: u32, | ||
| msg: String, |
There was a problem hiding this comment.
Do we really need this message field?
All the messages written here are basically the same, and basicall say the same thing as the name of the variant plus the offset field.
There was a problem hiding this comment.
Yeah, we need it for a lot of important context information in read_region/write_region as described in https://github.com/CosmWasm/cosmwasm/pull/418/files#r438383917. Those two need different context information than the Region deref.
There was a problem hiding this comment.
I think i see your point. You want to make sure that the pointers are dereferencable in different contexts, and you want to be able to later debug what exactly went wrong
There was a problem hiding this comment.
In this case it is not me debugging this but it is other people who build standard libraries for contract development in other languages that need to get Region handling right. Once this job is done, you hardly see this error again.
packages/vm/src/memory.rs
Outdated
| @@ -83,11 +83,11 @@ pub fn read_region(ctx: &Ctx, ptr: u32, max_length: usize) -> VmResult<Vec<u8>> | |||
| } | |||
| Ok(result) | |||
| } | |||
| None => panic!( | |||
| None => Err(CommunicationError::deref_err(region.offset, format!( | |||
| "Error dereferencing region {:?} in wasm memory of size {}. This typically happens when the given pointer does not point to a Region struct.", | |||
There was a problem hiding this comment.
Do we need all this text? It will be embedded inside:
The Wasm memory address {} provided by the contract could not be dereferenced: {}
We can simplify the wording here. (But I like returning this types error over a panic)
There was a problem hiding this comment.
I can try compress the text slightly, but this text and the debug info are is very helpful. When you read a Region it almost always succeeds, since you can read any 12 bytes into a Region, even if you got completely broken data.
Now you try to read the region and get some error. Most likely the deref that returns the error is not the problem but the Region you read before was garbage. E.g. when you should read a 12 MB Region but the Wasm memory is only 1 MB long. Or when you are supposed to read a Region with length > capacity. This is why I add so context here and the helper text. Even a stacktrace is probably way less helpful.
"Error dereferencing region" however is wrong since the Region was dereferenced before. It must be "Tried to access memory of region {:?} in wasm memory of size {}. This typically happens when the given Region pointer does not point to a valid Region struct."
packages/vm/src/memory.rs
Outdated
| Ok(()) | ||
| }, | ||
| None => panic!( | ||
| None => Err(CommunicationError::deref_err(region.offset, format!( | ||
| "Error dereferencing region {:?} in wasm memory of size {}. This typically happens when the given pointer does not point to a Region struct.", |
webmaster128
force-pushed
the
create-deref-error
branch
from
d03e152
to
9ec07b6
Compare
Closes #416