Fair reusing of wasm runtime instances (#3011)

* Add test from original bug report

Original is from @pepyakin in 3d7b27f3421818e8d6de568e02fbc2947a06246b.
I adapted it to work with the latest master.

* No longer cleanup module instance

* Replace runtime cache with synchronous clone

* Fix test

* Preserve initial runtime memory and restore it on fetch

* Remove leftover comment

* Fix style

* Improve variable naming

* Replace get_into() with get()

* Handle missing memory export better

* Return earlier when creating runtime first time

* Improve comments

* fmt

* Fix #2967.

* Eradicate `code` from `Error::InvalidCode`

* tidy

* A state snapshot doc.

* Store multiple runtimes by hash.

* Get rid of deref.

* Docs

* Use Self for instantiate_module

* REVERT ME

* Should be ok

* Commit

* Remove dbg

* Use fast-memory's erase

* Clean and undo hacks.

* Introduce a dedicated error for heap_base

* Ban the start function.

* Clean, docs and refactor

* Add rustflags.

* Update Cargo.lock

* Apply Basti's suggestions

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

* Rename allocates_huge_stack_array

* Extend TestClientBuilder with set_heap_pages

* Update the test.

* Update core/executor/src/wasm_executor.rs

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

* Update core/executor/src/wasm_runtimes_cache.rs

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

* Update core/executor/src/error.rs

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

* Update core/executor/src/error.rs

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

* Fix tests.

* Update cargo-lock

* Use wasmi master

* Use master wasmi

* Move tests.

* Use wasmi crates.io

* Update Cargo.lock

* Fix build.rs

* Bump runtime version

* Revert initial_heap_pages renaming

* Bump wasmi up to 0.5.0

* Bump runtime version

* Don't restore an instance every now and then

* Update core/executor/src/wasm_runtimes_cache.rs

Co-Authored-By: DemiMarie-parity <48690212+DemiMarie-parity@users.noreply.github.com>

* Propagate error in CacheError

* Clarify the get_heap_base call in instantiation

* Supply --export=__heap_base

See https://reviews.llvm.org/D62744

Co-authored-by: Jim Posen <jim.posen@gmail.com>

* Bump version.

* Use combinators for segments.

* Fix build.rs

* Fix build.rs for runtime-test

substrate/core/test-runtime/Cargo.toml

@@ -32,6 +32,7 @@ srml-system = { path = "../../srml/system", default-features = false }
 [dev-dependencies]
 substrate-executor = { path = "../executor" }
 substrate-test-runtime-client = { path = "./client" }
+state_machine = { package = "substrate-state-machine", path = "../state-machine" }
 
 [build-dependencies]
 wasm-builder-runner = { package = "substrate-wasm-builder-runner", version = "1.0.2", path = "../utils/wasm-builder-runner" }

substrate/core/test-runtime/build.rs

@@ -14,14 +14,22 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
-use wasm_builder_runner::{build_current_project, WasmBuilderSource};
+use wasm_builder_runner::{build_current_project_with_rustflags, WasmBuilderSource};
 
 fn main() {
-	build_current_project(
+	build_current_project_with_rustflags(
 		"wasm_binary.rs",
 		WasmBuilderSource::CratesOrPath {
 			path: "../utils/wasm-builder",
 			version: "1.0.4",
 		},
+		// Note that we set the stack-size to 1MB explicitly even though it is set
+		// to this value by default. This is because some of our tests (`restoration_of_globals`)
+		// depend on the stack-size.
+		//
+		// The --export=__heap_base instructs LLD to export __heap_base as a global variable, which
+		// is used by the external memory allocator.
+		"-Clink-arg=-zstack-size=1048576 \
+		-Clink-arg=--export=__heap_base",
 	);
 }

substrate/core/test-runtime/client/src/lib.rs

@@ -95,11 +95,12 @@ pub type LightExecutor = client::light::call_executor::RemoteOrLocalCallExecutor
 #[derive(Default)]
 pub struct GenesisParameters {
 	support_changes_trie: bool,
+	heap_pages_override: Option<u64>,
 }
 
 impl generic_test_client::GenesisInit for GenesisParameters {
 	fn genesis_storage(&self) -> (StorageOverlay, ChildrenStorageOverlay) {
-		let mut storage = genesis_config(self.support_changes_trie).genesis_map();
+		let mut storage = genesis_config(self.support_changes_trie, self.heap_pages_override).genesis_map();
 
 		let state_root = <<<runtime::Block as BlockT>::Header as HeaderT>::Hashing as HashT>::trie_root(
 			storage.clone().into_iter()
@@ -145,6 +146,9 @@ pub trait TestClientBuilderExt<B>: Sized {
 	/// Enable or disable support for changes trie in genesis.
 	fn set_support_changes_trie(self, support_changes_trie: bool) -> Self;
 
+	/// Override the default value for Wasm heap pages.
+	fn set_heap_pages(self, heap_pages: u64) -> Self;
+
 	/// Build the test client.
 	fn build(self) -> Client<B> {
 		self.build_with_longest_chain().0
@@ -160,6 +164,11 @@ impl<B> TestClientBuilderExt<B> for TestClientBuilder<
 > where
 	B: client::backend::Backend<runtime::Block, Blake2Hasher>,
 {
+	fn set_heap_pages(mut self, heap_pages: u64) -> Self {
+		self.genesis_init_mut().heap_pages_override = Some(heap_pages);
+		self
+	}
+
 	fn set_support_changes_trie(mut self, support_changes_trie: bool) -> Self {
 		self.genesis_init_mut().support_changes_trie = support_changes_trie;
 		self
@@ -170,17 +179,20 @@ impl<B> TestClientBuilderExt<B> for TestClientBuilder<
 	}
 }
 
-fn genesis_config(support_changes_trie: bool) -> GenesisConfig {
-	GenesisConfig::new(support_changes_trie, vec![
-		Sr25519Keyring::Alice.into(),
-		Sr25519Keyring::Bob.into(),
-		Sr25519Keyring::Charlie.into(),
-	], vec![
-		AccountKeyring::Alice.into(),
-		AccountKeyring::Bob.into(),
-		AccountKeyring::Charlie.into(),
-	],
-		1000
+fn genesis_config(support_changes_trie: bool, heap_pages_override: Option<u64>) -> GenesisConfig {
+	GenesisConfig::new(
+		support_changes_trie,
+		vec![
+			Sr25519Keyring::Alice.into(),
+			Sr25519Keyring::Bob.into(),
+			Sr25519Keyring::Charlie.into(),
+		], vec![
+			AccountKeyring::Alice.into(),
+			AccountKeyring::Bob.into(),
+			AccountKeyring::Charlie.into(),
+		],
+		1000,
+		heap_pages_override,
 	)
 }
 

substrate/core/test-runtime/src/genesismap.rs

@@ -28,6 +28,7 @@ pub struct GenesisConfig {
 	pub changes_trie_config: Option<ChangesTrieConfiguration>,
 	pub authorities: Vec<AuthorityId>,
 	pub balances: Vec<(AccountId, u64)>,
+	pub heap_pages_override: Option<u64>,
 }
 
 impl GenesisConfig {
@@ -35,7 +36,8 @@ impl GenesisConfig {
 		support_changes_trie: bool,
 		authorities: Vec<AuthorityId>,
 		endowed_accounts: Vec<AccountId>,
-		balance: u64
+		balance: u64,
+		heap_pages_override: Option<u64>,
 	) -> Self {
 		GenesisConfig {
 			changes_trie_config: match support_changes_trie {
@@ -44,6 +46,7 @@ impl GenesisConfig {
 			},
 			authorities: authorities.clone(),
 			balances: endowed_accounts.into_iter().map(|a| (a, balance)).collect(),
+			heap_pages_override,
 		}
 	}
 
@@ -54,7 +57,10 @@ impl GenesisConfig {
 			.map(|(k, v)| (blake2_256(&k[..])[..].to_vec(), v.to_vec()))
 			.chain(vec![
 				(well_known_keys::CODE.into(), wasm_runtime),
-				(well_known_keys::HEAP_PAGES.into(), vec![].and(&(16 as u64))),
+				(
+					well_known_keys::HEAP_PAGES.into(),
+					vec![].and(&(self.heap_pages_override.unwrap_or(16 as u64))),
+				),
 			].into_iter())
 			.collect();
 		if let Some(ref changes_trie_config) = self.changes_trie_config {

substrate/core/test-runtime/src/lib.rs

@@ -256,6 +256,8 @@ cfg_if! {
 				fn use_trie() -> u64;
 				fn benchmark_indirect_call() -> u64;
 				fn benchmark_direct_call() -> u64;
+				fn returns_mutable_static() -> u64;
+				fn allocates_huge_stack_array(trap: bool) -> Vec<u8>;
 				/// Returns the initialized block number.
 				fn get_block_number() -> u64;
 				/// Takes and returns the initialized block number.
@@ -287,6 +289,8 @@ cfg_if! {
 				fn use_trie() -> u64;
 				fn benchmark_indirect_call() -> u64;
 				fn benchmark_direct_call() -> u64;
+				fn returns_mutable_static() -> u64;
+				fn allocates_huge_stack_array(trap: bool) -> Vec<u8>;
 				/// Returns the initialized block number.
 				fn get_block_number() -> u64;
 				/// Takes and returns the initialized block number.
@@ -404,6 +408,11 @@ fn code_using_trie() -> u64 {
 	iter_pairs.len() as u64
 }
 
+#[cfg(not(feature = "std"))]
+/// Mutable static variables should be always observed to have
+/// the initialized value at the start of a runtime call.
+static mut MUTABLE_STATIC: u64 = 32;
+
 cfg_if! {
 	if #[cfg(feature = "std")] {
 		impl_runtime_apis! {
@@ -509,6 +518,14 @@ cfg_if! {
 					(0..1000).fold(0, |p, i| p + benchmark_add_one(i))
 				}
 
+				fn returns_mutable_static() -> u64 {
+					unimplemented!("is not expected to be invoked from non-wasm builds");
+				}
+
+				fn allocates_huge_stack_array(_trap: bool) -> Vec<u8> {
+					unimplemented!("is not expected to be invoked from non-wasm builds");
+				}
+
 				fn get_block_number() -> u64 {
 					system::get_block_number().expect("Block number is initialized")
 				}
@@ -665,6 +682,41 @@ cfg_if! {
 					(0..10000).fold(0, |p, i| p + benchmark_add_one(i))
 				}
 
+				fn returns_mutable_static() -> u64 {
+					unsafe {
+						MUTABLE_STATIC += 1;
+						MUTABLE_STATIC
+					}
+				}
+
+				fn allocates_huge_stack_array(trap: bool) -> Vec<u8> {
+					// Allocate a stack frame that is approx. 75% of the stack (assuming it is 1MB).
+					// This will just decrease (stacks in wasm32-u-u grow downwards) the stack
+					// pointer. This won't trap on the current compilers.
+					let mut data = [0u8; 1024 * 768];
+
+					// Then make sure we actually write something to it.
+					//
+					// If:
+					// 1. the stack area is placed at the beginning of the linear memory space, and
+					// 2. the stack pointer points to out-of-bounds area, and
+					// 3. a write is performed around the current stack pointer.
+					//
+					// then a trap should happen.
+					//
+					for (i, v) in data.iter_mut().enumerate() {
+						*v = i as u8; // deliberate truncation
+					}
+
+					if trap {
+						// There is a small chance of this to be pulled up in theory. In practice
+						// the probability of that is rather low.
+						panic!()
+					}
+
+					data.to_vec()
+				}
+
 				fn get_block_number() -> u64 {
 					system::get_block_number().expect("Block number is initialized")
 				}
@@ -715,3 +767,64 @@ cfg_if! {
 		}
 	}
 }
+
+#[cfg(test)]
+mod tests {
+	use substrate_test_runtime_client::{
+		prelude::*,
+		DefaultTestClientBuilderExt, TestClientBuilder,
+		runtime::TestAPI,
+	};
+	use runtime_primitives::{
+		generic::BlockId,
+		traits::ProvideRuntimeApi,
+	};
+	use state_machine::ExecutionStrategy;
+
+	#[test]
+	fn returns_mutable_static() {
+		let client = TestClientBuilder::new().set_execution_strategy(ExecutionStrategy::AlwaysWasm).build();
+		let runtime_api = client.runtime_api();
+		let block_id = BlockId::Number(client.info().chain.best_number);
+
+		let ret = runtime_api.returns_mutable_static(&block_id).unwrap();
+		assert_eq!(ret, 33);
+
+		// We expect that every invocation will need to return the initial
+		// value plus one. If the value increases more than that then it is
+		// a sign that the wasm runtime preserves the memory content.
+		let ret = runtime_api.returns_mutable_static(&block_id).unwrap();
+		assert_eq!(ret, 33);
+	}
+
+	// If we didn't restore the wasm instance properly, on a trap the stack pointer would not be
+	// returned to its initial value and thus the stack space is going to be leaked.
+	//
+	// See https://github.com/paritytech/substrate/issues/2967 for details
+	#[test]
+	fn restoration_of_globals() {
+		// Allocate 32 pages (of 65536 bytes) which gives the runtime 2048KB of heap to operate on
+		// (plus some additional space unused from the initial pages requested by the wasm runtime
+		// module).
+		//
+		// The fixture performs 2 allocations of 768KB and this theoretically gives 1536KB, however, due
+		// to our allocator algorithm there are inefficiencies.
+		const REQUIRED_MEMORY_PAGES: u64 = 32;
+
+		let client = TestClientBuilder::new()
+			.set_execution_strategy(ExecutionStrategy::AlwaysWasm)
+			.set_heap_pages(REQUIRED_MEMORY_PAGES)
+			.build();
+		let runtime_api = client.runtime_api();
+		let block_id = BlockId::Number(client.info().chain.best_number);
+
+		// On the first invocation we allocate approx. 768KB (75%) of stack and then trap.
+		let ret = runtime_api.allocates_huge_stack_array(&block_id, true);
+		assert!(ret.is_err());
+
+		// On the second invocation we allocate yet another 768KB (75%) of stack
+		let ret = runtime_api.allocates_huge_stack_array(&block_id, false);
+		assert!(ret.is_ok());
+	}
+
+}