From 4f057856f4a36d6088b6949a972f2186946147e9 Mon Sep 17 00:00:00 2001 From: Christoph Herzog Date: Wed, 17 May 2023 19:46:21 +0200 Subject: [PATCH] refactor!: Memory API modifications (try_clone/copy, duplicate_in_store) * Change the try_clone and try_copy methods to return a Result<_, MemoryError> This is a more informative return type than the previous Option<>, and allows handling failure cases better * Remove deprecated duplicate_in_store() method Closes #3810 --- lib/api/src/externals/memory.rs | 22 +------------------ lib/api/src/js/externals/memory.rs | 18 ++++----------- lib/api/src/js/vm.rs | 4 ++-- lib/api/src/jsc/externals/memory.rs | 19 ++++------------ lib/api/src/jsc/vm.rs | 4 ++-- lib/api/src/sys/externals/memory.rs | 20 ++++++----------- lib/api/src/sys/tunables.rs | 6 +++-- .../src/memory/fd_memory/memories.rs | 11 +++++----- lib/types/src/error.rs | 6 ++++- lib/vm/src/memory.rs | 12 +++++----- 10 files changed, 40 insertions(+), 82 deletions(-) diff --git a/lib/api/src/externals/memory.rs b/lib/api/src/externals/memory.rs index c20b7527d22..2fbbd0a35fa 100644 --- a/lib/api/src/externals/memory.rs +++ b/lib/api/src/externals/memory.rs @@ -137,9 +137,6 @@ impl Memory { self.0 .try_copy(&store) .map(|new_memory| Self::new_from_existing(new_store, new_memory.into())) - .ok_or_else(|| { - MemoryError::Generic("memory is not clonable or could not be copied".to_string()) - }) } pub(crate) fn from_vm_extern(store: &mut impl AsStoreMut, vm_extern: VMExternMemory) -> Self { @@ -152,7 +149,7 @@ impl Memory { } /// Attempts to clone this memory (if its clonable) - pub fn try_clone(&self, store: &impl AsStoreRef) -> Option { + pub fn try_clone(&self, store: &impl AsStoreRef) -> Result { self.0.try_clone(store) } @@ -172,23 +169,6 @@ impl Memory { self.0 .try_clone(&store) .map(|new_memory| Self::new_from_existing(new_store, new_memory)) - .ok_or_else(|| MemoryError::Generic("memory is not clonable".to_string())) - } - - /// Attempts to clone this memory (if its clonable) in a new store - /// (cloned memory will be shared between those that clone it) - #[deprecated = "use `share_in_store` or `copy_to_store` instead"] - pub fn duplicate_in_store( - &self, - store: &impl AsStoreRef, - new_store: &mut impl AsStoreMut, - ) -> Option { - if !self.ty(store).shared { - // We should only be able to duplicate in a new store if the memory is shared - return None; - } - #[allow(deprecated)] - self.0.duplicate_in_store(store, new_store).map(Self) } /// To `VMExtern`. diff --git a/lib/api/src/js/externals/memory.rs b/lib/api/src/js/externals/memory.rs index 3e9084521fd..30c95ddcb6a 100644 --- a/lib/api/src/js/externals/memory.rs +++ b/lib/api/src/js/externals/memory.rs @@ -142,25 +142,15 @@ impl Memory { /// Cloning memory will create another reference to the same memory that /// can be put into a new store - pub fn try_clone(&self, _store: &impl AsStoreRef) -> Option { + pub fn try_clone(&self, _store: &impl AsStoreRef) -> Result { self.handle.try_clone() } /// Copying the memory will actually copy all the bytes in the memory to /// a identical byte copy of the original that can be put into a new store - pub fn try_copy(&self, store: &impl AsStoreRef) -> Option { - self.try_clone(store).and_then(|mut mem| mem.copy().ok()) - } - - #[deprecated = "use `try_clone` and `try_copy` instead"] - pub fn duplicate_in_store( - &self, - store: &impl AsStoreRef, - new_store: &mut impl AsStoreMut, - ) -> Option { - self.try_clone(&store) - .and_then(|mut memory| memory.duplicate().ok()) - .map(|new_memory| Self::new_from_existing(new_store, new_memory.into())) + pub fn try_copy(&self, store: &impl AsStoreRef) -> Result { + let mut cloned = self.try_clone(store)?; + cloned.copy() } pub fn is_from_store(&self, _store: &impl AsStoreRef) -> bool { diff --git a/lib/api/src/js/vm.rs b/lib/api/src/js/vm.rs index 31f281ab1e0..b7cb94093d2 100644 --- a/lib/api/src/js/vm.rs +++ b/lib/api/src/js/vm.rs @@ -54,8 +54,8 @@ impl VMMemory { } /// Attempts to clone this memory (if its clonable) - pub(crate) fn try_clone(&self) -> Option { - Some(self.clone()) + pub(crate) fn try_clone(&self) -> Result { + Ok(self.clone()) } /// Copies this memory to a new memory diff --git a/lib/api/src/jsc/externals/memory.rs b/lib/api/src/jsc/externals/memory.rs index af198db92f6..38fcc952e3d 100644 --- a/lib/api/src/jsc/externals/memory.rs +++ b/lib/api/src/jsc/externals/memory.rs @@ -172,26 +172,15 @@ impl Memory { /// Cloning memory will create another reference to the same memory that /// can be put into a new store - pub fn try_clone(&self, _store: &impl AsStoreRef) -> Option { + pub fn try_clone(&self, _store: &impl AsStoreRef) -> Result { self.handle.try_clone() } /// Copying the memory will actually copy all the bytes in the memory to /// a identical byte copy of the original that can be put into a new store - pub fn try_copy(&self, store: &impl AsStoreRef) -> Option { - self.try_clone(store) - .and_then(|mut mem| mem.copy(store).ok()) - } - - #[deprecated = "use `try_clone` and `try_copy` instead"] - pub fn duplicate_in_store( - &self, - store: &impl AsStoreRef, - new_store: &mut impl AsStoreMut, - ) -> Option { - self.try_clone(&store) - .and_then(|mut memory| memory.duplicate(&store).ok()) - .map(|new_memory| Self::new_from_existing(new_store, new_memory.into())) + pub fn try_copy(&self, store: &impl AsStoreRef) -> Result { + let mut cloned = self.try_clone(store)?; + cloned.copy(store) } pub fn is_from_store(&self, _store: &impl AsStoreRef) -> bool { diff --git a/lib/api/src/jsc/vm.rs b/lib/api/src/jsc/vm.rs index 067717bd3a5..ae84b88d3dc 100644 --- a/lib/api/src/jsc/vm.rs +++ b/lib/api/src/jsc/vm.rs @@ -45,8 +45,8 @@ impl VMMemory { } /// Attempts to clone this memory (if its clonable) - pub(crate) fn try_clone(&self) -> Option { - Some(self.clone()) + pub(crate) fn try_clone(&self) -> Result { + Ok(self.clone()) } /// Copies this memory to a new memory diff --git a/lib/api/src/sys/externals/memory.rs b/lib/api/src/sys/externals/memory.rs index c830273ad4d..8890e571c8b 100644 --- a/lib/api/src/sys/externals/memory.rs +++ b/lib/api/src/sys/externals/memory.rs @@ -65,26 +65,20 @@ impl Memory { /// Cloning memory will create another reference to the same memory that /// can be put into a new store - pub fn try_clone(&self, store: &impl AsStoreRef) -> Option { + pub fn try_clone(&self, store: &impl AsStoreRef) -> Result { let mem = self.handle.get(store.as_store_ref().objects()); - mem.try_clone().map(|mem| mem.into()) + let cloned = mem.try_clone()?; + Ok(cloned.into()) } /// Copying the memory will actually copy all the bytes in the memory to /// a identical byte copy of the original that can be put into a new store - pub fn try_copy(&self, store: &impl AsStoreRef) -> Option> { - self.try_clone(store).and_then(|mut mem| mem.copy().ok()) - } - - #[deprecated = "use `try_clone` and `try_copy` instead"] - pub fn duplicate_in_store( + pub fn try_copy( &self, store: &impl AsStoreRef, - new_store: &mut impl AsStoreMut, - ) -> Option { - self.try_clone(&store) - .and_then(|mut memory| memory.copy().ok()) - .map(|new_memory| Self::new_from_existing(new_store, new_memory.into())) + ) -> Result, MemoryError> { + let mut mem = self.try_clone(store)?; + mem.copy() } /// To `VMExtern`. diff --git a/lib/api/src/sys/tunables.rs b/lib/api/src/sys/tunables.rs index 708d320c879..d5ef8b70704 100644 --- a/lib/api/src/sys/tunables.rs +++ b/lib/api/src/sys/tunables.rs @@ -120,8 +120,10 @@ mod tests { } } - fn try_clone(&self) -> Option> { - None + fn try_clone(&self) -> Result, MemoryError> { + Err(MemoryError::InvalidMemory { + reason: "VMTinyMemory can not be cloned".to_string(), + }) } fn copy(&mut self) -> Result, MemoryError> { diff --git a/lib/sys-utils/src/memory/fd_memory/memories.rs b/lib/sys-utils/src/memory/fd_memory/memories.rs index 25296d963c2..4b71a13d3aa 100644 --- a/lib/sys-utils/src/memory/fd_memory/memories.rs +++ b/lib/sys-utils/src/memory/fd_memory/memories.rs @@ -343,9 +343,8 @@ impl LinearMemory for VMOwnedMemory { } /// Owned memory can not be cloned (this will always return None) - fn try_clone(&self) -> Option> { - tracing::warn!("trying to clone owned memory"); - None + fn try_clone(&self) -> Result, MemoryError> { + Err(MemoryError::MemoryNotShared) } /// Copies this memory to a new memory @@ -440,8 +439,8 @@ impl LinearMemory for VMSharedMemory { } /// Shared memory can always be cloned - fn try_clone(&self) -> Option> { - Some(Box::new(self.clone())) + fn try_clone(&self) -> Result, MemoryError> { + Ok(Box::new(self.clone())) } /// Copies this memory to a new memory @@ -515,7 +514,7 @@ impl LinearMemory for VMMemory { } /// Attempts to clone this memory (if its clonable) - fn try_clone(&self) -> Option> { + fn try_clone(&self) -> Result, MemoryError> { self.0.try_clone() } diff --git a/lib/types/src/error.rs b/lib/types/src/error.rs index 9bebab026d8..cb207dd0c90 100644 --- a/lib/types/src/error.rs +++ b/lib/types/src/error.rs @@ -47,6 +47,7 @@ pub enum DeserializeError { /// Error type describing things that can go wrong when operating on Wasm Memories. #[derive(Error, Debug, Clone, PartialEq, Eq, Hash)] +#[non_exhaustive] pub enum MemoryError { /// Low level error with mmap. #[error("Error when allocating memory: {0}")] @@ -60,7 +61,7 @@ pub enum MemoryError { /// The attempted amount to grow by in pages. attempted_delta: Pages, }, - /// The operation would cause the size of the memory size exceed the maximum. + /// Invalid memory was provided. #[error("The memory is invalid because {}", reason)] InvalidMemory { /// The reason why the provided memory is invalid. @@ -82,6 +83,9 @@ pub enum MemoryError { /// The number of pages requested as the maximum amount of memory. max_allowed: Pages, }, + /// Returned when a shared memory is required, but the given memory is not shared. + #[error("The memory is not shared")] + MemoryNotShared, /// A user defined error value, used for error cases not listed above. #[error("A user-defined error occurred: {0}")] Generic(String), diff --git a/lib/vm/src/memory.rs b/lib/vm/src/memory.rs index e31b594c0c9..37a6551b886 100644 --- a/lib/vm/src/memory.rs +++ b/lib/vm/src/memory.rs @@ -332,8 +332,8 @@ impl LinearMemory for VMOwnedMemory { } /// Owned memory can not be cloned (this will always return None) - fn try_clone(&self) -> Option> { - None + fn try_clone(&self) -> Result, MemoryError> { + Err(MemoryError::MemoryNotShared) } /// Copies this memory to a new memory @@ -429,8 +429,8 @@ impl LinearMemory for VMSharedMemory { } /// Shared memory can always be cloned - fn try_clone(&self) -> Option> { - Some(Box::new(self.clone())) + fn try_clone(&self) -> Result, MemoryError> { + Ok(Box::new(self.clone())) } /// Copies this memory to a new memory @@ -506,7 +506,7 @@ impl LinearMemory for VMMemory { } /// Attempts to clone this memory (if its clonable) - fn try_clone(&self) -> Option> { + fn try_clone(&self) -> Result, MemoryError> { self.0.try_clone() } @@ -637,7 +637,7 @@ where fn vmmemory(&self) -> NonNull; /// Attempts to clone this memory (if its clonable) - fn try_clone(&self) -> Option>; + fn try_clone(&self) -> Result, MemoryError>; #[doc(hidden)] /// # Safety