From 91ca80de1715c81e0839e03a179e1a2ce7210c23 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Tue, 12 Nov 2024 11:46:45 +0000 Subject: [PATCH 01/14] fix: move .cargo/config to .cargo/config.toml This fixes a cargo warning printed in recent toolchains. Since we do not support toolchains older than 1.38, there is no need to symlink to the new file to the old one. Signed-off-by: Patrick Roy --- .cargo/{config => config.toml} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .cargo/{config => config.toml} (100%) diff --git a/.cargo/config b/.cargo/config.toml similarity index 100% rename from .cargo/config rename to .cargo/config.toml From f90b776e8e107ee8d1a47c50a865ee62585d3e3e Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 3 Oct 2024 09:57:41 +0100 Subject: [PATCH 02/14] fix: remove superfluous calls to `.subslice` in io.rs The call to `.subslice()` ensures that the range `[0, total]` is a valid subslice of the `buf: VolatileSlice` passed in. But `total = min(buf.len(), self.len()) <= buf.len()`, and thus `[0, total]` is definitely a valid subslice of `buf`. The `copy_{from,to}_volatile_slice` functions do not actually care about the length of the `VolatileSlice` passed in - it relies on the safety invariant to ensure that the passed slice has length at least `total`. Thus it doesn't matter if we pass a slice of length `total`, or of a length greater than `total`. It will simply access the first `total` bytes in the slice. Also clarify the safety comment, as some slightly mistakes seemingly snuck in when copying them. Signed-off-by: Patrick Roy --- src/io.rs | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/io.rs b/src/io.rs index e7932368..25fed3f7 100644 --- a/src/io.rs +++ b/src/io.rs @@ -220,16 +220,16 @@ impl WriteVolatile for &mut [u8] { buf: &VolatileSlice, ) -> Result { let total = buf.len().min(self.len()); - let src = buf.subslice(0, total)?; // SAFETY: - // We check above that `src` is contiguously allocated memory of length `total <= self.len())`. - // Furthermore, both src and dst of the call to - // copy_from_volatile_slice are valid for reads and writes respectively of length `total` - // since total is the minimum of lengths of the memory areas pointed to. The areas do not - // overlap, since `dst` is inside guest memory, and buf is a slice (no slices to guest - // memory are possible without violating rust's aliasing rules). - let written = unsafe { copy_from_volatile_slice(self.as_mut_ptr(), &src, total) }; + // `buf` is contiguously allocated memory of length `total <= buf.len())` by the invariants + // of `VolatileSlice`. + // Furthermore, both source and destination of the call to copy_from_volatile_slice are valid + // for reads and writes respectively of length `total` since total is the minimum of lengths + // of the memory areas pointed to. The areas do not overlap, since the source is inside guest + // memory, and the destination is a pointer derived from a slice (no slices to guest memory + // are possible without violating rust's aliasing rules). + let written = unsafe { copy_from_volatile_slice(self.as_mut_ptr(), buf, total) }; // Advance the slice, just like the stdlib: https://doc.rust-lang.org/src/std/io/impls.rs.html#335 *self = std::mem::take(self).split_at_mut(written).1; @@ -259,15 +259,16 @@ impl ReadVolatile for &[u8] { buf: &mut VolatileSlice, ) -> Result { let total = buf.len().min(self.len()); - let dst = buf.subslice(0, total)?; // SAFETY: - // We check above that `dst` is contiguously allocated memory of length `total <= self.len())`. - // Furthermore, both src and dst of the call to copy_to_volatile_slice are valid for reads - // and writes respectively of length `total` since total is the minimum of lengths of the - // memory areas pointed to. The areas do not overlap, since `dst` is inside guest memory, - // and buf is a slice (no slices to guest memory are possible without violating rust's aliasing rules). - let read = unsafe { copy_to_volatile_slice(&dst, self.as_ptr(), total) }; + // `buf` is contiguously allocated memory of length `total <= buf.len())` by the invariants + // of `VolatileSlice`. + // Furthermore, both source and destination of the call to copy_to_volatile_slice are valid + // for reads and writes respectively of length `total` since total is the minimum of lengths + // of the memory areas pointed to. The areas do not overlap, since the destination is inside + // guest memory, and the source is a pointer derived from a slice (no slices to guest memory + // are possible without violating rust's aliasing rules). + let read = unsafe { copy_to_volatile_slice(buf, self.as_ptr(), total) }; // Advance the slice, just like the stdlib: https://doc.rust-lang.org/src/std/io/impls.rs.html#232-310 *self = self.split_at(read).1; From 16df883690a3d05be2c4c1e2b3d3597f2b5cb99f Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Tue, 12 Nov 2024 07:36:20 +0000 Subject: [PATCH 03/14] Clarify behavior of `VolatileMemory::compute_end_offset` Fix the doc comment, and add a unit test for this functions. Particularly the "allow length 0 accesses at the end of the slice" behavior was undocumented before. Signed-off-by: Patrick Roy --- src/volatile_memory.rs | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/volatile_memory.rs b/src/volatile_memory.rs index 7dbbd7c7..1f739823 100644 --- a/src/volatile_memory.rs +++ b/src/volatile_memory.rs @@ -276,7 +276,18 @@ pub trait VolatileMemory { unsafe { Ok(&*(slice.addr as *const T)) } } - /// Returns the sum of `base` and `offset` if the resulting address is valid. + /// Returns the sum of `base` and `offset` if it is valid to access a range of `offset` + /// bytes starting at `base`. + /// + /// Specifically, allows accesses of length 0 at the end of a slice: + /// + /// ```rust + /// # use vm_memory::{VolatileMemory, VolatileSlice}; + /// let mut arr = [1, 2, 3]; + /// let slice = VolatileSlice::from(arr.as_mut_slice()); + /// + /// assert_eq!(slice.compute_end_offset(3, 0).unwrap(), 3); + /// ``` fn compute_end_offset(&self, base: usize, offset: usize) -> Result { let mem_end = compute_offset(base, offset)?; if mem_end > self.len() { @@ -1662,6 +1673,28 @@ mod tests { const DEFAULT_PAGE_SIZE: NonZeroUsize = unsafe { NonZeroUsize::new_unchecked(0x1000) }; + #[test] + fn test_compute_end_offset() { + let mut array = [1, 2, 3, 4, 5]; + let slice = VolatileSlice::from(array.as_mut_slice()); + + // Iterate over all valid ranges, assert that they pass validation. + // This includes edge cases such as len = 0 and base = 5! + for len in 0..slice.len() { + for base in 0..=slice.len() - len { + assert_eq!( + slice.compute_end_offset(base, len).unwrap(), + len + base, + "compute_end_offset rejected valid base/offset pair {base} + {len}" + ); + } + } + + // Check invalid configurations + slice.compute_end_offset(5, 1).unwrap_err(); + slice.compute_end_offset(6, 0).unwrap_err(); + } + #[test] fn test_display_error() { assert_eq!( From e35402316a363bc63ef136919bbf643cae0e3fda Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Tue, 12 Nov 2024 07:39:20 +0000 Subject: [PATCH 04/14] Drop assert from `read/write_volatile` in `GuestMemory` The comment on the assertion was wrong, as we are not even doing anything unsafe in the first place. Additionally, the `offset` variable is unused by these functions, so the assertion is at best a sanity check that the `try_access` implementation is correct, although I don't particularly see the value of that. Remove the assertion to prevent confusion. Signed-off-by: Patrick Roy --- src/guest_memory.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/guest_memory.rs b/src/guest_memory.rs index 8da6ec39..9818d6ce 100644 --- a/src/guest_memory.rs +++ b/src/guest_memory.rs @@ -681,10 +681,7 @@ pub trait GuestMemory { where F: ReadVolatile, { - self.try_access(count, addr, |offset, len, caddr, region| -> Result { - // Check if something bad happened before doing unsafe things. - assert!(offset <= count); - + self.try_access(count, addr, |_, len, caddr, region| -> Result { let mut vslice = region.get_slice(caddr, len)?; src.read_volatile(&mut vslice) @@ -704,10 +701,7 @@ pub trait GuestMemory { where F: WriteVolatile, { - self.try_access(count, addr, |offset, len, caddr, region| -> Result { - // Check if something bad happened before doing unsafe things. - assert!(offset <= count); - + self.try_access(count, addr, |_, len, caddr, region| -> Result { let vslice = region.get_slice(caddr, len)?; // For a non-RAM region, reading could have side effects, so we From fe69c98228b764b0cd549b04be56f2eea4e227a1 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Tue, 12 Nov 2024 07:47:04 +0000 Subject: [PATCH 05/14] Implement `VolatileMemory::get_slice` in terms of `subslice` The two functions contain exactly the same body, so just have one call the other. No functional change intended. Signed-off-by: Patrick Roy --- src/volatile_memory.rs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/src/volatile_memory.rs b/src/volatile_memory.rs index 1f739823..be0cd0af 100644 --- a/src/volatile_memory.rs +++ b/src/volatile_memory.rs @@ -1019,19 +1019,7 @@ impl VolatileMemory for VolatileSlice<'_, B> { } fn get_slice(&self, offset: usize, count: usize) -> Result> { - let _ = self.compute_end_offset(offset, count)?; - Ok( - // SAFETY: This is safe because the pointer is range-checked by compute_end_offset, and - // the lifetime is the same as self. - unsafe { - VolatileSlice::with_bitmap( - self.addr.add(offset), - count, - self.bitmap.slice_at(offset), - self.mmap, - ) - }, - ) + self.subslice(offset, count) } } From 8c916d9bc961ffebb68736b426e243ef4bd67f3f Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Tue, 12 Nov 2024 10:13:40 +0000 Subject: [PATCH 06/14] add `retry_eintr!` utility macro Add a macro that automatically retries a given I/O operation if EINTR is returned. This is a fairly common pattern in vm-memory. Signed-off-by: Patrick Roy --- src/io.rs | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/io.rs b/src/io.rs index 25fed3f7..5b9517ed 100644 --- a/src/io.rs +++ b/src/io.rs @@ -14,6 +14,24 @@ use std::io::Stdout; #[cfg(feature = "rawfd")] use std::os::fd::AsRawFd; +macro_rules! retry_eintr { + ($io_call: expr) => { + loop { + let r = $io_call; + + if let Err(crate::VolatileMemoryError::IOError(ref err)) = r { + if err.kind() == std::io::ErrorKind::Interrupted { + continue; + } + } + + break r; + } + }; +} + +pub(crate) use retry_eintr; + /// A version of the standard library's [`Read`](std::io::Read) trait that operates on volatile /// memory instead of slices /// @@ -44,10 +62,7 @@ pub trait ReadVolatile { let mut partial_buf = buf.offset(0)?; while !partial_buf.is_empty() { - match self.read_volatile(&mut partial_buf) { - Err(VolatileMemoryError::IOError(err)) if err.kind() == ErrorKind::Interrupted => { - continue - } + match retry_eintr!(self.read_volatile(&mut partial_buf)) { Ok(0) => { return Err(VolatileMemoryError::IOError(std::io::Error::new( ErrorKind::UnexpectedEof, @@ -93,10 +108,7 @@ pub trait WriteVolatile { let mut partial_buf = buf.offset(0)?; while !partial_buf.is_empty() { - match self.write_volatile(&partial_buf) { - Err(VolatileMemoryError::IOError(err)) if err.kind() == ErrorKind::Interrupted => { - continue - } + match retry_eintr!(self.write_volatile(&partial_buf)) { Ok(0) => { return Err(VolatileMemoryError::IOError(std::io::Error::new( ErrorKind::WriteZero, From c90fdadd02ee6a73c2749ff2aab957738b8ac694 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 3 Oct 2024 07:32:04 +0100 Subject: [PATCH 07/14] chore: remove deprecated methods The old `Read`/`Write` based APIs for operating on guest memory have been deprecated for a fairly long time now, and are superseceded by their `ReadVolatile`/`WriteVolatile` equivalents. The `fold` and friends functions have been deprecated for way longer. Various `.as_ptr` APIs in volatile_memory.rs are deprecated in favor of pointer guards. Let's clean up the crate and remove all of these. Signed-off-by: Patrick Roy --- CHANGELOG.md | 7 ++ src/bitmap/mod.rs | 21 ---- src/bytes.rs | 95 --------------- src/guest_memory.rs | 269 +---------------------------------------- src/mmap.rs | 186 +--------------------------- src/volatile_memory.rs | 231 +---------------------------------- 6 files changed, 10 insertions(+), 799 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 252f85dc..0d114741 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ - \[[#311](https://github.com/rust-vmm/vm-memory/pull/311)\] Allow compiling without the ReadVolatile and WriteVolatile implementations +### Removed + +- \[[#307](https://github.com/rust-vmm/vm-memory/pull/304)\] Remove deprecated functions `Bytes::read_from`, `Bytes::read_exact_from`, + `Bytes::write_to`, `Bytes::write_all_to`, `GuestMemory::as_slice`, `GuestMemory::as_slice_mut`, `GuestMemory::with_regions`, + `GuestMemory::with_regions_mut`, `GuestMemory::map_and_fold`, `VolatileSlice::as_ptr`, `VolatileRef::as_ptr`, and + `VolatileArrayRef::as_ptr`. + ## \[v0.16.1\] ### Added diff --git a/src/bitmap/mod.rs b/src/bitmap/mod.rs index 1f3acc3e..6269895b 100644 --- a/src/bitmap/mod.rs +++ b/src/bitmap/mod.rs @@ -109,7 +109,6 @@ pub type MS<'a, M> = BS<'a, <::R as GuestMemoryRegion>::B>; pub(crate) mod tests { use super::*; - use std::io::Cursor; use std::marker::PhantomData; use std::mem::size_of_val; use std::result::Result; @@ -264,26 +263,6 @@ pub(crate) mod tests { .unwrap(); dirty_offset += step; - // Test `read_from`. - #[allow(deprecated)] // test of deprecated functions - h.test_access(bytes, dirty_offset, BUF_SIZE, |m, addr| { - assert_eq!( - m.read_from(addr, &mut Cursor::new(&buf), BUF_SIZE).unwrap(), - BUF_SIZE - ) - }) - .unwrap(); - dirty_offset += step; - - // Test `read_exact_from`. - #[allow(deprecated)] // test of deprecated functions - h.test_access(bytes, dirty_offset, BUF_SIZE, |m, addr| { - m.read_exact_from(addr, &mut Cursor::new(&buf), BUF_SIZE) - .unwrap() - }) - .unwrap(); - dirty_offset += step; - // Test `store`. h.test_access(bytes, dirty_offset, size_of_val(&val), |m, addr| { m.store(val, addr, Ordering::Relaxed).unwrap() diff --git a/src/bytes.rs b/src/bytes.rs index 6274c3a9..b5d51ff7 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -11,7 +11,6 @@ //! Define the `ByteValued` trait to mark that it is safe to instantiate the struct with random //! data. -use std::io::{Read, Write}; use std::mem::{size_of, MaybeUninit}; use std::result::Result; use std::slice::{from_raw_parts, from_raw_parts_mut}; @@ -277,72 +276,6 @@ pub trait Bytes { self.read_slice(result.as_mut_slice(), addr).map(|_| result) } - /// Reads up to `count` bytes from an object and writes them into the container at `addr`. - /// - /// Returns the number of bytes written into the container. - /// - /// # Arguments - /// * `addr` - Begin writing at this address. - /// * `src` - Copy from `src` into the container. - /// * `count` - Copy `count` bytes from `src` into the container. - #[deprecated( - note = "Use `.read_volatile_from` or the functions of the `ReadVolatile` trait instead" - )] - fn read_from(&self, addr: A, src: &mut F, count: usize) -> Result - where - F: Read; - - /// Reads exactly `count` bytes from an object and writes them into the container at `addr`. - /// - /// # Errors - /// - /// Returns an error if `count` bytes couldn't have been copied from `src` to the container. - /// Part of the data may have been copied nevertheless. - /// - /// # Arguments - /// * `addr` - Begin writing at this address. - /// * `src` - Copy from `src` into the container. - /// * `count` - Copy exactly `count` bytes from `src` into the container. - #[deprecated( - note = "Use `.read_exact_volatile_from` or the functions of the `ReadVolatile` trait instead" - )] - fn read_exact_from(&self, addr: A, src: &mut F, count: usize) -> Result<(), Self::E> - where - F: Read; - - /// Reads up to `count` bytes from the container at `addr` and writes them it into an object. - /// - /// Returns the number of bytes written into the object. - /// - /// # Arguments - /// * `addr` - Begin reading from this address. - /// * `dst` - Copy from the container to `dst`. - /// * `count` - Copy `count` bytes from the container to `dst`. - #[deprecated( - note = "Use `.write_volatile_to` or the functions of the `WriteVolatile` trait instead" - )] - fn write_to(&self, addr: A, dst: &mut F, count: usize) -> Result - where - F: Write; - - /// Reads exactly `count` bytes from the container at `addr` and writes them into an object. - /// - /// # Errors - /// - /// Returns an error if `count` bytes couldn't have been copied from the container to `dst`. - /// Part of the data may have been copied nevertheless. - /// - /// # Arguments - /// * `addr` - Begin reading from this address. - /// * `dst` - Copy from the container to `dst`. - /// * `count` - Copy exactly `count` bytes from the container to `dst`. - #[deprecated( - note = "Use `.write_all_volatile_to` or the functions of the `WriteVolatile` trait instead" - )] - fn write_all_to(&self, addr: A, dst: &mut F, count: usize) -> Result<(), Self::E> - where - F: Write; - /// Atomically store a value at the specified address. fn store(&self, val: T, addr: A, order: Ordering) -> Result<(), Self::E>; @@ -481,34 +414,6 @@ pub(crate) mod tests { Ok(()) } - fn read_from(&self, _: usize, _: &mut F, _: usize) -> Result - where - F: Read, - { - unimplemented!() - } - - fn read_exact_from(&self, _: usize, _: &mut F, _: usize) -> Result<(), Self::E> - where - F: Read, - { - unimplemented!() - } - - fn write_to(&self, _: usize, _: &mut F, _: usize) -> Result - where - F: Write, - { - unimplemented!() - } - - fn write_all_to(&self, _: usize, _: &mut F, _: usize) -> Result<(), Self::E> - where - F: Write, - { - unimplemented!() - } - fn store( &self, _val: T, diff --git a/src/guest_memory.rs b/src/guest_memory.rs index 9818d6ce..865f9c52 100644 --- a/src/guest_memory.rs +++ b/src/guest_memory.rs @@ -43,7 +43,7 @@ use std::convert::From; use std::fs::File; -use std::io::{self, Read, Write}; +use std::io; use std::ops::{BitAnd, BitOr, Deref}; use std::rc::Rc; use std::sync::atomic::Ordering; @@ -56,8 +56,6 @@ use crate::io::{ReadVolatile, WriteVolatile}; use crate::volatile_memory::{self, VolatileSlice}; use crate::GuestMemoryError; -static MAX_ACCESS_CHUNK: usize = 4096; - /// Errors associated with handling guest memory accesses. #[allow(missing_docs)] #[derive(Debug, thiserror::Error)] @@ -233,34 +231,6 @@ pub trait GuestMemoryRegion: Bytes { None } - /// Returns a slice corresponding to the data in the region. - /// - /// Returns `None` if the region does not support slice-based access. - /// - /// # Safety - /// - /// Unsafe because of possible aliasing. - #[deprecated = "It is impossible to use this function for accessing memory of a running virtual \ - machine without violating aliasing rules "] - unsafe fn as_slice(&self) -> Option<&[u8]> { - None - } - - /// Returns a mutable slice corresponding to the data in the region. - /// - /// Returns `None` if the region does not support slice-based access. - /// - /// # Safety - /// - /// Unsafe because of possible aliasing. Mutable accesses performed through the - /// returned slice are not visible to the dirty bitmap tracking functionality of - /// the region, and must be manually recorded using the associated bitmap object. - #[deprecated = "It is impossible to use this function for accessing memory of a running virtual \ - machine without violating aliasing rules "] - unsafe fn as_mut_slice(&self) -> Option<&mut [u8]> { - None - } - /// Returns a [`VolatileSlice`](struct.VolatileSlice.html) of `count` bytes starting at /// `offset`. #[allow(unused_variables)] @@ -441,34 +411,6 @@ pub trait GuestMemory { /// Returns the region containing the specified address or `None`. fn find_region(&self, addr: GuestAddress) -> Option<&Self::R>; - /// Perform the specified action on each region. - /// - /// It only walks children of current region and does not step into sub regions. - #[deprecated(since = "0.6.0", note = "Use `.iter()` instead")] - fn with_regions(&self, cb: F) -> std::result::Result<(), E> - where - F: Fn(usize, &Self::R) -> std::result::Result<(), E>, - { - for (index, region) in self.iter().enumerate() { - cb(index, region)?; - } - Ok(()) - } - - /// Perform the specified action on each region mutably. - /// - /// It only walks children of current region and does not step into sub regions. - #[deprecated(since = "0.6.0", note = "Use `.iter()` instead")] - fn with_regions_mut(&self, mut cb: F) -> std::result::Result<(), E> - where - F: FnMut(usize, &Self::R) -> std::result::Result<(), E>, - { - for (index, region) in self.iter().enumerate() { - cb(index, region)?; - } - Ok(()) - } - /// Gets an iterator over the entries in the collection. /// /// # Examples @@ -496,46 +438,6 @@ pub trait GuestMemory { /// ``` fn iter(&self) -> impl Iterator; - /// Applies two functions, specified as callbacks, on the inner memory regions. - /// - /// # Arguments - /// * `init` - Starting value of the accumulator for the `foldf` function. - /// * `mapf` - "Map" function, applied to all the inner memory regions. It returns an array of - /// the same size as the memory regions array, containing the function's results - /// for each region. - /// * `foldf` - "Fold" function, applied to the array returned by `mapf`. It acts as an - /// operator, applying itself to the `init` value and to each subsequent elemnent - /// in the array returned by `mapf`. - /// - /// # Examples - /// - /// * Compute the total size of all memory mappings in KB by iterating over the memory regions - /// and dividing their sizes to 1024, then summing up the values in an accumulator. (uses the - /// `backend-mmap` feature) - /// - /// ``` - /// # #[cfg(feature = "backend-mmap")] - /// # { - /// # use vm_memory::{GuestAddress, GuestMemory, GuestMemoryRegion, GuestMemoryMmap}; - /// # - /// let start_addr1 = GuestAddress(0x0); - /// let start_addr2 = GuestAddress(0x400); - /// let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr1, 1024), (start_addr2, 2048)]) - /// .expect("Could not create guest memory"); - /// - /// let total_size = gm.map_and_fold(0, |(_, region)| region.len() / 1024, |acc, size| acc + size); - /// assert_eq!(3, total_size) - /// # } - /// ``` - #[deprecated(since = "0.6.0", note = "Use `.iter()` instead")] - fn map_and_fold(&self, init: T, mapf: F, foldf: G) -> T - where - F: Fn((usize, &Self::R)) -> T, - G: Fn(T, T) -> T, - { - self.iter().enumerate().map(mapf).fold(init, foldf) - } - /// Returns the maximum (inclusive) address managed by the /// [`GuestMemory`](trait.GuestMemory.html). /// @@ -894,175 +796,6 @@ impl Bytes for T { Ok(()) } - /// # Examples - /// - /// * Read bytes from /dev/urandom (uses the `backend-mmap` feature) - /// - /// ``` - /// # #[cfg(feature = "backend-mmap")] - /// # { - /// # use vm_memory::{Address, Bytes, GuestAddress, GuestMemoryMmap}; - /// # use std::fs::File; - /// # use std::path::Path; - /// # - /// # let start_addr = GuestAddress(0x1000); - /// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 0x400)]) - /// # .expect("Could not create guest memory"); - /// # let addr = GuestAddress(0x1010); - /// # let mut file = if cfg!(unix) { - /// let mut file = File::open(Path::new("/dev/urandom")).expect("Could not open /dev/urandom"); - /// # file - /// # } else { - /// # File::open(Path::new("c:\\Windows\\system32\\ntoskrnl.exe")) - /// # .expect("Could not open c:\\Windows\\system32\\ntoskrnl.exe") - /// # }; - /// - /// gm.read_from(addr, &mut file, 128) - /// .expect("Could not read from /dev/urandom into guest memory"); - /// - /// let read_addr = addr.checked_add(8).expect("Could not compute read address"); - /// let rand_val: u32 = gm - /// .read_obj(read_addr) - /// .expect("Could not read u32 val from /dev/urandom"); - /// # } - /// ``` - fn read_from(&self, addr: GuestAddress, src: &mut F, count: usize) -> Result - where - F: Read, - { - self.try_access(count, addr, |offset, len, caddr, region| -> Result { - // Check if something bad happened before doing unsafe things. - assert!(offset <= count); - - let len = std::cmp::min(len, MAX_ACCESS_CHUNK); - let mut buf = vec![0u8; len].into_boxed_slice(); - - loop { - match src.read(&mut buf[..]) { - Ok(bytes_read) => { - // We don't need to update the dirty bitmap manually here because it's - // expected to be handled by the logic within the `Bytes` - // implementation for the region object. - let bytes_written = region.write(&buf[0..bytes_read], caddr)?; - assert_eq!(bytes_written, bytes_read); - break Ok(bytes_read); - } - Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => continue, - Err(e) => break Err(Error::IOError(e)), - } - } - }) - } - - fn read_exact_from(&self, addr: GuestAddress, src: &mut F, count: usize) -> Result<()> - where - F: Read, - { - #[allow(deprecated)] // this function itself is deprecated - let res = self.read_from(addr, src, count)?; - if res != count { - return Err(Error::PartialBuffer { - expected: count, - completed: res, - }); - } - Ok(()) - } - - /// # Examples - /// - /// * Write 128 bytes to /dev/null (uses the `backend-mmap` feature) - /// - /// ``` - /// # #[cfg(not(unix))] - /// # extern crate vmm_sys_util; - /// # #[cfg(feature = "backend-mmap")] - /// # { - /// # use vm_memory::{Bytes, GuestAddress, GuestMemoryMmap}; - /// # - /// # let start_addr = GuestAddress(0x1000); - /// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 1024)]) - /// # .expect("Could not create guest memory"); - /// # let mut file = if cfg!(unix) { - /// # use std::fs::OpenOptions; - /// let mut file = OpenOptions::new() - /// .write(true) - /// .open("/dev/null") - /// .expect("Could not open /dev/null"); - /// # file - /// # } else { - /// # use vmm_sys_util::tempfile::TempFile; - /// # TempFile::new().unwrap().into_file() - /// # }; - /// - /// gm.write_to(start_addr, &mut file, 128) - /// .expect("Could not write 128 bytes to the provided address"); - /// # } - /// ``` - fn write_to(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result - where - F: Write, - { - self.try_access(count, addr, |offset, len, caddr, region| -> Result { - // Check if something bad happened before doing unsafe things. - assert!(offset <= count); - - let len = std::cmp::min(len, MAX_ACCESS_CHUNK); - let mut buf = vec![0u8; len].into_boxed_slice(); - let bytes_read = region.read(&mut buf, caddr)?; - assert_eq!(bytes_read, len); - // For a non-RAM region, reading could have side effects, so we - // must use write_all(). - dst.write_all(&buf).map_err(Error::IOError)?; - Ok(len) - }) - } - - /// # Examples - /// - /// * Write 128 bytes to /dev/null (uses the `backend-mmap` feature) - /// - /// ``` - /// # #[cfg(not(unix))] - /// # extern crate vmm_sys_util; - /// # #[cfg(feature = "backend-mmap")] - /// # { - /// # use vm_memory::{Bytes, GuestAddress, GuestMemoryMmap}; - /// # - /// # let start_addr = GuestAddress(0x1000); - /// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 1024)]) - /// # .expect("Could not create guest memory"); - /// # let mut file = if cfg!(unix) { - /// # use std::fs::OpenOptions; - /// let mut file = OpenOptions::new() - /// .write(true) - /// .open("/dev/null") - /// .expect("Could not open /dev/null"); - /// # file - /// # } else { - /// # use vmm_sys_util::tempfile::TempFile; - /// # TempFile::new().unwrap().into_file() - /// # }; - /// - /// gm.write_all_to(start_addr, &mut file, 128) - /// .expect("Could not write 128 bytes to the provided address"); - /// # } - /// ``` - fn write_all_to(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result<()> - where - F: Write, - { - #[allow(deprecated)] // this function itself is deprecated - let res = self.write_to(addr, dst, count)?; - if res != count { - return Err(Error::PartialBuffer { - expected: count, - completed: res, - }); - } - Ok(()) - } - fn store(&self, val: O, addr: GuestAddress, order: Ordering) -> Result<()> { // `find_region` should really do what `to_region_addr` is doing right now, except // it should keep returning a `Result`. diff --git a/src/mmap.rs b/src/mmap.rs index 4e5122bc..bc821d41 100644 --- a/src/mmap.rs +++ b/src/mmap.rs @@ -13,7 +13,6 @@ //! This implementation is mmap-ing the memory of the guest into the current process. use std::borrow::Borrow; -use std::io::{Read, Write}; #[cfg(unix)] use std::io::{Seek, SeekFrom}; use std::ops::Deref; @@ -233,190 +232,6 @@ impl Bytes for GuestRegionMmap { .map_err(Into::into) } - /// # Examples - /// - /// * Read bytes from /dev/urandom - /// - /// ``` - /// # use vm_memory::{Address, Bytes, GuestAddress, GuestMemoryMmap}; - /// # use std::fs::File; - /// # use std::path::Path; - /// # - /// # let start_addr = GuestAddress(0x1000); - /// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 0x400)]) - /// # .expect("Could not create guest memory"); - /// # let addr = GuestAddress(0x1010); - /// # let mut file = if cfg!(unix) { - /// let mut file = File::open(Path::new("/dev/urandom")).expect("Could not open /dev/urandom"); - /// # file - /// # } else { - /// # File::open(Path::new("c:\\Windows\\system32\\ntoskrnl.exe")) - /// # .expect("Could not open c:\\Windows\\system32\\ntoskrnl.exe") - /// # }; - /// - /// gm.read_from(addr, &mut file, 128) - /// .expect("Could not read from /dev/urandom into guest memory"); - /// - /// let read_addr = addr.checked_add(8).expect("Could not compute read address"); - /// let rand_val: u32 = gm - /// .read_obj(read_addr) - /// .expect("Could not read u32 val from /dev/urandom"); - /// ``` - fn read_from( - &self, - addr: MemoryRegionAddress, - src: &mut F, - count: usize, - ) -> guest_memory::Result - where - F: Read, - { - let maddr = addr.raw_value() as usize; - #[allow(deprecated)] // function itself is deprecated - self.as_volatile_slice() - .unwrap() - .read_from::(maddr, src, count) - .map_err(Into::into) - } - - /// # Examples - /// - /// * Read bytes from /dev/urandom - /// - /// ``` - /// # use vm_memory::{Address, Bytes, GuestAddress, GuestMemoryMmap}; - /// # use std::fs::File; - /// # use std::path::Path; - /// # - /// # let start_addr = GuestAddress(0x1000); - /// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 0x400)]) - /// # .expect("Could not create guest memory"); - /// # let addr = GuestAddress(0x1010); - /// # let mut file = if cfg!(unix) { - /// let mut file = File::open(Path::new("/dev/urandom")).expect("Could not open /dev/urandom"); - /// # file - /// # } else { - /// # File::open(Path::new("c:\\Windows\\system32\\ntoskrnl.exe")) - /// # .expect("Could not open c:\\Windows\\system32\\ntoskrnl.exe") - /// # }; - /// - /// gm.read_exact_from(addr, &mut file, 128) - /// .expect("Could not read from /dev/urandom into guest memory"); - /// - /// let read_addr = addr.checked_add(8).expect("Could not compute read address"); - /// let rand_val: u32 = gm - /// .read_obj(read_addr) - /// .expect("Could not read u32 val from /dev/urandom"); - /// ``` - fn read_exact_from( - &self, - addr: MemoryRegionAddress, - src: &mut F, - count: usize, - ) -> guest_memory::Result<()> - where - F: Read, - { - let maddr = addr.raw_value() as usize; - #[allow(deprecated)] // function itself is deprecated - self.as_volatile_slice() - .unwrap() - .read_exact_from::(maddr, src, count) - .map_err(Into::into) - } - - /// Writes data from the region to a writable object. - /// - /// # Examples - /// - /// * Write 128 bytes to a /dev/null file - /// - /// ``` - /// # #[cfg(not(unix))] - /// # extern crate vmm_sys_util; - /// # use vm_memory::{Address, Bytes, GuestAddress, GuestMemoryMmap}; - /// # - /// # let start_addr = GuestAddress(0x1000); - /// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 0x400)]) - /// # .expect("Could not create guest memory"); - /// # let mut file = if cfg!(unix) { - /// # use std::fs::OpenOptions; - /// let mut file = OpenOptions::new() - /// .write(true) - /// .open("/dev/null") - /// .expect("Could not open /dev/null"); - /// # file - /// # } else { - /// # use vmm_sys_util::tempfile::TempFile; - /// # TempFile::new().unwrap().into_file() - /// # }; - /// - /// gm.write_to(start_addr, &mut file, 128) - /// .expect("Could not write to file from guest memory"); - /// ``` - fn write_to( - &self, - addr: MemoryRegionAddress, - dst: &mut F, - count: usize, - ) -> guest_memory::Result - where - F: Write, - { - let maddr = addr.raw_value() as usize; - #[allow(deprecated)] // function itself is deprecated - self.as_volatile_slice() - .unwrap() - .write_to::(maddr, dst, count) - .map_err(Into::into) - } - - /// Writes data from the region to a writable object. - /// - /// # Examples - /// - /// * Write 128 bytes to a /dev/null file - /// - /// ``` - /// # #[cfg(not(unix))] - /// # extern crate vmm_sys_util; - /// # use vm_memory::{Address, Bytes, GuestAddress, GuestMemoryMmap}; - /// # - /// # let start_addr = GuestAddress(0x1000); - /// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 0x400)]) - /// # .expect("Could not create guest memory"); - /// # let mut file = if cfg!(unix) { - /// # use std::fs::OpenOptions; - /// let mut file = OpenOptions::new() - /// .write(true) - /// .open("/dev/null") - /// .expect("Could not open /dev/null"); - /// # file - /// # } else { - /// # use vmm_sys_util::tempfile::TempFile; - /// # TempFile::new().unwrap().into_file() - /// # }; - /// - /// gm.write_all_to(start_addr, &mut file, 128) - /// .expect("Could not write to file from guest memory"); - /// ``` - fn write_all_to( - &self, - addr: MemoryRegionAddress, - dst: &mut F, - count: usize, - ) -> guest_memory::Result<()> - where - F: Write, - { - let maddr = addr.raw_value() as usize; - #[allow(deprecated)] // function itself is deprecated - self.as_volatile_slice() - .unwrap() - .write_all_to::(maddr, dst, count) - .map_err(Into::into) - } - fn store( &self, val: T, @@ -648,6 +463,7 @@ mod tests { use std::mem; #[cfg(feature = "rawfd")] use std::{fs::File, path::Path}; + use std::io::Write; use vmm_sys_util::tempfile::TempFile; type GuestMemoryMmap = super::GuestMemoryMmap<()>; diff --git a/src/volatile_memory.rs b/src/volatile_memory.rs index be0cd0af..989c79e1 100644 --- a/src/volatile_memory.rs +++ b/src/volatile_memory.rs @@ -27,7 +27,7 @@ //! not reordered or elided the access. use std::cmp::min; -use std::io::{self, Read, Write}; +use std::io; use std::marker::PhantomData; use std::mem::{align_of, size_of}; use std::ptr::copy; @@ -435,18 +435,6 @@ impl<'a, B: BitmapSlice> VolatileSlice<'a, B> { } } - /// Returns a pointer to the beginning of the slice. Mutable accesses performed - /// using the resulting pointer are not automatically accounted for by the dirty bitmap - /// tracking functionality. - #[deprecated( - since = "0.12.1", - note = "Use `.ptr_guard()` or `.ptr_guard_mut()` instead" - )] - #[cfg(not(all(feature = "xen", unix)))] - pub fn as_ptr(&self) -> *mut u8 { - self.addr - } - /// Returns a guard for the pointer to the underlying memory. pub fn ptr_guard(&self) -> PtrGuard { PtrGuard::read(self.mmap, self.addr, self.len()) @@ -805,199 +793,6 @@ impl Bytes for VolatileSlice<'_, B> { Ok(()) } - /// # Examples - /// - /// * Read bytes from /dev/urandom - /// - /// ``` - /// # use vm_memory::{Bytes, VolatileMemory, VolatileSlice}; - /// # use std::fs::File; - /// # use std::path::Path; - /// # - /// # if cfg!(unix) { - /// # let mut mem = [0u8; 1024]; - /// # let vslice = VolatileSlice::from(&mut mem[..]); - /// let mut file = File::open(Path::new("/dev/urandom")).expect("Could not open /dev/urandom"); - /// - /// vslice - /// .read_from(32, &mut file, 128) - /// .expect("Could not read bytes from file into VolatileSlice"); - /// - /// let rand_val: u32 = vslice - /// .read_obj(40) - /// .expect("Could not read value from VolatileSlice"); - /// # } - /// ``` - fn read_from(&self, addr: usize, src: &mut F, count: usize) -> Result - where - F: Read, - { - let _ = self.compute_end_offset(addr, count)?; - - let mut dst = vec![0; count]; - - let bytes_read = loop { - match src.read(&mut dst) { - Ok(n) => break n, - Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => continue, - Err(e) => return Err(Error::IOError(e)), - } - }; - - // There is no guarantee that the read implementation is well-behaved, see the docs for - // Read::read. - assert!(bytes_read <= count); - - let slice = self.subslice(addr, bytes_read)?; - - // SAFETY: We have checked via compute_end_offset that accessing the specified - // region of guest memory is valid. We asserted that the value returned by `read` is between - // 0 and count (the length of the buffer passed to it), and that the - // regions don't overlap because we allocated the Vec outside of guest memory. - Ok(unsafe { copy_to_volatile_slice(&slice, dst.as_ptr(), bytes_read) }) - } - - /// # Examples - /// - /// * Read bytes from /dev/urandom - /// - /// ``` - /// # use vm_memory::{Bytes, VolatileMemory, VolatileSlice}; - /// # use std::fs::File; - /// # use std::path::Path; - /// # - /// # if cfg!(unix) { - /// # let mut mem = [0u8; 1024]; - /// # let vslice = VolatileSlice::from(&mut mem[..]); - /// let mut file = File::open(Path::new("/dev/urandom")).expect("Could not open /dev/urandom"); - /// - /// vslice - /// .read_exact_from(32, &mut file, 128) - /// .expect("Could not read bytes from file into VolatileSlice"); - /// - /// let rand_val: u32 = vslice - /// .read_obj(40) - /// .expect("Could not read value from VolatileSlice"); - /// # } - /// ``` - fn read_exact_from(&self, addr: usize, src: &mut F, count: usize) -> Result<()> - where - F: Read, - { - let _ = self.compute_end_offset(addr, count)?; - - let mut dst = vec![0; count]; - - // Read into buffer that can be copied into guest memory - src.read_exact(&mut dst).map_err(Error::IOError)?; - - let slice = self.subslice(addr, count)?; - - // SAFETY: We have checked via compute_end_offset that accessing the specified - // region of guest memory is valid. We know that `dst` has len `count`, and that the - // regions don't overlap because we allocated the Vec outside of guest memory - unsafe { copy_to_volatile_slice(&slice, dst.as_ptr(), count) }; - Ok(()) - } - - /// # Examples - /// - /// * Write 128 bytes to /dev/null - /// - /// ``` - /// # use vm_memory::{Bytes, VolatileMemory, VolatileSlice}; - /// # use std::fs::OpenOptions; - /// # use std::path::Path; - /// # - /// # if cfg!(unix) { - /// # let mut mem = [0u8; 1024]; - /// # let vslice = VolatileSlice::from(&mut mem[..]); - /// let mut file = OpenOptions::new() - /// .write(true) - /// .open("/dev/null") - /// .expect("Could not open /dev/null"); - /// - /// vslice - /// .write_to(32, &mut file, 128) - /// .expect("Could not write value from VolatileSlice to /dev/null"); - /// # } - /// ``` - fn write_to(&self, addr: usize, dst: &mut F, count: usize) -> Result - where - F: Write, - { - let _ = self.compute_end_offset(addr, count)?; - let mut src = Vec::with_capacity(count); - - let slice = self.subslice(addr, count)?; - - // SAFETY: We checked the addr and count so accessing the slice is safe. - // It is safe to read from volatile memory. The Vec has capacity for exactly `count` - // many bytes, and the memory regions pointed to definitely do not overlap, as we - // allocated src outside of guest memory. - // The call to set_len is safe because the bytes between 0 and count have been initialized - // via copying from guest memory, and the Vec's capacity is `count` - unsafe { - copy_from_volatile_slice(src.as_mut_ptr(), &slice, count); - src.set_len(count); - } - - loop { - match dst.write(&src) { - Ok(n) => break Ok(n), - Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => continue, - Err(e) => break Err(Error::IOError(e)), - } - } - } - - /// # Examples - /// - /// * Write 128 bytes to /dev/null - /// - /// ``` - /// # use vm_memory::{Bytes, VolatileMemory, VolatileSlice}; - /// # use std::fs::OpenOptions; - /// # use std::path::Path; - /// # - /// # if cfg!(unix) { - /// # let mut mem = [0u8; 1024]; - /// # let vslice = VolatileSlice::from(&mut mem[..]); - /// let mut file = OpenOptions::new() - /// .write(true) - /// .open("/dev/null") - /// .expect("Could not open /dev/null"); - /// - /// vslice - /// .write_all_to(32, &mut file, 128) - /// .expect("Could not write value from VolatileSlice to /dev/null"); - /// # } - /// ``` - fn write_all_to(&self, addr: usize, dst: &mut F, count: usize) -> Result<()> - where - F: Write, - { - let _ = self.compute_end_offset(addr, count)?; - let mut src = Vec::with_capacity(count); - - let slice = self.subslice(addr, count)?; - - // SAFETY: We checked the addr and count so accessing the slice is safe. - // It is safe to read from volatile memory. The Vec has capacity for exactly `count` - // many bytes, and the memory regions pointed to definitely do not overlap, as we - // allocated src outside of guest memory. - // The call to set_len is safe because the bytes between 0 and count have been initialized - // via copying from guest memory, and the Vec's capacity is `count` - unsafe { - copy_from_volatile_slice(src.as_mut_ptr(), &slice, count); - src.set_len(count); - } - - dst.write_all(&src).map_err(Error::IOError)?; - - Ok(()) - } - fn store(&self, val: T, addr: usize, order: Ordering) -> Result<()> { self.get_atomic_ref::(addr).map(|r| { r.store(val.into(), order); @@ -1085,18 +880,6 @@ where } } - /// Returns a pointer to the underlying memory. Mutable accesses performed - /// using the resulting pointer are not automatically accounted for by the dirty bitmap - /// tracking functionality. - #[deprecated( - since = "0.12.1", - note = "Use `.ptr_guard()` or `.ptr_guard_mut()` instead" - )] - #[cfg(not(all(feature = "xen", unix)))] - pub fn as_ptr(&self) -> *mut u8 { - self.addr as *mut u8 - } - /// Returns a guard for the pointer to the underlying memory. pub fn ptr_guard(&self) -> PtrGuard { PtrGuard::read(self.mmap, self.addr as *mut u8, self.len()) @@ -1278,18 +1061,6 @@ where size_of::() } - /// Returns a pointer to the underlying memory. Mutable accesses performed - /// using the resulting pointer are not automatically accounted for by the dirty bitmap - /// tracking functionality. - #[deprecated( - since = "0.12.1", - note = "Use `.ptr_guard()` or `.ptr_guard_mut()` instead" - )] - #[cfg(not(all(feature = "xen", unix)))] - pub fn as_ptr(&self) -> *mut u8 { - self.addr - } - /// Returns a guard for the pointer to the underlying memory. pub fn ptr_guard(&self) -> PtrGuard { PtrGuard::read(self.mmap, self.addr, self.len()) From 21192e2b995502e58f9714d985a5a0c5703f640a Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 3 Oct 2024 07:45:32 +0100 Subject: [PATCH 08/14] fix: move volatile memory access methods to `Bytes` trait The `read_[exact_]_volatile_from` and `write_[all_]volatile_to` functions were intended to be replacements for their `Read` and `Write` based counterparts. However, those used to live in the `Bytes` trait, not the `GuestMemory` trait. Fix up this goof on my part by moving them to the `Bytes` trait. Signed-off-by: Patrick Roy --- CHANGELOG.md | 5 ++ src/bytes.rs | 145 ++++++++++++++++++++++++++++++++ src/guest_memory.rs | 185 ++++++++++++----------------------------- src/mmap.rs | 64 +++++++++++++- src/volatile_memory.rs | 36 +++++++- 5 files changed, 300 insertions(+), 135 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d114741..1881e7bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,11 @@ - \[[#311](https://github.com/rust-vmm/vm-memory/pull/311)\] Allow compiling without the ReadVolatile and WriteVolatile implementations +### Changed + +- \[[#307](https://github.com/rust-vmm/vm-memory/pull/304)\] Move `read_volatile_from`, `read_exact_volatile_from`, + `write_volatile_to` and `write_all_volatile_to` functions from the `GuestMemory` trait to the `Bytes` trait. + ### Removed - \[[#307](https://github.com/rust-vmm/vm-memory/pull/304)\] Remove deprecated functions `Bytes::read_from`, `Bytes::read_exact_from`, diff --git a/src/bytes.rs b/src/bytes.rs index b5d51ff7..401c488d 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -18,6 +18,7 @@ use std::sync::atomic::Ordering; use crate::atomic_integer::AtomicInteger; use crate::volatile_memory::VolatileSlice; +use crate::{ReadVolatile, WriteVolatile}; /// Types for which it is safe to initialize from raw data. /// @@ -276,6 +277,102 @@ pub trait Bytes { self.read_slice(result.as_mut_slice(), addr).map(|_| result) } + /// Reads up to `count` bytes from `src` and writes them into the container at `addr`. + /// Unlike `VolatileRead::read_volatile`, this function retries on `EINTR` being returned from + /// the underlying I/O `read` operation. + /// + /// Returns the number of bytes written into the container. + /// + /// # Arguments + /// * `addr` - Begin writing at this address. + /// * `src` - Copy from `src` into the container. + /// * `count` - Copy `count` bytes from `src` into the container. + /// + /// # Examples + /// + /// * Read bytes from /dev/urandom (uses the `backend-mmap` feature) + /// + /// ``` + /// # #[cfg(all(feature = "backend-mmap", feature = "rawfd"))] + /// # { + /// # use vm_memory::{Address, GuestMemory, Bytes, GuestAddress, GuestMemoryMmap}; + /// # use std::fs::File; + /// # use std::path::Path; + /// # + /// # let start_addr = GuestAddress(0x1000); + /// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 0x400)]) + /// # .expect("Could not create guest memory"); + /// # let addr = GuestAddress(0x1010); + /// # let mut file = if cfg!(unix) { + /// let mut file = File::open(Path::new("/dev/urandom")).expect("Could not open /dev/urandom"); + /// # file + /// # } else { + /// # File::open(Path::new("c:\\Windows\\system32\\ntoskrnl.exe")) + /// # .expect("Could not open c:\\Windows\\system32\\ntoskrnl.exe") + /// # }; + /// + /// gm.read_volatile_from(addr, &mut file, 128) + /// .expect("Could not read from /dev/urandom into guest memory"); + /// + /// let read_addr = addr.checked_add(8).expect("Could not compute read address"); + /// let rand_val: u32 = gm + /// .read_obj(read_addr) + /// .expect("Could not read u32 val from /dev/urandom"); + /// # } + /// ``` + fn read_volatile_from(&self, addr: A, src: &mut F, count: usize) -> Result + where + F: ReadVolatile; + + /// Reads exactly `count` bytes from an object and writes them into the container at `addr`. + /// + /// # Errors + /// + /// Returns an error if `count` bytes couldn't have been copied from `src` to the container. + /// Part of the data may have been copied nevertheless. + /// + /// # Arguments + /// * `addr` - Begin writing at this address. + /// * `src` - Copy from `src` into the container. + /// * `count` - Copy exactly `count` bytes from `src` into the container. + fn read_exact_volatile_from( + &self, + addr: A, + src: &mut F, + count: usize, + ) -> Result<(), Self::E> + where + F: ReadVolatile; + + /// Reads up to `count` bytes from the container at `addr` and writes them into `dst`. + /// Unlike `VolatileWrite::write_volatile`, this function retries on `EINTR` being returned by + /// the underlying I/O `write` operation. + /// + /// Returns the number of bytes written into the object. + /// + /// # Arguments + /// * `addr` - Begin reading from this address. + /// * `dst` - Copy from the container to `dst`. + /// * `count` - Copy `count` bytes from the container to `dst`. + fn write_volatile_to(&self, addr: A, dst: &mut F, count: usize) -> Result + where + F: WriteVolatile; + + /// Reads exactly `count` bytes from the container at `addr` and writes them into an object. + /// + /// # Errors + /// + /// Returns an error if `count` bytes couldn't have been copied from the container to `dst`. + /// Part of the data may have been copied nevertheless. + /// + /// # Arguments + /// * `addr` - Begin reading from this address. + /// * `dst` - Copy from the container to `dst`. + /// * `count` - Copy exactly `count` bytes from the container to `dst`. + fn write_all_volatile_to(&self, addr: A, dst: &mut F, count: usize) -> Result<(), Self::E> + where + F: WriteVolatile; + /// Atomically store a value at the specified address. fn store(&self, val: T, addr: A, order: Ordering) -> Result<(), Self::E>; @@ -414,6 +511,54 @@ pub(crate) mod tests { Ok(()) } + fn read_volatile_from( + &self, + _addr: usize, + _src: &mut F, + _count: usize, + ) -> Result + where + F: ReadVolatile, + { + unimplemented!() + } + + fn read_exact_volatile_from( + &self, + _addr: usize, + _src: &mut F, + _count: usize, + ) -> Result<(), Self::E> + where + F: ReadVolatile, + { + unimplemented!() + } + + fn write_volatile_to( + &self, + _addr: usize, + _dst: &mut F, + _count: usize, + ) -> Result + where + F: WriteVolatile, + { + unimplemented!() + } + + fn write_all_volatile_to( + &self, + _addr: usize, + _dst: &mut F, + _count: usize, + ) -> Result<(), Self::E> + where + F: WriteVolatile, + { + unimplemented!() + } + fn store( &self, _val: T, diff --git a/src/guest_memory.rs b/src/guest_memory.rs index 865f9c52..189ea8dd 100644 --- a/src/guest_memory.rs +++ b/src/guest_memory.rs @@ -54,7 +54,6 @@ use crate::bitmap::{Bitmap, BS, MS}; use crate::bytes::{AtomicAccess, Bytes}; use crate::io::{ReadVolatile, WriteVolatile}; use crate::volatile_memory::{self, VolatileSlice}; -use crate::GuestMemoryError; /// Errors associated with handling guest memory accesses. #[allow(missing_docs)] @@ -538,137 +537,6 @@ pub trait GuestMemory { } } - /// Reads up to `count` bytes from an object and writes them into guest memory at `addr`. - /// - /// Returns the number of bytes written into guest memory. - /// - /// # Arguments - /// * `addr` - Begin writing at this address. - /// * `src` - Copy from `src` into the container. - /// * `count` - Copy `count` bytes from `src` into the container. - /// - /// # Examples - /// - /// * Read bytes from /dev/urandom (uses the `backend-mmap` feature) - /// - /// ``` - /// # #[cfg(all(feature = "backend-mmap", feature = "rawfd"))] - /// # { - /// # use vm_memory::{Address, GuestMemory, Bytes, GuestAddress, GuestMemoryMmap}; - /// # use std::fs::File; - /// # use std::path::Path; - /// # - /// # let start_addr = GuestAddress(0x1000); - /// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 0x400)]) - /// # .expect("Could not create guest memory"); - /// # let addr = GuestAddress(0x1010); - /// # let mut file = if cfg!(unix) { - /// let mut file = File::open(Path::new("/dev/urandom")).expect("Could not open /dev/urandom"); - /// # file - /// # } else { - /// # File::open(Path::new("c:\\Windows\\system32\\ntoskrnl.exe")) - /// # .expect("Could not open c:\\Windows\\system32\\ntoskrnl.exe") - /// # }; - /// - /// gm.read_volatile_from(addr, &mut file, 128) - /// .expect("Could not read from /dev/urandom into guest memory"); - /// - /// let read_addr = addr.checked_add(8).expect("Could not compute read address"); - /// let rand_val: u32 = gm - /// .read_obj(read_addr) - /// .expect("Could not read u32 val from /dev/urandom"); - /// # } - /// ``` - fn read_volatile_from(&self, addr: GuestAddress, src: &mut F, count: usize) -> Result - where - F: ReadVolatile, - { - self.try_access(count, addr, |_, len, caddr, region| -> Result { - let mut vslice = region.get_slice(caddr, len)?; - - src.read_volatile(&mut vslice) - .map_err(GuestMemoryError::from) - }) - } - - /// Reads up to `count` bytes from guest memory at `addr` and writes them it into an object. - /// - /// Returns the number of bytes copied from guest memory. - /// - /// # Arguments - /// * `addr` - Begin reading from this address. - /// * `dst` - Copy from guest memory to `dst`. - /// * `count` - Copy `count` bytes from guest memory to `dst`. - fn write_volatile_to(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result - where - F: WriteVolatile, - { - self.try_access(count, addr, |_, len, caddr, region| -> Result { - let vslice = region.get_slice(caddr, len)?; - - // For a non-RAM region, reading could have side effects, so we - // must use write_all(). - dst.write_all_volatile(&vslice)?; - - Ok(len) - }) - } - - /// Reads exactly `count` bytes from an object and writes them into guest memory at `addr`. - /// - /// # Errors - /// - /// Returns an error if `count` bytes couldn't have been copied from `src` to guest memory. - /// Part of the data may have been copied nevertheless. - /// - /// # Arguments - /// * `addr` - Begin writing at this address. - /// * `src` - Copy from `src` into guest memory. - /// * `count` - Copy exactly `count` bytes from `src` into guest memory. - fn read_exact_volatile_from( - &self, - addr: GuestAddress, - src: &mut F, - count: usize, - ) -> Result<()> - where - F: ReadVolatile, - { - let res = self.read_volatile_from(addr, src, count)?; - if res != count { - return Err(Error::PartialBuffer { - expected: count, - completed: res, - }); - } - Ok(()) - } - - /// Reads exactly `count` bytes from guest memory at `addr` and writes them into an object. - /// - /// # Errors - /// - /// Returns an error if `count` bytes couldn't have been copied from guest memory to `dst`. - /// Part of the data may have been copied nevertheless. - /// - /// # Arguments - /// * `addr` - Begin reading from this address. - /// * `dst` - Copy from guest memory to `dst`. - /// * `count` - Copy exactly `count` bytes from guest memory to `dst`. - fn write_all_volatile_to(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result<()> - where - F: WriteVolatile, - { - let res = self.write_volatile_to(addr, dst, count)?; - if res != count { - return Err(Error::PartialBuffer { - expected: count, - completed: res, - }); - } - Ok(()) - } - /// Get the host virtual address corresponding to the guest address. /// /// Some [`GuestMemory`](trait.GuestMemory.html) implementations, like `GuestMemoryMmap`, @@ -796,6 +664,59 @@ impl Bytes for T { Ok(()) } + fn read_volatile_from(&self, addr: GuestAddress, src: &mut F, count: usize) -> Result + where + F: ReadVolatile, + { + self.try_access(count, addr, |_, len, caddr, region| -> Result { + region.read_volatile_from(caddr, src, len) + }) + } + + fn read_exact_volatile_from( + &self, + addr: GuestAddress, + src: &mut F, + count: usize, + ) -> Result<()> + where + F: ReadVolatile, + { + let res = self.read_volatile_from(addr, src, count)?; + if res != count { + return Err(Error::PartialBuffer { + expected: count, + completed: res, + }); + } + Ok(()) + } + + fn write_volatile_to(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result + where + F: WriteVolatile, + { + self.try_access(count, addr, |_, len, caddr, region| -> Result { + // For a non-RAM region, reading could have side effects, so we + // must use write_all(). + region.write_all_volatile_to(caddr, dst, len).map(|()| len) + }) + } + + fn write_all_volatile_to(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result<()> + where + F: WriteVolatile, + { + let res = self.write_volatile_to(addr, dst, count)?; + if res != count { + return Err(Error::PartialBuffer { + expected: count, + completed: res, + }); + } + Ok(()) + } + fn store(&self, val: O, addr: GuestAddress, order: Ordering) -> Result<()> { // `find_region` should really do what `to_region_addr` is doing right now, except // it should keep returning a `Result`. diff --git a/src/mmap.rs b/src/mmap.rs index bc821d41..a5c81f24 100644 --- a/src/mmap.rs +++ b/src/mmap.rs @@ -26,7 +26,7 @@ use crate::guest_memory::{ self, FileOffset, GuestAddress, GuestMemory, GuestMemoryRegion, GuestUsize, MemoryRegionAddress, }; use crate::volatile_memory::{VolatileMemory, VolatileSlice}; -use crate::{AtomicAccess, Bytes}; +use crate::{AtomicAccess, Bytes, ReadVolatile, WriteVolatile}; #[cfg(all(not(feature = "xen"), unix))] pub use crate::mmap_unix::{Error as MmapRegionError, MmapRegion, MmapRegionBuilder}; @@ -232,6 +232,66 @@ impl Bytes for GuestRegionMmap { .map_err(Into::into) } + fn read_volatile_from( + &self, + addr: MemoryRegionAddress, + src: &mut F, + count: usize, + ) -> Result + where + F: ReadVolatile, + { + self.as_volatile_slice() + .unwrap() + .read_volatile_from(addr.0 as usize, src, count) + .map_err(Into::into) + } + + fn read_exact_volatile_from( + &self, + addr: MemoryRegionAddress, + src: &mut F, + count: usize, + ) -> Result<(), Self::E> + where + F: ReadVolatile, + { + self.as_volatile_slice() + .unwrap() + .read_exact_volatile_from(addr.0 as usize, src, count) + .map_err(Into::into) + } + + fn write_volatile_to( + &self, + addr: MemoryRegionAddress, + dst: &mut F, + count: usize, + ) -> Result + where + F: WriteVolatile, + { + self.as_volatile_slice() + .unwrap() + .write_volatile_to(addr.0 as usize, dst, count) + .map_err(Into::into) + } + + fn write_all_volatile_to( + &self, + addr: MemoryRegionAddress, + dst: &mut F, + count: usize, + ) -> Result<(), Self::E> + where + F: WriteVolatile, + { + self.as_volatile_slice() + .unwrap() + .write_all_volatile_to(addr.0 as usize, dst, count) + .map_err(Into::into) + } + fn store( &self, val: T, @@ -460,10 +520,10 @@ mod tests { use crate::bitmap::AtomicBitmap; use crate::GuestAddressSpace; + use std::io::Write; use std::mem; #[cfg(feature = "rawfd")] use std::{fs::File, path::Path}; - use std::io::Write; use vmm_sys_util::tempfile::TempFile; type GuestMemoryMmap = super::GuestMemoryMmap<()>; diff --git a/src/volatile_memory.rs b/src/volatile_memory.rs index 989c79e1..486f5720 100644 --- a/src/volatile_memory.rs +++ b/src/volatile_memory.rs @@ -45,7 +45,7 @@ use crate::mmap_xen::{MmapXen as MmapInfo, MmapXenSlice}; #[cfg(not(feature = "xen"))] type MmapInfo = std::marker::PhantomData<()>; -use crate::io::{ReadVolatile, WriteVolatile}; +use crate::io::{retry_eintr, ReadVolatile, WriteVolatile}; use copy_slice_impl::{copy_from_volatile_slice, copy_to_volatile_slice}; /// `VolatileMemory` related errors. @@ -793,6 +793,40 @@ impl Bytes for VolatileSlice<'_, B> { Ok(()) } + fn read_volatile_from(&self, addr: usize, src: &mut F, count: usize) -> Result + where + F: ReadVolatile, + { + let slice = self.offset(addr)?; + /* Unwrap safe here because (0, min(len, count)) is definitely a valid subslice */ + let mut slice = slice.subslice(0, slice.len().min(count)).unwrap(); + retry_eintr!(src.read_volatile(&mut slice)) + } + + fn read_exact_volatile_from(&self, addr: usize, src: &mut F, count: usize) -> Result<()> + where + F: ReadVolatile, + { + src.read_exact_volatile(&mut self.get_slice(addr, count)?) + } + + fn write_volatile_to(&self, addr: usize, dst: &mut F, count: usize) -> Result + where + F: WriteVolatile, + { + let slice = self.offset(addr)?; + /* Unwrap safe here because (0, min(len, count)) is definitely a valid subslice */ + let slice = slice.subslice(0, slice.len().min(count)).unwrap(); + retry_eintr!(dst.write_volatile(&slice)) + } + + fn write_all_volatile_to(&self, addr: usize, dst: &mut F, count: usize) -> Result<()> + where + F: WriteVolatile, + { + dst.write_all_volatile(&self.get_slice(addr, count)?) + } + fn store(&self, val: T, addr: usize, order: Ordering) -> Result<()> { self.get_atomic_ref::(addr).map(|r| { r.store(val.into(), order); From 4327288f031f7f821173c3cd1c5585b519f20ef7 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Tue, 12 Nov 2024 11:34:47 +0000 Subject: [PATCH 09/14] doc: clarify behavior of `Bytes::read` and co Capture the actual behavior of various edge cases around empty buffers and containers in the doc comment. Signed-off-by: Patrick Roy --- src/bytes.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/bytes.rs b/src/bytes.rs index 401c488d..2a9508e7 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -227,16 +227,41 @@ pub trait Bytes { /// Returns the number of bytes written. The number of bytes written can /// be less than the length of the slice if there isn't enough room in the /// container. + /// + /// If the given slice is empty (e.g. has length 0), always returns `Ok(0)`, even if `addr` + /// is otherwise out of bounds. However, if the container is empty, it will + /// return an error (unless the slice is also empty, in which case the above takes precedence). + /// + /// ```rust + /// # use vm_memory::{Bytes, VolatileMemoryError, VolatileSlice}; + /// let mut arr = [1, 2, 3, 4, 5]; + /// let slice = VolatileSlice::from(arr.as_mut_slice()); + /// + /// assert_eq!(slice.write(&[1, 2, 3], 0).unwrap(), 3); + /// assert_eq!(slice.write(&[1, 2, 3], 3).unwrap(), 2); + /// assert!(matches!( + /// slice.write(&[1, 2, 3], 5).unwrap_err(), + /// VolatileMemoryError::OutOfBounds { addr: 5 } + /// )); + /// assert_eq!(slice.write(&[], 5).unwrap(), 0); + /// ``` fn write(&self, buf: &[u8], addr: A) -> Result; /// Reads data from the container at `addr` into a slice. /// /// Returns the number of bytes read. The number of bytes read can be less than the length /// of the slice if there isn't enough data within the container. + /// + /// If the given slice is empty (e.g. has length 0), always returns `Ok(0)`, even if `addr` + /// is otherwise out of bounds. However, if the container is empty, it will + /// return an error (unless the slice is also empty, in which case the above takes precedence). fn read(&self, buf: &mut [u8], addr: A) -> Result; /// Writes the entire content of a slice into the container at `addr`. /// + /// If the given slice is empty (e.g. has length 0), always returns `Ok(0)`, even if `addr` + /// is otherwise out of bounds. + /// /// # Errors /// /// Returns an error if there isn't enough space within the container to write the entire slice. @@ -245,6 +270,9 @@ pub trait Bytes { /// Reads data from the container at `addr` to fill an entire slice. /// + /// If the given slice is empty (e.g. has length 0), always returns `Ok(0)`, even if `addr` + /// is otherwise out of bounds. + /// /// # Errors /// /// Returns an error if there isn't enough data within the container to fill the entire slice. From 5509bba65b1ab463b6a04e54e91bb76eb1b8270e Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 13 Nov 2024 07:30:56 +0000 Subject: [PATCH 10/14] chore: update coverage Signed-off-by: Patrick Roy --- coverage_config_x86_64.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json index 9647dc46..c55dbb37 100644 --- a/coverage_config_x86_64.json +++ b/coverage_config_x86_64.json @@ -1,5 +1,5 @@ { - "coverage_score": 91.41, + "coverage_score": 92.92, "exclude_path": "mmap_windows.rs", "crate_features": "backend-mmap,backend-atomic,backend-bitmap" } From 6c8925463c8ad13c1fb6511e76192c232d4c2d56 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Mon, 3 Feb 2025 12:45:19 +0000 Subject: [PATCH 11/14] refactor: impl ByteValued::as_bytes via as_mut_slice It's the same cast that happens, but like this we can use the `From<&mut [u8]>` impl for VolatileSlice and avoid an unsafe block. Signed-off-by: Patrick Roy --- src/bytes.rs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index 2a9508e7..a28aad94 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -113,16 +113,8 @@ pub unsafe trait ByteValued: Copy + Send + Sync { /// Converts a mutable reference to `self` into a `VolatileSlice`. This is /// useful because `VolatileSlice` provides a `Bytes` implementation. - /// - /// # Safety - /// - /// Unlike most `VolatileMemory` implementation, this method requires an exclusive - /// reference to `self`; this trivially fulfills `VolatileSlice::new`'s requirement - /// that all accesses to `self` use volatile accesses (because there can - /// be no other accesses). fn as_bytes(&mut self) -> VolatileSlice { - // SAFETY: This is safe because the lifetime is the same as self - unsafe { VolatileSlice::new(self as *mut Self as *mut _, size_of::()) } + VolatileSlice::from(self.as_mut_slice()) } } From c33d5a86f03bbea74dd150cdc4016e448d0ebafa Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Mon, 3 Feb 2025 12:46:53 +0000 Subject: [PATCH 12/14] fix: mark endian types as `repr(transparent)` Without this, the impl of `ByteValued` was technically undefined, although we had some const asserts that ensured rustc didn't muck us over by adding padding or anything. Signed-off-by: Patrick Roy --- src/endian.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/endian.rs b/src/endian.rs index 36e1352d..40e49b14 100644 --- a/src/endian.rs +++ b/src/endian.rs @@ -49,6 +49,7 @@ macro_rules! endian_type { /// /// See module level documentation for examples. #[derive(Copy, Clone, Eq, PartialEq, Debug, Default)] + #[repr(transparent)] pub struct $new_type($old_type); impl $new_type { From b1aaf8e1b8084879e3d7d8583866de2f98ead6b4 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Mon, 3 Feb 2025 12:52:10 +0000 Subject: [PATCH 13/14] Add ByteValued::{write_all_to,read_exact_from} Suggested-by: Paolo Bonzini Signed-off-by: Patrick Roy --- src/bytes.rs | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/src/bytes.rs b/src/bytes.rs index a28aad94..dc3c4b1e 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -11,6 +11,7 @@ //! Define the `ByteValued` trait to mark that it is safe to instantiate the struct with random //! data. +use std::io::{Read, Write}; use std::mem::{size_of, MaybeUninit}; use std::result::Result; use std::slice::{from_raw_parts, from_raw_parts_mut}; @@ -116,6 +117,20 @@ pub unsafe trait ByteValued: Copy + Send + Sync { fn as_bytes(&mut self) -> VolatileSlice { VolatileSlice::from(self.as_mut_slice()) } + + /// Writes this [`ByteValued`]'s byte representation to the given [`Write`] impl. + fn write_all_to(&self, mut writer: W) -> Result<(), std::io::Error> { + writer.write_all(self.as_slice()) + } + + /// Constructs an instance of this [`ByteValued`] by reading from the given [`Read`] impl. + fn read_exact_from(mut reader: R) -> Result { + // SAFETY: ByteValued objects must be assignable from arbitrary byte + // sequences and are mandated to be packed. + // Hence, zeroed memory is a fine initialization. + let mut result: Self = unsafe { MaybeUninit::::zeroed().assume_init() }; + reader.read_exact(result.as_mut_slice()).map(|_| result) + } } macro_rules! byte_valued_array { @@ -407,6 +422,7 @@ pub(crate) mod tests { use std::cell::RefCell; use std::fmt::Debug; + use std::io::ErrorKind; use std::mem::align_of; // Helper method to test atomic accesses for a given `b: Bytes` that's supposed to be @@ -607,7 +623,7 @@ pub(crate) mod tests { } #[repr(C)] - #[derive(Copy, Clone, Default)] + #[derive(Copy, Clone, Default, Debug)] struct S { a: u32, b: u32, @@ -623,4 +639,24 @@ pub(crate) mod tests { assert_eq!(s.a, 0); assert_eq!(s.b, 0x0101_0101); } + + #[test] + fn test_byte_valued_io() { + let a: [u8; 8] = [0, 0, 0, 0, 1, 1, 1, 1]; + + let result = S::read_exact_from(&a[1..]); + assert_eq!(result.unwrap_err().kind(), ErrorKind::UnexpectedEof); + + let s = S::read_exact_from(&a[..]).unwrap(); + assert_eq!(s.a, 0); + assert_eq!(s.b, 0x0101_0101); + + let mut b = Vec::new(); + s.write_all_to(&mut b).unwrap(); + assert_eq!(a.as_ref(), b.as_slice()); + + let mut b = [0; 7]; + let result = s.write_all_to(b.as_mut_slice()); + assert_eq!(result.unwrap_err().kind(), ErrorKind::WriteZero); + } } From 1ab6cd677dc79f8815c3144347c61059ed62f6bf Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 7 Feb 2025 10:09:30 +0000 Subject: [PATCH 14/14] add `ByteValued::zeroed()` We had the same unsafe code for constructing an all-zeroes `ByteValued` in two locations. Unify these into a single `ByteValued::zeroed()` function. Suggested-by: Paolo Bonzini Signed-off-by: Patrick Roy --- src/bytes.rs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/bytes.rs b/src/bytes.rs index dc3c4b1e..253d368f 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -118,6 +118,14 @@ pub unsafe trait ByteValued: Copy + Send + Sync { VolatileSlice::from(self.as_mut_slice()) } + /// Constructs a `Self` ewhose binary representation is set to all zeroes. + fn zeroed() -> Self { + // SAFETY: ByteValued objects must be assignable from arbitrary byte + // sequences and are mandated to be packed. + // Hence, zeroed memory is a fine initialization. + unsafe { MaybeUninit::::zeroed().assume_init() } + } + /// Writes this [`ByteValued`]'s byte representation to the given [`Write`] impl. fn write_all_to(&self, mut writer: W) -> Result<(), std::io::Error> { writer.write_all(self.as_slice()) @@ -125,10 +133,7 @@ pub unsafe trait ByteValued: Copy + Send + Sync { /// Constructs an instance of this [`ByteValued`] by reading from the given [`Read`] impl. fn read_exact_from(mut reader: R) -> Result { - // SAFETY: ByteValued objects must be assignable from arbitrary byte - // sequences and are mandated to be packed. - // Hence, zeroed memory is a fine initialization. - let mut result: Self = unsafe { MaybeUninit::::zeroed().assume_init() }; + let mut result = Self::zeroed(); reader.read_exact(result.as_mut_slice()).map(|_| result) } } @@ -305,10 +310,7 @@ pub trait Bytes { /// /// Returns an error if there's not enough data inside the container. fn read_obj(&self, addr: A) -> Result { - // SAFETY: ByteValued objects must be assignable from a arbitrary byte - // sequence and are mandated to be packed. - // Hence, zeroed memory is a fine initialization. - let mut result: T = unsafe { MaybeUninit::::zeroed().assume_init() }; + let mut result = T::zeroed(); self.read_slice(result.as_mut_slice(), addr).map(|_| result) } @@ -659,4 +661,11 @@ pub(crate) mod tests { let result = s.write_all_to(b.as_mut_slice()); assert_eq!(result.unwrap_err().kind(), ErrorKind::WriteZero); } + + #[test] + fn test_byte_valued_zeroed() { + let s = S::zeroed(); + + assert!(s.as_slice().iter().all(|&b| b == 0x0)); + } }