From 910b0539a7c96bfe6b6dfac5748ae79cc63dda18 Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Wed, 10 Jun 2020 13:54:41 +0200 Subject: [PATCH 1/6] Create CommunicationError::DerefErr --- packages/vm/src/errors.rs | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/vm/src/errors.rs b/packages/vm/src/errors.rs index 90aeddd459..3da07dcb89 100644 --- a/packages/vm/src/errors.rs +++ b/packages/vm/src/errors.rs @@ -239,12 +239,24 @@ pub type VmResult = core::result::Result; pub enum CommunicationError { #[snafu(display("Got a zero Wasm address"))] ZeroAddress { backtrace: snafu::Backtrace }, + #[snafu(display( + "A Wasm memory address provided by the contract could not be dereferenced: {}", + msg + ))] + DerefErr { + msg: String, + backtrace: snafu::Backtrace, + }, } impl CommunicationError { pub fn zero_address() -> Self { ZeroAddress {}.build() } + + pub fn deref_err>(msg: S) -> Self { + DerefErr { msg: msg.into() }.build() + } } impl From for VmError { @@ -494,7 +506,6 @@ mod test { // CommunicationError constructors - #[allow(unreachable_patterns)] // since CommunicationError is non_exhaustive, this should not create a warning. But it does :( #[test] fn communication_error_zero_address() { let error = CommunicationError::zero_address(); @@ -504,6 +515,15 @@ mod test { } } + #[test] + fn communication_error_deref_err() { + let error = CommunicationError::deref_err("broken stuff"); + match error { + CommunicationError::DerefErr { msg, .. } => assert_eq!(msg, "broken stuff"), + e => panic!("Unexpected error: {:?}", e), + } + } + // FfiError constructors #[test] From b432c50bb3209ee042b2fdb99686c42402465c7a Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Wed, 10 Jun 2020 14:01:38 +0200 Subject: [PATCH 2/6] Propagate deref errors from get_region and set_region --- packages/vm/src/errors.rs | 2 ++ packages/vm/src/memory.rs | 32 ++++++++++++++++++++++---------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/packages/vm/src/errors.rs b/packages/vm/src/errors.rs index 3da07dcb89..10cbfa6fc2 100644 --- a/packages/vm/src/errors.rs +++ b/packages/vm/src/errors.rs @@ -259,6 +259,8 @@ impl CommunicationError { } } +pub type CommunicationResult = core::result::Result; + impl From for VmError { fn from(communication_error: CommunicationError) -> Self { VmError::CommunicationErr { diff --git a/packages/vm/src/memory.rs b/packages/vm/src/memory.rs index 78119dca4e..5453a2afb6 100644 --- a/packages/vm/src/memory.rs +++ b/packages/vm/src/memory.rs @@ -5,7 +5,7 @@ use wasmer_runtime_core::{ }; use crate::conversion::to_u32; -use crate::errors::{VmError, VmResult}; +use crate::errors::{CommunicationError, CommunicationResult, VmError, VmResult}; /****** read/write to wasm memory buffer ****/ @@ -62,7 +62,7 @@ pub fn get_memory_info(ctx: &Ctx) -> MemoryInfo { /// memory region, which is copied in the second step. /// Errors if the length of the region exceeds `max_length`. pub fn read_region(ctx: &Ctx, ptr: u32, max_length: usize) -> VmResult> { - let region = get_region(ctx, ptr); + let region = get_region(ctx, ptr)?; if region.length > to_u32(max_length)? { return Err(VmError::region_length_too_big( @@ -106,7 +106,7 @@ pub fn maybe_read_region(ctx: &Ctx, ptr: u32, max_length: usize) -> VmResult VmResult<()> { - let mut region = get_region(ctx, ptr); + let mut region = get_region(ctx, ptr)?; let region_capacity = region.capacity as usize; if data.len() > region_capacity { @@ -122,7 +122,7 @@ pub fn write_region(ctx: &Ctx, ptr: u32, data: &[u8]) -> VmResult<()> { cells[i].set(data[i]) } region.length = data.len() as u32; - set_region(ctx, ptr, region); + set_region(ctx, ptr, region)?; Ok(()) }, None => panic!( @@ -134,17 +134,29 @@ pub fn write_region(ctx: &Ctx, ptr: u32, data: &[u8]) -> VmResult<()> { } /// Reads in a Region at ptr in wasm memory and returns a copy of it -fn get_region(ctx: &Ctx, ptr: u32) -> Region { +fn get_region(ctx: &Ctx, ptr: u32) -> CommunicationResult { let memory = ctx.memory(0); let wptr = WasmPtr::::new(ptr); - let cell = wptr.deref(memory).unwrap(); - cell.get() + match wptr.deref(memory) { + Some(cell) => Ok(cell.get()), + None => Err(CommunicationError::deref_err( + "Could not dereference this pointer to a Region", + )), + } } /// Overrides a Region at ptr in wasm memory with data -fn set_region(ctx: &Ctx, ptr: u32, data: Region) { +fn set_region(ctx: &Ctx, ptr: u32, data: Region) -> CommunicationResult<()> { let memory = ctx.memory(0); let wptr = WasmPtr::::new(ptr); - let cell = wptr.deref(memory).unwrap(); - cell.set(data); + + match wptr.deref(memory) { + Some(cell) => { + cell.set(data); + Ok(()) + } + None => Err(CommunicationError::deref_err( + "Could not dereference this pointer to a Region", + )), + } } From b2b426dfad8415723f1b057974b89067676922db Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Wed, 10 Jun 2020 14:06:16 +0200 Subject: [PATCH 3/6] Add offset to CommunicationError::DerefErr --- packages/vm/src/errors.rs | 20 +++++++++++++++----- packages/vm/src/memory.rs | 2 ++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/packages/vm/src/errors.rs b/packages/vm/src/errors.rs index 10cbfa6fc2..ffc0e2a957 100644 --- a/packages/vm/src/errors.rs +++ b/packages/vm/src/errors.rs @@ -240,10 +240,13 @@ pub enum CommunicationError { #[snafu(display("Got a zero Wasm address"))] ZeroAddress { backtrace: snafu::Backtrace }, #[snafu(display( - "A Wasm memory address provided by the contract could not be dereferenced: {}", + "The Wasm memory address {} provided by the contract could not be dereferenced: {}", + offset, msg ))] DerefErr { + /// the position in a Wasm linear memory + offset: u32, msg: String, backtrace: snafu::Backtrace, }, @@ -254,8 +257,12 @@ impl CommunicationError { ZeroAddress {}.build() } - pub fn deref_err>(msg: S) -> Self { - DerefErr { msg: msg.into() }.build() + pub fn deref_err>(offset: u32, msg: S) -> Self { + DerefErr { + offset, + msg: msg.into(), + } + .build() } } @@ -519,9 +526,12 @@ mod test { #[test] fn communication_error_deref_err() { - let error = CommunicationError::deref_err("broken stuff"); + let error = CommunicationError::deref_err(345, "broken stuff"); match error { - CommunicationError::DerefErr { msg, .. } => assert_eq!(msg, "broken stuff"), + CommunicationError::DerefErr { offset, msg, .. } => { + assert_eq!(offset, 345); + assert_eq!(msg, "broken stuff"); + } e => panic!("Unexpected error: {:?}", e), } } diff --git a/packages/vm/src/memory.rs b/packages/vm/src/memory.rs index 5453a2afb6..d6e409f5ba 100644 --- a/packages/vm/src/memory.rs +++ b/packages/vm/src/memory.rs @@ -140,6 +140,7 @@ fn get_region(ctx: &Ctx, ptr: u32) -> CommunicationResult { match wptr.deref(memory) { Some(cell) => Ok(cell.get()), None => Err(CommunicationError::deref_err( + ptr, "Could not dereference this pointer to a Region", )), } @@ -156,6 +157,7 @@ fn set_region(ctx: &Ctx, ptr: u32, data: Region) -> CommunicationResult<()> { Ok(()) } None => Err(CommunicationError::deref_err( + ptr, "Could not dereference this pointer to a Region", )), } From 9bdd795ca463044d568f24aae8bac5f88be3c362 Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Wed, 10 Jun 2020 14:13:17 +0200 Subject: [PATCH 4/6] Replace panics with CommunicationError::deref_err --- packages/vm/src/memory.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/vm/src/memory.rs b/packages/vm/src/memory.rs index d6e409f5ba..81355eba04 100644 --- a/packages/vm/src/memory.rs +++ b/packages/vm/src/memory.rs @@ -83,11 +83,11 @@ pub fn read_region(ctx: &Ctx, ptr: u32, max_length: usize) -> VmResult> } 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.", region, memory.size().bytes().0 - ), + )).into()), } } @@ -125,11 +125,11 @@ pub fn write_region(ctx: &Ctx, ptr: u32, data: &[u8]) -> VmResult<()> { set_region(ctx, ptr, region)?; 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.", region, memory.size().bytes().0 - ), + )).into()), } } From 08e733477f8d76135e038d3a1b5691b844daecfa Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Wed, 10 Jun 2020 14:16:32 +0200 Subject: [PATCH 5/6] Add CHANGELOG entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index be699bc5f0..2974df1145 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,9 @@ directly to get back the external dependencies. - Rename `MockQuerier::with_staking` to `MockQuerier::update_staking` to match `::update_balance`. +- Instead of panicking, `read_region`/`write_region`/`get_region`/`set_region` + now return a new `CommunicationError::DerefErr` when dereferencing a pointer + provided by the contract fails. ## 0.8.1 (2020-06-08) From 9ec07b6411b542128e22957148d60a8bdeb224f1 Mon Sep 17 00:00:00 2001 From: Simon Warta Date: Wed, 10 Jun 2020 22:27:42 +0200 Subject: [PATCH 6/6] Improve error messages --- packages/vm/src/memory.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/vm/src/memory.rs b/packages/vm/src/memory.rs index 81355eba04..f58e313003 100644 --- a/packages/vm/src/memory.rs +++ b/packages/vm/src/memory.rs @@ -84,7 +84,7 @@ pub fn read_region(ctx: &Ctx, ptr: u32, max_length: usize) -> VmResult> Ok(result) } 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.", + "Tried to access memory of region {:?} in wasm memory of size {} bytes. This typically happens when the given Region pointer does not point to a proper Region struct.", region, memory.size().bytes().0 )).into()), @@ -126,7 +126,7 @@ pub fn write_region(ctx: &Ctx, ptr: u32, data: &[u8]) -> VmResult<()> { Ok(()) }, 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.", + "Tried to access memory of region {:?} in wasm memory of size {} bytes. This typically happens when the given Region pointer does not point to a proper Region struct.", region, memory.size().bytes().0 )).into()),