read_storage panics (paritytech#3589)

* Add a failing test case

* Actual fix

* read_child_storage, fix wasm side

* Bump the impl version.

* Alternative (paritytech#3597)

* Update with_std.rs

* Update with_std.rs

* Update wasm_executor.rs

* Update wasm_executor.rs

* Update with_std.rs

* Update wasm_executor.rs

Author: pepyakin

core/executor/src/wasm_executor.rs

@@ -629,9 +629,9 @@ impl_wasm_host_interface! {
 			)?;
 
 			if let Some(value) = maybe_value {
-				let value = &value[value_offset as usize..];
-				let written = std::cmp::min(value_len as usize, value.len());
-				context.write_memory(value_data, &value[..written])
+				let data = &value[value.len().min(value_offset as usize)..];
+				let written = std::cmp::min(value_len as usize, data.len());
+				context.write_memory(value_data, &data[..written])
 					.map_err(|_| "Invalid attempt to set value in ext_get_storage_into")?;
 				Ok(value.len() as u32)
 			} else {
@@ -658,10 +658,10 @@ impl_wasm_host_interface! {
 			)?;
 
 			if let Some(value) = maybe_value {
-				let value = &value[value_offset as usize..];
-				let written = std::cmp::min(value_len as usize, value.len());
-				context.write_memory(value_data, &value[..written])
-					.map_err(|_| "Invalid attempt to set value in ext_get_child_storage_into")?;
+				let data = &value[value.len().min(value_offset as usize)..];
+				let written = std::cmp::min(value_len as usize, data.len());
+				context.write_memory(value_data, &data[..written])
+					.map_err(|_| "Invalid attempt to get value in ext_get_child_storage_into")?;
 				Ok(value.len() as u32)
 			} else {
 				Ok(u32::max_value())

core/sr-io/with_std.rs

@@ -54,9 +54,9 @@ impl StorageApi for () {
 
 	fn read_storage(key: &[u8], value_out: &mut [u8], value_offset: usize) -> Option<usize> {
 		ext::with(|ext| ext.storage(key).map(|value| {
-			let value = &value[value_offset..];
-			let written = std::cmp::min(value.len(), value_out.len());
-			value_out[..written].copy_from_slice(&value[..written]);
+			let data = &value[value_offset.min(value.len())..];
+			let written = std::cmp::min(data.len(), value_out.len());
+			value_out[..written].copy_from_slice(&data[..written]);
 			value.len()
 		})).expect("read_storage cannot be called outside of an Externalities-provided environment.")
 	}
@@ -85,9 +85,9 @@ impl StorageApi for () {
 			let storage_key = child_storage_key_or_panic(storage_key);
 			ext.child_storage(storage_key, key)
 				.map(|value| {
-					let value = &value[value_offset..];
-					let written = std::cmp::min(value.len(), value_out.len());
-					value_out[..written].copy_from_slice(&value[..written]);
+					let data = &value[value_offset.min(value.len())..];
+					let written = std::cmp::min(data.len(), value_out.len());
+					value_out[..written].copy_from_slice(&data[..written]);
 					value.len()
 				})
 		})

core/test-runtime/src/lib.rs

@@ -280,6 +280,8 @@ cfg_if! {
 				///
 				/// Returns the signature generated for the message `sr25519`.
 				fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic);
+				/// Run various tests against storage.
+				fn test_storage();
 			}
 		}
 	} else {
@@ -322,6 +324,8 @@ cfg_if! {
 				///
 				/// Returns the signature generated for the message `sr25519`.
 				fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic);
+				/// Run various tests against storage.
+				fn test_storage();
 			}
 		}
 	}
@@ -590,6 +594,11 @@ cfg_if! {
 				fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic) {
 					test_sr25519_crypto()
 				}
+
+				fn test_storage() {
+					test_read_storage();
+					test_read_child_storage();
+				}
 			}
 
 			impl aura_primitives::AuraApi<Block, AuraId> for Runtime {
@@ -805,6 +814,11 @@ cfg_if! {
 				fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic) {
 					test_sr25519_crypto()
 				}
+
+				fn test_storage() {
+					test_read_storage();
+					test_read_child_storage();
+				}
 			}
 
 			impl aura_primitives::AuraApi<Block, AuraId> for Runtime {
@@ -887,6 +901,46 @@ fn test_sr25519_crypto() -> (sr25519::AppSignature, sr25519::AppPublic) {
 	(signature, public0)
 }
 
+fn test_read_storage() {
+	const KEY: &[u8] = b":read_storage";
+	runtime_io::set_storage(KEY, b"test");
+
+	let mut v = [0u8; 4];
+	let r = runtime_io::read_storage(
+		KEY,
+		&mut v,
+		0
+	);
+	assert_eq!(r, Some(4));
+	assert_eq!(&v, b"test");
+
+	let mut v = [0u8; 4];
+	let r = runtime_io::read_storage(KEY, &mut v, 8);
+	assert_eq!(r, Some(4));
+	assert_eq!(&v, &[0, 0, 0, 0]);
+}
+
+fn test_read_child_storage() {
+	const CHILD_KEY: &[u8] = b":child_storage:default:read_child_storage";
+	const KEY: &[u8] = b":read_child_storage";
+	runtime_io::set_child_storage(CHILD_KEY, KEY, b"test");
+
+	let mut v = [0u8; 4];
+	let r = runtime_io::read_child_storage(
+		CHILD_KEY,
+		KEY,
+		&mut v,
+		0
+	);
+	assert_eq!(r, Some(4));
+	assert_eq!(&v, b"test");
+
+	let mut v = [0u8; 4];
+	let r = runtime_io::read_child_storage(CHILD_KEY, KEY, &mut v, 8);
+	assert_eq!(r, Some(4));
+	assert_eq!(&v, &[0, 0, 0, 0]);
+}
+
 #[cfg(test)]
 mod tests {
 	use substrate_test_runtime_client::{
@@ -981,4 +1035,15 @@ mod tests {
 		let ret = runtime_api.vec_with_capacity(&new_block_id, 1048576);
 		assert!(ret.is_ok());
 	}
+
+	#[test]
+	fn test_storage() {
+		let client = TestClientBuilder::new()
+			.set_execution_strategy(ExecutionStrategy::Both)
+			.build();
+		let runtime_api = client.runtime_api();
+		let block_id = BlockId::Number(client.info().chain.best_number);
+
+		runtime_api.test_storage(&block_id).unwrap();
+	}
 }

node/runtime/src/lib.rs

@@ -83,7 +83,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
 	spec_version: 156,
-	impl_version: 156,
+	impl_version: 157,
 	apis: RUNTIME_API_VERSIONS,
 };