diff --git a/CHANGELOG.md b/CHANGELOG.md index f479177eab..65746d2988 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,6 +82,9 @@ contract. A standard library should ensure this never happens by correctly encoding string input values. - Merge trait `ReadonlyStorage` into `Storage`. +- The imports `canonicalize_address` and `humanize_address` now return a memory + address to an error `Region`. If this address is 0, the call succeeded. +- Bump `cosmwasm_vm_version_1` to `cosmwasm_vm_version_2`. ## 0.8.1 (2020-06-08) diff --git a/README.md b/README.md index a4a987388c..065c44a37b 100644 --- a/README.md +++ b/README.md @@ -138,8 +138,8 @@ extern "C" { #[cfg(feature = "iterator")] fn db_next(iterator_id: u32) -> u32; - fn canonicalize_address(source: u32, destination: u32) -> i32; - fn humanize_address(source: u32, destination: u32) -> i32; + fn canonicalize_address(source: u32, destination: u32) -> u32; + fn humanize_address(source: u32, destination: u32) -> u32; /// Executes a query on the chain (import). Not to be confused with the /// query export, which queries the state of the contract. diff --git a/packages/std/src/entry_points.rs b/packages/std/src/entry_points.rs index 3b7c828615..0c61916518 100644 --- a/packages/std/src/entry_points.rs +++ b/packages/std/src/entry_points.rs @@ -96,7 +96,7 @@ macro_rules! create_entry_points { $crate::create_entry_points!(@migration; $contract, $migration); - // Other C externs like cosmwasm_vm_version_1, allocate, deallocate are available + // Other C externs like cosmwasm_vm_version_2, allocate, deallocate are available // automatically because we `use cosmwasm_std`. } }; diff --git a/packages/std/src/exports.rs b/packages/std/src/exports.rs index c812d26c66..3ecb8f3b67 100644 --- a/packages/std/src/exports.rs +++ b/packages/std/src/exports.rs @@ -1,6 +1,6 @@ //! exports exposes the public wasm API //! -//! cosmwasm_vm_version_1, allocate and deallocate turn into Wasm exports +//! cosmwasm_vm_version_2, allocate and deallocate turn into Wasm exports //! as soon as cosmwasm_std is `use`d in the contract, even privately. //! //! do_init and do_wrapper should be wrapped with a extern "C" entry point @@ -26,7 +26,7 @@ extern "C" fn requires_staking() -> () {} /// They can be checked by cosmwasm_vm. /// Update this whenever the Wasm VM interface breaks. #[no_mangle] -extern "C" fn cosmwasm_vm_version_1() -> () {} +extern "C" fn cosmwasm_vm_version_2() -> () {} /// allocate reserves the given number of bytes in wasm memory and returns a pointer /// to a Region defining this data. This space is managed by the calling process diff --git a/packages/std/src/imports.rs b/packages/std/src/imports.rs index dc5801c76f..00554a0e7c 100644 --- a/packages/std/src/imports.rs +++ b/packages/std/src/imports.rs @@ -28,8 +28,8 @@ extern "C" { #[cfg(feature = "iterator")] fn db_next(iterator_id: u32) -> u32; - fn canonicalize_address(source: u32, destination: u32) -> i32; - fn humanize_address(source: u32, destination: u32) -> i32; + fn canonicalize_address(source: u32, destination: u32) -> u32; + fn humanize_address(source: u32, destination: u32) -> u32; /// Executes a query on the chain (import). Not to be confused with the /// query export, which queries the state of the contract. @@ -157,9 +157,13 @@ impl Api for ExternalApi { let send_ptr = &*send as *const Region as u32; let canon = alloc(CANONICAL_ADDRESS_BUFFER_LENGTH); - let read = unsafe { canonicalize_address(send_ptr, canon as u32) }; - if read < 0 { - return Err(StdError::generic_err("canonicalize_address returned error")); + let result = unsafe { canonicalize_address(send_ptr, canon as u32) }; + if result != 0 { + let error = unsafe { consume_string_region_written_by_vm(result as *mut Region) }; + return Err(StdError::generic_err(format!( + "canonicalize_address errored: {}", + error + ))); } let out = unsafe { consume_region(canon) }; @@ -171,18 +175,28 @@ impl Api for ExternalApi { let send_ptr = &*send as *const Region as u32; let human = alloc(HUMAN_ADDRESS_BUFFER_LENGTH); - let read = unsafe { humanize_address(send_ptr, human as u32) }; - if read < 0 { - return Err(StdError::generic_err("humanize_address returned error")); + let result = unsafe { humanize_address(send_ptr, human as u32) }; + if result != 0 { + let error = unsafe { consume_string_region_written_by_vm(result as *mut Region) }; + return Err(StdError::generic_err(format!( + "humanize_address errored: {}", + error + ))); } - let out = unsafe { consume_region(human) }; - // we know input was correct when created, so let's save some bytes - let result = unsafe { String::from_utf8_unchecked(out) }; - Ok(HumanAddr(result)) + let address = unsafe { consume_string_region_written_by_vm(human) }; + Ok(address.into()) } } +/// Takes a pointer to a Region and reads the data into a String. +/// This is for trusted string sources only. +unsafe fn consume_string_region_written_by_vm(from: *mut Region) -> String { + let data = consume_region(from); + // We trust the VM/chain to return correct UTF-8, so let's save some gas + String::from_utf8_unchecked(data) +} + /// A stateless convenience wrapper around imports provided by the VM pub struct ExternalQuerier {} diff --git a/packages/vm/src/compatability.rs b/packages/vm/src/compatability.rs index 7da881a6c6..f113811b7d 100644 --- a/packages/vm/src/compatability.rs +++ b/packages/vm/src/compatability.rs @@ -25,7 +25,7 @@ static SUPPORTED_IMPORTS: &[&str] = &[ /// Basically, anything that is used in calls.rs /// This is unlikely to change much, must be frozen at 1.0 to avoid breaking existing contracts static REQUIRED_EXPORTS: &[&str] = &[ - "cosmwasm_vm_version_1", + "cosmwasm_vm_version_2", "query", "init", "handle", @@ -176,7 +176,7 @@ mod test { fn test_check_wasm_old_contract() { match check_wasm(CONTRACT_0_7, &default_features()) { Err(VmError::StaticValidationErr { msg, .. }) => assert!(msg.starts_with( - "Wasm contract doesn't have required export: \"cosmwasm_vm_version_1\"" + "Wasm contract doesn't have required export: \"cosmwasm_vm_version_2\"" )), Err(e) => panic!("Unexpected error {:?}", e), Ok(_) => panic!("This must not succeeed"), @@ -184,7 +184,7 @@ mod test { match check_wasm(CONTRACT_0_6, &default_features()) { Err(VmError::StaticValidationErr { msg, .. }) => assert!(msg.starts_with( - "Wasm contract doesn't have required export: \"cosmwasm_vm_version_1\"" + "Wasm contract doesn't have required export: \"cosmwasm_vm_version_2\"" )), Err(e) => panic!("Unexpected error {:?}", e), Ok(_) => panic!("This must not succeeed"), @@ -309,7 +309,7 @@ mod test { match check_wasm_exports(&module) { Err(VmError::StaticValidationErr { msg, .. }) => { assert!(msg.starts_with( - "Wasm contract doesn't have required export: \"cosmwasm_vm_version_1\"" + "Wasm contract doesn't have required export: \"cosmwasm_vm_version_2\"" )); } Err(e) => panic!("Unexpected error {:?}", e), @@ -323,7 +323,7 @@ mod test { match check_wasm_exports(&module) { Err(VmError::StaticValidationErr { msg, .. }) => { assert!(msg.starts_with( - "Wasm contract doesn't have required export: \"cosmwasm_vm_version_1\"" + "Wasm contract doesn't have required export: \"cosmwasm_vm_version_2\"" )); } Err(e) => panic!("Unexpected error {:?}", e), diff --git a/packages/vm/src/context.rs b/packages/vm/src/context.rs index eb9d18c371..ca5e07c58a 100644 --- a/packages/vm/src/context.rs +++ b/packages/vm/src/context.rs @@ -380,8 +380,8 @@ mod test { "db_scan" => Func::new(|_a: u32, _b: u32, _c: i32| -> u32 { 0 }), "db_next" => Func::new(|_a: u32| -> u32 { 0 }), "query_chain" => Func::new(|_a: u32| -> u32 { 0 }), - "canonicalize_address" => Func::new(|_a: u32, _b: u32| -> i32 { 0 }), - "humanize_address" => Func::new(|_a: u32, _b: u32| -> i32 { 0 }), + "canonicalize_address" => Func::new(|_a: u32, _b: u32| -> u32 { 0 }), + "humanize_address" => Func::new(|_a: u32, _b: u32| -> u32 { 0 }), }, }; let mut instance = Box::from(module.instantiate(&import_obj).unwrap()); diff --git a/packages/vm/src/errors.rs b/packages/vm/src/errors.rs index b4309f171a..48f483cfb5 100644 --- a/packages/vm/src/errors.rs +++ b/packages/vm/src/errors.rs @@ -269,6 +269,7 @@ impl CommunicationError { InvalidOrder { value }.build() } + #[allow(dead_code)] pub(crate) fn invalid_utf8(msg: S) -> Self { InvalidUtf8 { msg: msg.to_string(), diff --git a/packages/vm/src/imports.rs b/packages/vm/src/imports.rs index 4b8ee29d97..8f8b3bc9f6 100644 --- a/packages/vm/src/imports.rs +++ b/packages/vm/src/imports.rs @@ -18,7 +18,7 @@ use crate::conversion::to_u32; use crate::errors::{CommunicationError, VmError, VmResult}; #[cfg(feature = "iterator")] use crate::memory::maybe_read_region; -use crate::memory::{read_region, read_string_region, write_region}; +use crate::memory::{read_region, write_region}; use crate::serde::to_vec; use crate::traits::{Api, Querier, Storage}; @@ -46,17 +46,7 @@ pub fn do_read(ctx: &mut Ctx, key_ptr: u32) -> VmResult< Some(data) => data, None => return Ok(0), }; - - let out_ptr = with_func_from_context::(ctx, "allocate", |allocate| { - let out_size = to_u32(out_data.len())?; - let ptr = allocate.call(out_size)?; - if ptr == 0 { - return Err(CommunicationError::zero_address().into()); - } - Ok(ptr) - })?; - write_region(ctx, out_ptr, &out_data)?; - Ok(out_ptr) + write_to_contract::(ctx, &out_data) } /// Writes a storage entry from Wasm memory into the VM's storage @@ -90,16 +80,32 @@ pub fn do_remove(ctx: &mut Ctx, key_ptr: u32) -> VmResul Ok(()) } -pub fn do_canonicalize_address( +pub fn do_canonicalize_address( api: A, ctx: &mut Ctx, source_ptr: u32, destination_ptr: u32, -) -> VmResult<()> { - let human: HumanAddr = read_string_region(ctx, source_ptr, MAX_LENGTH_HUMAN_ADDRESS)?.into(); - let canon = api.canonical_address(&human)?; - write_region(ctx, destination_ptr, canon.as_slice())?; - Ok(()) +) -> VmResult { + let source_data = read_region(ctx, source_ptr, MAX_LENGTH_HUMAN_ADDRESS)?; + if source_data.is_empty() { + return Ok(write_to_contract::(ctx, b"Input is empty")?); + } + + let source_string = match String::from_utf8(source_data) { + Ok(s) => s, + Err(_) => return Ok(write_to_contract::(ctx, b"Input is not valid UTF-8")?), + }; + let human: HumanAddr = source_string.into(); + match api.canonical_address(&human) { + Ok(canon) => { + write_region(ctx, destination_ptr, canon.as_slice())?; + Ok(0) + } + // This check is unrelyable since we cannot tell by the error type if the error should be + // reported to the contract or indicates a broken backend. + // Err(FfiError::Other { error, .. }) => Ok(write_to_contract::(ctx, &error.as_bytes())?), + Err(error) => Err(error.into()), + } } pub fn do_humanize_address( @@ -107,11 +113,26 @@ pub fn do_humanize_address( ctx: &mut Ctx, source_ptr: u32, destination_ptr: u32, -) -> VmResult<()> { +) -> VmResult { let canonical = Binary(read_region(ctx, source_ptr, MAX_LENGTH_CANONICAL_ADDRESS)?); + // TODO: how to report API errors back to the contract? let human = api.human_address(&CanonicalAddr(canonical))?; write_region(ctx, destination_ptr, human.as_str().as_bytes())?; - Ok(()) + Ok(0) +} + +/// Creates a Region in the contract, writes the given data to it and returns the memory location +fn write_to_contract(ctx: &mut Ctx, input: &[u8]) -> VmResult { + let target_ptr = with_func_from_context::(ctx, "allocate", |allocate| { + let out_size = to_u32(input.len())?; + let ptr = allocate.call(out_size)?; + if ptr == 0 { + return Err(CommunicationError::zero_address().into()); + } + Ok(ptr) + })?; + write_region(ctx, target_ptr, input)?; + Ok(target_ptr) } pub fn do_query_chain(ctx: &mut Ctx, request_ptr: u32) -> VmResult { @@ -122,16 +143,7 @@ pub fn do_query_chain(ctx: &mut Ctx, request_ptr: u32) - try_consume_gas::(ctx, used_gas)?; let serialized = to_vec(&res)?; - let out_ptr = with_func_from_context::(ctx, "allocate", |allocate| { - let out_size = to_u32(serialized.len())?; - let ptr = allocate.call(out_size)?; - if ptr == 0 { - return Err(CommunicationError::zero_address().into()); - } - Ok(ptr) - })?; - write_region(ctx, out_ptr, &serialized)?; - Ok(out_ptr) + write_to_contract::(ctx, &serialized) } #[cfg(feature = "iterator")] @@ -173,16 +185,7 @@ pub fn do_next(ctx: &mut Ctx, iterator_id: u32) -> VmRes out_data.extend(key); out_data.extend_from_slice(&keylen_bytes); - let out_ptr = with_func_from_context::(ctx, "allocate", |allocate| { - let out_size = to_u32(out_data.len())?; - let ptr = allocate.call(out_size)?; - if ptr == 0 { - return Err(CommunicationError::zero_address().into()); - } - Ok(ptr) - })?; - write_region(ctx, out_ptr, &out_data)?; - Ok(out_ptr) + write_to_contract::(ctx, &out_data) } #[cfg(test)] @@ -206,6 +209,7 @@ mod test { static CONTRACT: &[u8] = include_bytes!("../testdata/contract.wasm"); // shorthands for function generics below + type MA = MockApi; type MS = MockStorage; type MQ = MockQuerier; @@ -237,8 +241,8 @@ mod test { "db_scan" => Func::new(|_a: u32, _b: u32, _c: i32| -> u32 { 0 }), "db_next" => Func::new(|_a: u32| -> u32 { 0 }), "query_chain" => Func::new(|_a: u32| -> u32 { 0 }), - "canonicalize_address" => Func::new(|_a: i32, _b: i32| -> i32 { 0 }), - "humanize_address" => Func::new(|_a: i32, _b: i32| -> i32 { 0 }), + "canonicalize_address" => Func::new(|_a: i32, _b: i32| -> u32 { 0 }), + "humanize_address" => Func::new(|_a: i32, _b: i32| -> u32 { 0 }), }, }; let mut instance = Box::from(module.instantiate(&import_obj).unwrap()); @@ -542,7 +546,7 @@ mod test { leave_default_data(ctx); let api = MockApi::new(8); - do_canonicalize_address(api, ctx, source_ptr, dest_ptr).unwrap(); + do_canonicalize_address::(api, ctx, source_ptr, dest_ptr).unwrap(); assert_eq!(force_read(ctx, dest_ptr), b"foo\0\0\0\0\0"); } @@ -559,31 +563,25 @@ mod test { leave_default_data(ctx); let api = MockApi::new(8); - let result = do_canonicalize_address(api, ctx, source_ptr1, dest_ptr); - match result.unwrap_err() { - VmError::CommunicationErr { - source: CommunicationError::InvalidUtf8 { .. }, - } => {} - err => panic!("Incorrect error returned: {:?}", err), - } + let res = do_canonicalize_address::(api, ctx, source_ptr1, dest_ptr).unwrap(); + assert_ne!(res, 0); + let err = String::from_utf8(force_read(ctx, res)).unwrap(); + assert_eq!(err, "Input is not valid UTF-8"); - // TODO: would be nice if do_canonicalize_address could differentiate between different errors - // from Api.canonical_address and return INVALID_INPUT for those cases as well. - let result = do_canonicalize_address(api, ctx, source_ptr2, dest_ptr); - match result.unwrap_err() { - VmError::FfiErr { - source: FfiError::Other { .. }, - } => {} - err => panic!("Incorrect error returned: {:?}", err), - }; + let res = do_canonicalize_address::(api, ctx, source_ptr2, dest_ptr).unwrap(); + assert_ne!(res, 0); + let err = String::from_utf8(force_read(ctx, res)).unwrap(); + assert_eq!(err, "Input is empty"); - let result = do_canonicalize_address(api, ctx, source_ptr3, dest_ptr); + let result = do_canonicalize_address::(api, ctx, source_ptr3, dest_ptr); match result.unwrap_err() { VmError::FfiErr { - source: FfiError::Other { .. }, - } => {} + source: FfiError::Other { error, .. }, + } => { + assert_eq!(error, "Invalid input: human address too long"); + } err => panic!("Incorrect error returned: {:?}", err), - }; + } } #[test] @@ -597,7 +595,7 @@ mod test { leave_default_data(ctx); let api = MockApi::new(8); - let result = do_canonicalize_address(api, ctx, source_ptr, dest_ptr); + let result = do_canonicalize_address::(api, ctx, source_ptr, dest_ptr); match result.unwrap_err() { VmError::CommunicationErr { source: @@ -623,7 +621,7 @@ mod test { leave_default_data(ctx); let api = MockApi::new(8); - let result = do_canonicalize_address(api, ctx, source_ptr, dest_ptr); + let result = do_canonicalize_address::(api, ctx, source_ptr, dest_ptr); match result.unwrap_err() { VmError::CommunicationErr { source: CommunicationError::RegionTooSmall { size, required, .. }, @@ -646,7 +644,8 @@ mod test { leave_default_data(ctx); let api = MockApi::new(8); - do_humanize_address(api, ctx, source_ptr, dest_ptr).unwrap(); + let error_ptr = do_humanize_address(api, ctx, source_ptr, dest_ptr).unwrap(); + assert_eq!(error_ptr, 0); assert_eq!(force_read(ctx, dest_ptr), b"foo"); } diff --git a/packages/vm/src/instance.rs b/packages/vm/src/instance.rs index bfee1c5e62..e644de64e3 100644 --- a/packages/vm/src/instance.rs +++ b/packages/vm/src/instance.rs @@ -75,7 +75,6 @@ where }), // Writes the given value into the database entry at the given key. // Ownership of both input and output pointer is not transferred to the host. - // Returns 0 on success. Returns negative value on error. "db_write" => Func::new(move |ctx: &mut Ctx, key_ptr: u32, value_ptr: u32| -> VmResult<()> { do_write::(ctx, key_ptr, value_ptr) }), @@ -83,25 +82,22 @@ where // scans will not find this key. // At the moment it is not possible to differentiate between a key that existed before and one that did not exist (https://github.com/CosmWasm/cosmwasm/issues/290). // Ownership of both key pointer is not transferred to the host. - // Returns 0 on success. Returns negative value on error. "db_remove" => Func::new(move |ctx: &mut Ctx, key_ptr: u32| -> VmResult<()> { do_remove::(ctx, key_ptr) }), // Reads human address from source_ptr and writes canonicalized representation to destination_ptr. // A prepared and sufficiently large memory Region is expected at destination_ptr that points to pre-allocated memory. - // Returns 0 on success. Returns negative value on error. + // Returns 0 on success. Returns a non-zero memory location to a Region containing an UTF-8 encoded error string for invalid inputs. // Ownership of both input and output pointer is not transferred to the host. - "canonicalize_address" => Func::new(move |ctx: &mut Ctx, source_ptr: u32, destination_ptr: u32| -> VmResult { - do_canonicalize_address(api, ctx, source_ptr, destination_ptr)?; - Ok(0) // TODO: work out error handling strategy (https://github.com/CosmWasm/cosmwasm/issues/433) + "canonicalize_address" => Func::new(move |ctx: &mut Ctx, source_ptr: u32, destination_ptr: u32| -> VmResult { + do_canonicalize_address::(api, ctx, source_ptr, destination_ptr) }), // Reads canonical address from source_ptr and writes humanized representation to destination_ptr. // A prepared and sufficiently large memory Region is expected at destination_ptr that points to pre-allocated memory. - // Returns 0 on success. Returns negative value on error. + // Returns 0 on success. Returns a non-zero memory location to a Region containing an UTF-8 encoded error string for invalid inputs. // Ownership of both input and output pointer is not transferred to the host. - "humanize_address" => Func::new(move |ctx: &mut Ctx, source_ptr: u32, destination_ptr: u32| -> VmResult { - do_humanize_address(api, ctx, source_ptr, destination_ptr)?; - Ok(0) // TODO: work out error handling strategy (https://github.com/CosmWasm/cosmwasm/issues/433) + "humanize_address" => Func::new(move |ctx: &mut Ctx, source_ptr: u32, destination_ptr: u32| -> VmResult { + do_humanize_address(api, ctx, source_ptr, destination_ptr) }), "query_chain" => Func::new(move |ctx: &mut Ctx, request_ptr: u32| -> VmResult { do_query_chain::(ctx, request_ptr) @@ -662,7 +658,7 @@ mod singlepass_test { let init_used = orig_gas - instance.get_gas_left(); println!("init used: {}", init_used); - assert_eq!(init_used, 65338); + assert_eq!(init_used, 70810); } #[test] @@ -686,7 +682,7 @@ mod singlepass_test { let handle_used = gas_before_handle - instance.get_gas_left(); println!("handle used: {}", handle_used); - assert_eq!(handle_used, 96318); + assert_eq!(handle_used, 97825); } #[test] diff --git a/packages/vm/src/memory.rs b/packages/vm/src/memory.rs index 2fa4ccfe4f..8413680241 100644 --- a/packages/vm/src/memory.rs +++ b/packages/vm/src/memory.rs @@ -90,12 +90,6 @@ pub fn read_region(ctx: &Ctx, ptr: u32, max_length: usize) -> VmResult> } } -pub fn read_string_region(ctx: &Ctx, ptr: u32, max_length: usize) -> VmResult { - let data = read_region(ctx, ptr, max_length)?; - let out = String::from_utf8(data).map_err(CommunicationError::invalid_utf8)?; - Ok(out) -} - /// maybe_read_region is like read_region, but gracefully handles null pointer (0) by returning None /// meant to be used where the argument is optional (like scan) #[cfg(feature = "iterator")] diff --git a/packages/vm/testdata/contract_0.9.wasm b/packages/vm/testdata/contract_0.9.wasm index a6bd46f9d6..284120d1cb 100644 Binary files a/packages/vm/testdata/contract_0.9.wasm and b/packages/vm/testdata/contract_0.9.wasm differ