refactor(isolation/filemap): Improve API

This shifts usages of `CStr` and `OsStr` onto API consumers.

All places where guest paths are used now use `&str`,
and all places where host paths are used now use `&Path`
(or the owned variants, respectively).

Author: fogti

src/hypercall.rs

@@ -259,19 +259,35 @@ fn translate_last_errno() -> Option<i32> {
 	None
 }
 
+/// Decodes a guest path given at address `path_addr` in `mem`.
+///
+/// # Safety
+///
+/// The calling convention of hypercalls ensures that the given address doesn't alias with anything mutable.
+/// The return value is only valid for the duration of the hypercall.
+unsafe fn decode_guest_path(mem: &MmapMemory, path_addr: GuestPhysAddr) -> Option<&str> {
+	let requested_path_ptr = mem.host_address(path_addr).unwrap() as *const i8;
+	unsafe { CStr::from_ptr(requested_path_ptr) }.to_str().ok()
+}
+
 /// unlink deletes a name from the filesystem. This is used to handle `unlink` syscalls from the guest.
 ///
-/// Note for when using Landlock: Unlinking files results in them being veiled. If a text
+/// Note for when using Landlock: Unlinking files results in them being veiled. If a
 /// file (that existed during initialization) called `log.txt` is unlinked, attempting to
 /// open `log.txt` again will result in an error.
 fn unlink(mem: &MmapMemory, sysunlink: &mut UnlinkParams, file_map: &mut UhyveFileMap) {
-	let requested_path_ptr = mem.host_address(sysunlink.name).unwrap() as *const i8;
-	let guest_path = unsafe { CStr::from_ptr(requested_path_ptr) };
+	let guest_path = if let Some(guest_path) = unsafe { decode_guest_path(mem, sysunlink.name) } {
+		guest_path
+	} else {
+		error!("The kernel requested to unlink() an non-UTF8 path: Rejecting...");
+		sysunlink.ret = -EINVAL;
+		return;
+	};
 	sysunlink.ret = match file_map.unlink(guest_path) {
 		Ok(Some(host_path)) => {
 			// We can safely unwrap here, as host_path.as_bytes will never contain internal \0 bytes
 			// As host_path_c_string is a valid CString, this implementation is presumed to be safe.
-			let host_path_c_string = CString::new(host_path.as_bytes()).unwrap();
+			let host_path_c_string = CString::new(host_path.as_os_str().as_bytes()).unwrap();
 			if unsafe { libc::unlink(host_path_c_string.as_c_str().as_ptr()) } < 0 {
 				-translate_last_errno().unwrap_or(1)
 			} else {
@@ -293,9 +309,14 @@ fn unlink(mem: &MmapMemory, sysunlink: &mut UnlinkParams, file_map: &mut UhyveFi
 
 /// Handles an open syscall by opening a file on the host.
 fn open(mem: &MmapMemory, sysopen: &mut OpenParams, file_map: &mut UhyveFileMap) {
-	let requested_path_ptr = mem.host_address(sysopen.name).unwrap() as *const i8;
+	let guest_path = if let Some(guest_path) = unsafe { decode_guest_path(mem, sysopen.name) } {
+		guest_path
+	} else {
+		error!("The kernel requested to open() an non-UTF8 path: Rejecting...");
+		sysopen.ret = -EINVAL;
+		return;
+	};
 	let mut flags = sysopen.flags & ALLOWED_OPEN_FLAGS;
-	let guest_path = unsafe { CStr::from_ptr(requested_path_ptr) };
 	// See: https://lwn.net/Articles/926782/
 	// See: https://github.com/hermit-os/kernel/commit/71bc629
 	if (flags & (O_DIRECTORY | O_CREAT)) == (O_DIRECTORY | O_CREAT) {
@@ -378,10 +399,7 @@ fn open(mem: &MmapMemory, sysopen: &mut OpenParams, file_map: &mut UhyveFileMap)
 
 /// Handles an close syscall by closing the file on the host.
 fn close(sysclose: &mut CloseParams, file_map: &mut UhyveFileMap) {
-	sysclose.ret = if file_map
-		.fdmap
-		.is_fd_present(GuestFd(sysclose.fd.into_raw_fd()))
-	{
+	sysclose.ret = if file_map.fdmap.is_fd_present(GuestFd(sysclose.fd)) {
 		match file_map.fdmap.remove(GuestFd(sysclose.fd)) {
 			Some(FdData::Raw(fd)) => {
 				if unsafe { libc::close(fd) } < 0 {

src/isolation/filemap/mod.rs

@@ -1,8 +1,4 @@
-use std::{
-	ffi::{CStr, CString, OsString},
-	os::unix::ffi::OsStrExt,
-	path::PathBuf,
-};
+use std::{ffi::CString, os::unix::ffi::OsStrExt, path::PathBuf};
 
 #[cfg(target_os = "linux")]
 use libc::{O_DIRECT, O_SYNC};
@@ -99,17 +95,17 @@ impl UhyveFileMap {
 	/// Returns the host_path on the host filesystem given a requested guest_path, if it exists.
 	///
 	/// * `guest_path` - The guest path that is to be looked up in the map.
-	pub fn get_host_path(&self, guest_path: &CStr) -> Option<UhyveMapLeaf> {
-		tree::resolve_guest_path(&self.root, guest_path.to_bytes())
+	pub fn get_host_path(&self, guest_path: &str) -> Option<UhyveMapLeaf> {
+		tree::resolve_guest_path(&self.root, guest_path.as_bytes())
 	}
 
 	/// Returns an array of all host paths (for Landlock).
 	#[cfg(target_os = "linux")]
-	pub(crate) fn get_all_host_paths(&self) -> impl Iterator<Item = &std::ffi::OsStr> {
-		tree::get_all_host_paths(&self.root).map(|i| i.as_os_str())
+	pub(crate) fn get_all_host_paths(&self) -> impl Iterator<Item = &std::path::Path> {
+		tree::get_all_host_paths(&self.root)
 	}
 
-	/// Returns an iterator (non-unique) over all mountable guest directories.
+	/// Returns an iterator over all mountable guest directories.
 	pub(crate) fn get_all_guest_dirs(&self) -> impl Iterator<Item = String> {
 		tree::get_all_guest_dirs(&self.root)
 	}
@@ -139,15 +135,15 @@ impl UhyveFileMap {
 	///
 	/// Note that this is also used for the entire setup of the uhyve file tree,
 	/// and this also called for the entire initial mapping.
-	pub fn create_leaf(&mut self, guest_path: &CStr, file: UhyveMapLeaf) -> bool {
-		tree::create_leaf(&mut self.root, guest_path.to_bytes(), file)
+	pub fn create_leaf(&mut self, guest_path: &str, file: UhyveMapLeaf) -> bool {
+		tree::create_leaf(&mut self.root, guest_path.as_bytes(), file)
 	}
 
 	/// Inserts an opened temporary file into the file map. Returns a CString so that
 	/// the file can be directly used by [crate::hypercall::open].
 	///
 	/// * `guest_path` - The requested guest path.
-	pub fn create_temporary_file(&mut self, guest_path: &CStr) -> Option<CString> {
+	pub fn create_temporary_file(&mut self, guest_path: &str) -> Option<CString> {
 		let host_path = self.tempdir.path().join(Uuid::new_v4().to_string());
 		trace!("create_temporary_file (host_path): {host_path:#?}");
 		let ret = CString::new(host_path.as_os_str().as_bytes()).unwrap();
@@ -159,7 +155,7 @@ impl UhyveFileMap {
 	}
 
 	/// Attempt to remove a file. Note that this will fail on non-empty directories.
-	pub fn unlink(&mut self, guest_path: &CStr) -> Result<Option<OsString>, ()> {
-		tree::unlink(&mut self.root, guest_path.to_bytes()).map(|i| i.map(|j| j.into_os_string()))
+	pub fn unlink(&mut self, guest_path: &str) -> Result<Option<PathBuf>, ()> {
+		tree::unlink(&mut self.root, guest_path.as_bytes())
 	}
 }

src/isolation/filemap/tests.rs

@@ -43,39 +43,37 @@ fn test_uhyvefilemap() {
 	);
 
 	assert_eq!(
-		map.get_host_path(c"readme_file.md")
+		map.get_host_path("readme_file.md")
 			.unwrap()
 			.unwrap_on_host(),
 		OsString::from(&map_results[0])
 	);
 	assert_eq!(
-		map.get_host_path(c"guest_folder").unwrap().unwrap_on_host(),
+		map.get_host_path("guest_folder").unwrap().unwrap_on_host(),
 		OsString::from(&map_results[1])
 	);
 	assert_eq!(
-		map.get_host_path(c"guest_symlink")
-			.unwrap()
-			.unwrap_on_host(),
+		map.get_host_path("guest_symlink").unwrap().unwrap_on_host(),
 		OsString::from(&map_results[2])
 	);
 	assert_eq!(
-		map.get_host_path(c"guest_dangling_symlink")
+		map.get_host_path("guest_dangling_symlink")
 			.unwrap()
 			.unwrap_on_host(),
 		OsString::from(&map_results[3])
 	);
 	assert_eq!(
-		map.get_host_path(c"guest_file").unwrap().unwrap_on_host(),
+		map.get_host_path("guest_file").unwrap().unwrap_on_host(),
 		OsString::from(&map_results[4])
 	);
 	assert_eq!(
-		map.get_host_path(c"guest_file_symlink")
+		map.get_host_path("guest_file_symlink")
 			.unwrap()
 			.unwrap_on_host(),
 		OsString::from(&map_results[5])
 	);
 
-	assert!(map.get_host_path(c"this_file_is_not_mapped").is_none());
+	assert!(map.get_host_path("this_file_is_not_mapped").is_none());
 }
 
 #[test]
@@ -110,12 +108,7 @@ fn test_uhyvefilemap_directory() {
 		},
 	);
 
-	let mut found_host_path = map.get_host_path(
-		CString::new(target_guest_path.as_os_str().as_bytes())
-			.unwrap()
-			.as_c_str(),
-	);
-
+	let mut found_host_path = map.get_host_path(target_guest_path.to_str().unwrap());
 	assert_eq!(found_host_path.unwrap().unwrap_on_host(), target_host_path);
 
 	// Tests successful directory traversal of the child directory.
@@ -124,11 +117,7 @@ fn test_uhyvefilemap_directory() {
 	target_host_path.pop();
 	target_guest_path.pop();
 
-	found_host_path = map.get_host_path(
-		CString::new(target_guest_path.as_os_str().as_bytes())
-			.unwrap()
-			.as_c_str(),
-	);
+	found_host_path = map.get_host_path(target_guest_path.to_str().unwrap());
 	assert_eq!(found_host_path.unwrap().unwrap_on_host(), target_host_path);
 
 	// Tests directory traversal leading to valid symbolic link with an
@@ -154,11 +143,7 @@ fn test_uhyvefilemap_directory() {
 	target_guest_path = PathBuf::from("/root/this_symlink_leads_to_a_file");
 	target_host_path = fixture_path.clone();
 	target_host_path.push("this_folder_exists/file_in_folder.txt");
-	found_host_path = map.get_host_path(
-		CString::new(target_guest_path.as_os_str().as_bytes())
-			.unwrap()
-			.as_c_str(),
-	);
+	found_host_path = map.get_host_path(target_guest_path.to_str().unwrap());
 	assert_eq!(found_host_path.unwrap().unwrap_on_host(), target_host_path);
 
 	// Tests directory traversal with no maps
@@ -172,11 +157,7 @@ fn test_uhyvefilemap_directory() {
 			sync: false,
 		},
 	);
-	found_host_path = map.get_host_path(
-		CString::new(target_guest_path.as_os_str().as_bytes())
-			.unwrap()
-			.as_c_str(),
-	);
+	found_host_path = map.get_host_path(target_guest_path.to_str().unwrap());
 	assert!(found_host_path.is_none());
 }
 

src/vm.rs

@@ -353,7 +353,7 @@ impl<VirtBackend: VirtualizationBackend> UhyveVm<VirtBackend> {
 				file_sandbox_mode,
 				kernel_path.into(),
 				output,
-				host_paths,
+				host_paths.map(|i| i.as_os_str()),
 				temp_dir,
 				#[cfg(feature = "instrument")]
 				trace,