From d7aa26371c9dee02ce3dbd861e5ab319a4aabe31 Mon Sep 17 00:00:00 2001 From: ccgauche Date: Mon, 14 Dec 2020 17:07:05 +0100 Subject: [PATCH 1/9] Fixed memory issue now using alloc --- common/sys/src/plugin/errors.rs | 7 ++++++ common/sys/src/plugin/module.rs | 38 +++++++++++++++++++++++---------- plugin/rt/src/lib.rs | 15 +++++++++++-- 3 files changed, 47 insertions(+), 13 deletions(-) diff --git a/common/sys/src/plugin/errors.rs b/common/sys/src/plugin/errors.rs index c7e686e24f..6e63172149 100644 --- a/common/sys/src/plugin/errors.rs +++ b/common/sys/src/plugin/errors.rs @@ -16,6 +16,13 @@ pub enum PluginModuleError { FunctionGet(ResolveError), Compile(wasmer_runtime::error::CompileError), Instantiate(wasmer_runtime::error::Error), + MemoryAllocation(MemoryAllocationError), RunFunction(RuntimeError), Encoding(Box), } + +#[derive(Debug)] +pub enum MemoryAllocationError { + AllocatorNotFound(ResolveError), + CantAllocate(RuntimeError), +} diff --git a/common/sys/src/plugin/module.rs b/common/sys/src/plugin/module.rs index f77c1f79d0..8920ff08b6 100644 --- a/common/sys/src/plugin/module.rs +++ b/common/sys/src/plugin/module.rs @@ -8,7 +8,7 @@ use std::{ use error::RuntimeError; use wasmer_runtime::*; -use super::errors::{PluginError, PluginModuleError}; +use super::errors::{MemoryAllocationError, PluginError, PluginModuleError}; use plugin_api::{Action, Event}; // This represent a WASM function interface @@ -60,8 +60,7 @@ impl PluginModule { Ok(e) => e, Err(e) => return Some(Err(e)), }; - let mem = instance.context().memory(0); - match execute_raw(&mem, &func, &request.bytes).map_err(PluginModuleError::RunFunction) { + match execute_raw(&instance, &func, &request.bytes).map_err(PluginModuleError::RunFunction) { Ok(e) => e, Err(e) => return Some(Err(e)), } @@ -92,22 +91,27 @@ impl PreparedEventQuery { } } -const MEMORY_POS: usize = 100000; - // This function is not public because this function should not be used without // an interface to limit unsafe behaviours #[allow(clippy::needless_range_loop)] fn execute_raw( - memory: &Memory, + instance: &Instance, function: &Function, bytes: &[u8], ) -> Result, RuntimeError> { - let view = memory.view::(); + // This reserves space for the buffer let len = bytes.len(); - for (cell, byte) in view[MEMORY_POS..len + MEMORY_POS].iter().zip(bytes.iter()) { - cell.set(*byte) - } - let start = function.call(MEMORY_POS as i32, len as u32)? as usize; + let start = { + let memory_pos = reserve_wasm_memory_buffer(len,instance).expect("Fatal error while allocating memory for a plugin! Closing server...") as usize; + let memory = instance.context().memory(0); + let view = memory.view::(); + for (cell, byte) in view[memory_pos..memory_pos+len].iter().zip(bytes.iter()) { + cell.set(*byte) + } + function.call(memory_pos as i32, len as u32)? as usize + }; + + let memory = instance.context().memory(0); let view = memory.view::(); let mut new_len_bytes = [0u8; 4]; // TODO: It is probably better to dirrectly make the new_len_bytes @@ -156,3 +160,15 @@ pub fn read_action(ctx: &mut Ctx, ptr: u32, len: u32) { } } } + +fn reserve_wasm_memory_buffer<'a>( + value: usize, + instance: &'a Instance, +) -> Result { + instance + .exports + .get::>("wasm_prepare_buffer") + .map_err(|e| MemoryAllocationError::AllocatorNotFound(e))? + .call(value as i32) + .map_err(|e| MemoryAllocationError::CantAllocate(e)) +} \ No newline at end of file diff --git a/plugin/rt/src/lib.rs b/plugin/rt/src/lib.rs index 81b3fadc65..a8dd8c9b2b 100644 --- a/plugin/rt/src/lib.rs +++ b/plugin/rt/src/lib.rs @@ -14,7 +14,7 @@ extern "C" { pub fn emit_action(action: api::Action) { emit_actions(vec![action]) } pub fn emit_actions(actions: Vec) { - let ret = bincode::serialize(&actions).unwrap(); + let ret = bincode::serialize(&actions).expect("Can't serialize action in emit"); unsafe { raw_emit_actions(ret.as_ptr(), ret.len()); } @@ -29,10 +29,21 @@ where } pub fn write_output(value: impl Serialize) -> i32 { - let ret = bincode::serialize(&value).unwrap(); + let ret = bincode::serialize(&value).expect("Can't serialize event output"); let len = ret.len() as u32; unsafe { ::std::ptr::write(1 as _, len); } ret.as_ptr() as _ } + +static mut BUFFERS: Vec = Vec::new(); + +/// Allocate buffer from wasm linear memory +#[no_mangle] +pub fn wasm_prepare_buffer(size: i32) -> i32 { + unsafe { + BUFFERS = Vec::::with_capacity(size as usize); + BUFFERS.as_ptr() as i32 + } +} From 438a0dce884d5d1266716b30fbc77ab112a60323 Mon Sep 17 00:00:00 2001 From: ccgauche Date: Mon, 14 Dec 2020 17:43:32 +0100 Subject: [PATCH 2/9] added unsafe on function --- plugin/rt/src/lib.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/plugin/rt/src/lib.rs b/plugin/rt/src/lib.rs index a8dd8c9b2b..ef7c25d0cd 100644 --- a/plugin/rt/src/lib.rs +++ b/plugin/rt/src/lib.rs @@ -41,9 +41,7 @@ static mut BUFFERS: Vec = Vec::new(); /// Allocate buffer from wasm linear memory #[no_mangle] -pub fn wasm_prepare_buffer(size: i32) -> i32 { - unsafe { - BUFFERS = Vec::::with_capacity(size as usize); - BUFFERS.as_ptr() as i32 - } +pub unsafe fn wasm_prepare_buffer(size: i32) -> i32 { + BUFFERS = Vec::::with_capacity(size as usize); + BUFFERS.as_ptr() as i32 } From 248e8f770667e2ad415a6c7b2593be4d706d1032 Mon Sep 17 00:00:00 2001 From: ccgauche Date: Mon, 14 Dec 2020 18:22:03 +0100 Subject: [PATCH 3/9] added safety section --- plugin/rt/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugin/rt/src/lib.rs b/plugin/rt/src/lib.rs index ef7c25d0cd..f010266708 100644 --- a/plugin/rt/src/lib.rs +++ b/plugin/rt/src/lib.rs @@ -40,6 +40,8 @@ pub fn write_output(value: impl Serialize) -> i32 { static mut BUFFERS: Vec = Vec::new(); /// Allocate buffer from wasm linear memory +/// # Safety +/// This function should never be used only intented to by used by the host #[no_mangle] pub unsafe fn wasm_prepare_buffer(size: i32) -> i32 { BUFFERS = Vec::::with_capacity(size as usize); From 69dba49884fcd8318167899a6a21bfc7bd6caf16 Mon Sep 17 00:00:00 2001 From: ccgauche Date: Mon, 14 Dec 2020 18:31:01 +0100 Subject: [PATCH 4/9] Fixed clippy warnings --- common/sys/src/plugin/module.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/sys/src/plugin/module.rs b/common/sys/src/plugin/module.rs index 8920ff08b6..120173b421 100644 --- a/common/sys/src/plugin/module.rs +++ b/common/sys/src/plugin/module.rs @@ -168,7 +168,7 @@ fn reserve_wasm_memory_buffer<'a>( instance .exports .get::>("wasm_prepare_buffer") - .map_err(|e| MemoryAllocationError::AllocatorNotFound(e))? + .map_err(MemoryAllocationError::AllocatorNotFound)? .call(value as i32) - .map_err(|e| MemoryAllocationError::CantAllocate(e)) + .map_err(MemoryAllocationError::CantAllocate) } \ No newline at end of file From ea1efd1ce17c8e6a67d63c1ad76432f8cb2df2f7 Mon Sep 17 00:00:00 2001 From: ccgauche Date: Mon, 14 Dec 2020 18:46:04 +0100 Subject: [PATCH 5/9] Runned FMT --- common/sys/src/plugin/module.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/common/sys/src/plugin/module.rs b/common/sys/src/plugin/module.rs index 120173b421..c0884a6723 100644 --- a/common/sys/src/plugin/module.rs +++ b/common/sys/src/plugin/module.rs @@ -60,7 +60,9 @@ impl PluginModule { Ok(e) => e, Err(e) => return Some(Err(e)), }; - match execute_raw(&instance, &func, &request.bytes).map_err(PluginModuleError::RunFunction) { + match execute_raw(&instance, &func, &request.bytes) + .map_err(PluginModuleError::RunFunction) + { Ok(e) => e, Err(e) => return Some(Err(e)), } @@ -102,15 +104,17 @@ fn execute_raw( // This reserves space for the buffer let len = bytes.len(); let start = { - let memory_pos = reserve_wasm_memory_buffer(len,instance).expect("Fatal error while allocating memory for a plugin! Closing server...") as usize; + let memory_pos = reserve_wasm_memory_buffer(len, instance) + .expect("Fatal error while allocating memory for a plugin! Closing server...") + as usize; let memory = instance.context().memory(0); let view = memory.view::(); - for (cell, byte) in view[memory_pos..memory_pos+len].iter().zip(bytes.iter()) { + for (cell, byte) in view[memory_pos..memory_pos + len].iter().zip(bytes.iter()) { cell.set(*byte) } function.call(memory_pos as i32, len as u32)? as usize }; - + let memory = instance.context().memory(0); let view = memory.view::(); let mut new_len_bytes = [0u8; 4]; @@ -171,4 +175,4 @@ fn reserve_wasm_memory_buffer<'a>( .map_err(MemoryAllocationError::AllocatorNotFound)? .call(value as i32) .map_err(MemoryAllocationError::CantAllocate) -} \ No newline at end of file +} From 7f20c78ce6163a911634044060eff5857437736e Mon Sep 17 00:00:00 2001 From: ccgauche Date: Mon, 14 Dec 2020 21:48:37 +0100 Subject: [PATCH 6/9] Updated to zesterer suggestion --- plugin/rt/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/rt/src/lib.rs b/plugin/rt/src/lib.rs index f010266708..bf7d4cdd09 100644 --- a/plugin/rt/src/lib.rs +++ b/plugin/rt/src/lib.rs @@ -44,6 +44,6 @@ static mut BUFFERS: Vec = Vec::new(); /// This function should never be used only intented to by used by the host #[no_mangle] pub unsafe fn wasm_prepare_buffer(size: i32) -> i32 { - BUFFERS = Vec::::with_capacity(size as usize); + BUFFERS = vec![0u8; size as usize]; BUFFERS.as_ptr() as i32 } From 56121cea6a116e42e3cd7b5cfa3baee3af10e98e Mon Sep 17 00:00:00 2001 From: ccgauche Date: Tue, 15 Dec 2020 09:29:57 +0100 Subject: [PATCH 7/9] Added memory caching and reallocation prevention --- hello_plugin/src/lib.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 hello_plugin/src/lib.rs diff --git a/hello_plugin/src/lib.rs b/hello_plugin/src/lib.rs new file mode 100644 index 0000000000..2de3c512ed --- /dev/null +++ b/hello_plugin/src/lib.rs @@ -0,0 +1,34 @@ +use veloren_plugin_rt::{ + api::{event::*, Action, GameMode}, + *, +}; + +#[veloren_plugin_rt::event_handler] +pub fn on_load(load: PluginLoadEvent) { + match load.game_mode { + GameMode::Server => emit_action(Action::Print("Hello, server!".to_owned())), + GameMode::Client => emit_action(Action::Print("Hello, client!".to_owned())), + GameMode::Singleplayer => emit_action(Action::Print("Hello, singleplayer!".to_owned())), + } +} + +#[event_handler] +pub fn on_command_testplugin(command: ChatCommandEvent) -> Result, String> { + Ok(vec![format!( + "Player of id {:?} sended command with args {:?}", + command.player, command.command_args + )]) +} + +#[event_handler] +pub fn on_player_join(input: PlayerJoinEvent) -> PlayerJoinResult { + emit_action(Action::PlayerSendMessage( + input.player_id, + format!("Welcome {} on our server", input.player_name), + )); + if input.player_name == "Cheater123" { + PlayerJoinResult::CloseConnection + } else { + PlayerJoinResult::None + } +} From cff08ac8aea010c1722c5c42e869854aaebd9ab4 Mon Sep 17 00:00:00 2001 From: ccgauche Date: Tue, 15 Dec 2020 16:04:14 +0100 Subject: [PATCH 8/9] Delete lib.rs --- hello_plugin/src/lib.rs | 34 ---------------------------------- 1 file changed, 34 deletions(-) delete mode 100644 hello_plugin/src/lib.rs diff --git a/hello_plugin/src/lib.rs b/hello_plugin/src/lib.rs deleted file mode 100644 index 2de3c512ed..0000000000 --- a/hello_plugin/src/lib.rs +++ /dev/null @@ -1,34 +0,0 @@ -use veloren_plugin_rt::{ - api::{event::*, Action, GameMode}, - *, -}; - -#[veloren_plugin_rt::event_handler] -pub fn on_load(load: PluginLoadEvent) { - match load.game_mode { - GameMode::Server => emit_action(Action::Print("Hello, server!".to_owned())), - GameMode::Client => emit_action(Action::Print("Hello, client!".to_owned())), - GameMode::Singleplayer => emit_action(Action::Print("Hello, singleplayer!".to_owned())), - } -} - -#[event_handler] -pub fn on_command_testplugin(command: ChatCommandEvent) -> Result, String> { - Ok(vec![format!( - "Player of id {:?} sended command with args {:?}", - command.player, command.command_args - )]) -} - -#[event_handler] -pub fn on_player_join(input: PlayerJoinEvent) -> PlayerJoinResult { - emit_action(Action::PlayerSendMessage( - input.player_id, - format!("Welcome {} on our server", input.player_name), - )); - if input.player_name == "Cheater123" { - PlayerJoinResult::CloseConnection - } else { - PlayerJoinResult::None - } -} From 2b9dee0727e7862a3aa98c1bf239943fda50d2a2 Mon Sep 17 00:00:00 2001 From: ccgauche Date: Tue, 15 Dec 2020 16:04:55 +0100 Subject: [PATCH 9/9] Updated buffer --- common/sys/src/plugin/module.rs | 66 +++++++++++++++++++++++---------- 1 file changed, 46 insertions(+), 20 deletions(-) diff --git a/common/sys/src/plugin/module.rs b/common/sys/src/plugin/module.rs index c0884a6723..1bbd9e164a 100644 --- a/common/sys/src/plugin/module.rs +++ b/common/sys/src/plugin/module.rs @@ -17,7 +17,7 @@ pub type Function<'a> = Func<'a, (i32, u32), i32>; #[derive(Clone)] // This structure represent the WASM State of the plugin. pub struct PluginModule { - wasm_instance: Arc>, + wasm_state: Arc>, events: HashSet, } @@ -30,10 +30,9 @@ impl PluginModule { "raw_emit_actions" => func!(read_action), }}) .map_err(PluginModuleError::Instantiate)?; - Ok(Self { events: instance.exports.into_iter().map(|(name, _)| name).collect(), - wasm_instance: Arc::new(Mutex::new(instance)), + wasm_state: Arc::new(Mutex::new(WasmState::new(instance))), }) } @@ -51,16 +50,8 @@ impl PluginModule { return None; } let bytes = { - let instance = self.wasm_instance.lock().unwrap(); - let func = match instance - .exports - .get(event_name) - .map_err(PluginModuleError::FunctionGet) - { - Ok(e) => e, - Err(e) => return Some(Err(e)), - }; - match execute_raw(&instance, &func, &request.bytes) + let mut state = self.wasm_state.lock().unwrap(); + match execute_raw(&mut state, event_name, &request.bytes) .map_err(PluginModuleError::RunFunction) { Ok(e) => e, @@ -71,6 +62,28 @@ impl PluginModule { } } +pub struct WasmMemoryContext { + memory_buffer_size: usize, + memory_pointer: i32, +} + +pub struct WasmState { + instance: Instance, + memory: WasmMemoryContext, +} + +impl WasmState { + fn new(instance: Instance) -> Self { + Self { + instance, + memory: WasmMemoryContext { + memory_buffer_size: 0, + memory_pointer: 0, + }, + } + } +} + // This structure represent a Pre-encoded event object (Useful to avoid // reencoding for each module in every plugin) pub struct PreparedEventQuery { @@ -97,17 +110,23 @@ impl PreparedEventQuery { // an interface to limit unsafe behaviours #[allow(clippy::needless_range_loop)] fn execute_raw( - instance: &Instance, - function: &Function, + context: &mut WasmState, + event_name: &str, bytes: &[u8], ) -> Result, RuntimeError> { // This reserves space for the buffer let len = bytes.len(); let start = { - let memory_pos = reserve_wasm_memory_buffer(len, instance) + let memory_pos = reserve_wasm_memory_buffer(len, &context.instance, &mut context.memory) .expect("Fatal error while allocating memory for a plugin! Closing server...") as usize; - let memory = instance.context().memory(0); + + let function: Func<(i32, u32), i32> = context + .instance + .exports + .get(event_name) + .expect("Function not found this should never happen"); + let memory = context.instance.context().memory(0); let view = memory.view::(); for (cell, byte) in view[memory_pos..memory_pos + len].iter().zip(bytes.iter()) { cell.set(*byte) @@ -115,7 +134,7 @@ fn execute_raw( function.call(memory_pos as i32, len as u32)? as usize }; - let memory = instance.context().memory(0); + let memory = context.instance.context().memory(0); let view = memory.view::(); let mut new_len_bytes = [0u8; 4]; // TODO: It is probably better to dirrectly make the new_len_bytes @@ -168,11 +187,18 @@ pub fn read_action(ctx: &mut Ctx, ptr: u32, len: u32) { fn reserve_wasm_memory_buffer<'a>( value: usize, instance: &'a Instance, + context: &mut WasmMemoryContext, ) -> Result { - instance + if context.memory_buffer_size >= value { + return Ok(context.memory_pointer); + } + let pointer = instance .exports .get::>("wasm_prepare_buffer") .map_err(MemoryAllocationError::AllocatorNotFound)? .call(value as i32) - .map_err(MemoryAllocationError::CantAllocate) + .map_err(MemoryAllocationError::CantAllocate)?; + context.memory_buffer_size = value; + context.memory_pointer = pointer; + Ok(pointer) }