From 1625a96586caf5088f207053064107d00bde3d10 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 26 Sep 2025 16:54:00 +0100 Subject: [PATCH 1/5] test: move test_check_range to mmap module Switching the check_range() implementation from try_access() to get_slices() will mean that the test requires a working implementation of get_slice(), which MockRegion does not have. So instead have the test use the actual memory regions defined in the mmap module. Signed-off-by: Patrick Roy --- src/mmap/mod.rs | 23 +++++++++++++++++++++++ src/region.rs | 23 ----------------------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/mmap/mod.rs b/src/mmap/mod.rs index 1ba59f54..eb062d33 100644 --- a/src/mmap/mod.rs +++ b/src/mmap/mod.rs @@ -325,6 +325,29 @@ mod tests { } } + #[test] + fn test_check_range() { + let start_addr1 = GuestAddress(0); + let start_addr2 = GuestAddress(0x800); + let start_addr3 = GuestAddress(0xc00); + let guest_mem = GuestMemoryMmap::from_ranges(&[ + (start_addr1, 0x400), + (start_addr2, 0x400), + (start_addr3, 0x400), + ]) + .unwrap(); + + assert!(guest_mem.check_range(start_addr1, 0x0)); + assert!(guest_mem.check_range(start_addr1, 0x200)); + assert!(guest_mem.check_range(start_addr1, 0x400)); + assert!(!guest_mem.check_range(start_addr1, 0xa00)); + assert!(guest_mem.check_range(start_addr2, 0x7ff)); + assert!(guest_mem.check_range(start_addr2, 0x800)); + assert!(!guest_mem.check_range(start_addr2, 0x801)); + assert!(!guest_mem.check_range(start_addr2, 0xc00)); + assert!(!guest_mem.check_range(start_addr1, usize::MAX)); + } + #[test] fn test_deref() { let f = TempFile::new().unwrap().into_file(); diff --git a/src/region.rs b/src/region.rs index 5d4cfba8..d3ca4350 100644 --- a/src/region.rs +++ b/src/region.rs @@ -773,27 +773,4 @@ pub(crate) mod tests { Some(GuestAddress(0x400 - 1)) ); } - - #[test] - fn test_check_range() { - let start_addr1 = GuestAddress(0); - let start_addr2 = GuestAddress(0x800); - let start_addr3 = GuestAddress(0xc00); - let guest_mem = new_guest_memory_collection_from_regions(&[ - (start_addr1, 0x400), - (start_addr2, 0x400), - (start_addr3, 0x400), - ]) - .unwrap(); - - assert!(guest_mem.check_range(start_addr1, 0x0)); - assert!(guest_mem.check_range(start_addr1, 0x200)); - assert!(guest_mem.check_range(start_addr1, 0x400)); - assert!(!guest_mem.check_range(start_addr1, 0xa00)); - assert!(guest_mem.check_range(start_addr2, 0x7ff)); - assert!(guest_mem.check_range(start_addr2, 0x800)); - assert!(!guest_mem.check_range(start_addr2, 0x801)); - assert!(!guest_mem.check_range(start_addr2, 0xc00)); - assert!(!guest_mem.check_range(start_addr1, usize::MAX)); - } } From 63eb6488b479483f4018308c9d1617cd046050ba Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 1 Oct 2025 11:06:27 +0100 Subject: [PATCH 2/5] refactor: change casts in GuestMemorySliceIter::do_next Declare `len` as type `usize`, which reduce the number of casts between usize and u64 (the variable is most often uses as a usize, and only once needs to be u64). Signed-off-by: Patrick Roy --- src/guest_memory.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/guest_memory.rs b/src/guest_memory.rs index a18b42ce..4f4d869c 100644 --- a/src/guest_memory.rs +++ b/src/guest_memory.rs @@ -517,15 +517,15 @@ impl<'a, M: GuestMemory + ?Sized> GuestMemorySliceIterator<'a, M> { }; let cap = region.len() - start.raw_value(); - let len = std::cmp::min(cap, self.count as GuestUsize); + let len = std::cmp::min(cap as usize, self.count); - self.count -= len as usize; + self.count -= len; self.addr = match self.addr.overflowing_add(len as GuestUsize) { (x @ GuestAddress(0), _) | (x, false) => x, (_, true) => return Some(Err(Error::GuestAddressOverflow)), }; - Some(region.get_slice(start, len as usize)) + Some(region.get_slice(start, len)) } } From e2d2f478dc788f9f4b30e3b50a27ee68d29405a2 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 1 Oct 2025 11:07:45 +0100 Subject: [PATCH 3/5] get_slices: panic if GuestMemoryRegion violates contract Panic in get_slices() if the call to .get_slice() returns a slice that does not match the `len` parameter passed to it. If this happens there is an application bug somewhere in the GuestMemoryRegion implementation, and the library code in vm-memory, while not doing anything unsound, will probably react unpredictably to this situation. This way, the final paragraph in the get_slices() documentatoin about cumulative lengths is actually true. Signed-off-by: Patrick Roy --- src/guest_memory.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/guest_memory.rs b/src/guest_memory.rs index 4f4d869c..5dcdc710 100644 --- a/src/guest_memory.rs +++ b/src/guest_memory.rs @@ -525,7 +525,13 @@ impl<'a, M: GuestMemory + ?Sized> GuestMemorySliceIterator<'a, M> { (_, true) => return Some(Err(Error::GuestAddressOverflow)), }; - Some(region.get_slice(start, len)) + Some(region.get_slice(start, len).inspect(|s| { + assert_eq!( + s.len(), + len, + "get_slice() returned a slice with wrong length" + ) + })) } } From a9aa4de1cfa66a0042706439c3110c95d264b83c Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 26 Sep 2025 16:55:41 +0100 Subject: [PATCH 4/5] deprecate GuestMemory::try_access() Deprecate 'GuestMemory::try_access()` in favor of the external iterator provided by `GuestMemory::get_slices()`. Rewrite all internal usages of `try_access()` in terms of `get_slices()`. Signed-off-by: Patrick Roy --- CHANGELOG.md | 4 +++ coverage_config_x86_64.json | 2 +- src/bitmap/mod.rs | 12 ++----- src/guest_memory.rs | 63 ++++++++++++++++++++++--------------- 4 files changed, 45 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 13cae5d8..8b7467a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,10 @@ - \[[#339](https://github.com/rust-vmm/vm-memory/pull/339)\] Fix `Bytes::read()` and `Bytes::write()` not to ignore `try_access()`'s `count` parameter +### Deprecated + +- \[[#349](https://github.com/rust-vmm/vm-memory/pull/349)\] Deprecate `GuestMemory::try_access()`. Use `GuestMemory::get_slices()` instead. + ## \[v0.16.1\] ### Added diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json index 13f2dfd7..1c8745c5 100644 --- a/coverage_config_x86_64.json +++ b/coverage_config_x86_64.json @@ -1,5 +1,5 @@ { - "coverage_score": 91.78, + "coverage_score": 90.82, "exclude_path": "mmap_windows.rs", "crate_features": "backend-mmap,backend-atomic,backend-bitmap" } diff --git a/src/bitmap/mod.rs b/src/bitmap/mod.rs index 802ebef1..352b970c 100644 --- a/src/bitmap/mod.rs +++ b/src/bitmap/mod.rs @@ -288,16 +288,10 @@ pub(crate) mod tests { // Finally, let's invoke the generic tests for `Bytes`. let check_range_closure = |m: &M, start: usize, len: usize, clean: bool| -> bool { - let mut check_result = true; - m.try_access(len, GuestAddress(start as u64), |_, size, reg_addr, reg| { - if !check_range(®.bitmap(), reg_addr.0 as usize, size, clean) { - check_result = false; - } - Ok(size) + m.get_slices(GuestAddress(start as u64), len).all(|r| { + let slice = r.unwrap(); + check_range(slice.bitmap(), 0, slice.len(), clean) }) - .unwrap(); - - check_result }; test_bytes( diff --git a/src/guest_memory.rs b/src/guest_memory.rs index 5dcdc710..7136db35 100644 --- a/src/guest_memory.rs +++ b/src/guest_memory.rs @@ -355,10 +355,9 @@ pub trait GuestMemory { /// Check whether the range [base, base + len) is valid. fn check_range(&self, base: GuestAddress, len: usize) -> bool { - match self.try_access(len, base, |_, count, _, _| -> Result { Ok(count) }) { - Ok(count) => count == len, - _ => false, - } + // get_slices() ensures that if no error happens, the cumulative length of all slices + // equal `len`. + self.get_slices(base, len).all(|r| r.is_ok()) } /// Returns the address plus the offset if it is present within the memory of the guest. @@ -376,6 +375,10 @@ pub trait GuestMemory { /// - the error code returned by the callback 'f' /// - the size of the already handled data when encountering the first hole /// - the size of the already handled data when the whole range has been handled + #[deprecated( + since = "0.17.0", + note = "supplemented by external iterator `get_slices()`" + )] fn try_access(&self, count: usize, addr: GuestAddress, mut f: F) -> Result where F: FnMut(usize, usize, MemoryRegionAddress, &Self::R) -> Result, @@ -533,6 +536,17 @@ impl<'a, M: GuestMemory + ?Sized> GuestMemorySliceIterator<'a, M> { ) })) } + + /// Adapts this [`GuestMemorySliceIterator`] to return `None` (e.g. gracefully terminate) + /// when it encounters an error after successfully producing at least one slice. + /// Return an error if requesting the first slice returns an error. + pub fn stop_on_error(self) -> Result>>> { + let mut peek = self.peekable(); + if let Some(err) = peek.next_if(Result::is_err) { + return Err(err.unwrap_err()); + } + Ok(peek.filter_map(Result::ok)) + } } impl<'a, M: GuestMemory + ?Sized> Iterator for GuestMemorySliceIterator<'a, M> { @@ -562,23 +576,15 @@ impl Bytes for T { type E = Error; fn write(&self, buf: &[u8], addr: GuestAddress) -> Result { - self.try_access( - buf.len(), - addr, - |offset, count, caddr, region| -> Result { - region.write(&buf[offset..(offset + count)], caddr) - }, - ) + self.get_slices(addr, buf.len()) + .stop_on_error()? + .try_fold(0, |acc, slice| Ok(acc + slice.write(&buf[acc..], 0)?)) } fn read(&self, buf: &mut [u8], addr: GuestAddress) -> Result { - self.try_access( - buf.len(), - addr, - |offset, count, caddr, region| -> Result { - region.read(&mut buf[offset..(offset + count)], caddr) - }, - ) + self.get_slices(addr, buf.len()) + .stop_on_error()? + .try_fold(0, |acc, slice| Ok(acc + slice.read(&mut buf[acc..], 0)?)) } /// # Examples @@ -642,9 +648,11 @@ impl Bytes for T { where F: ReadVolatile, { - self.try_access(count, addr, |_, len, caddr, region| -> Result { - region.read_volatile_from(caddr, src, len) - }) + self.get_slices(addr, count) + .stop_on_error()? + .try_fold(0, |acc, slice| { + Ok(acc + slice.read_volatile_from(0, src, slice.len())?) + }) } fn read_exact_volatile_from( @@ -670,11 +678,14 @@ impl Bytes for T { 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) - }) + self.get_slices(addr, count) + .stop_on_error()? + .try_fold(0, |acc, slice| { + // For a non-RAM region, reading could have side effects, so we + // must use write_all(). + slice.write_all_volatile_to(0, dst, slice.len())?; + Ok(acc + slice.len()) + }) } fn write_all_volatile_to(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result<()> From 9c8fb09c5eaf8753d4bba72cb5a84d2ae5385ee8 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Wed, 1 Oct 2025 13:02:49 +0100 Subject: [PATCH 5/5] fix(test): exclude more File-based test cases from miri Miri sometimes non-deterministically has its mocks for read(2)/write(2) do only partial reads/writes, which our unittests cannot deal with. See also commit 029c652f5858 ("fix(test): do not try to read files under miri"). Mark these test cases as cfg(not(miri)). Signed-off-by: Patrick Roy --- src/mmap/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/mmap/mod.rs b/src/mmap/mod.rs index eb062d33..2f64fcff 100644 --- a/src/mmap/mod.rs +++ b/src/mmap/mod.rs @@ -455,6 +455,7 @@ mod tests { #[test] #[cfg(feature = "rawfd")] + #[cfg(not(miri))] fn read_to_and_write_from_mem() { use std::mem;