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

@@ -121,19 +121,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.
 pub 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 {
@@ -155,9 +171,14 @@ pub fn unlink(mem: &MmapMemory, sysunlink: &mut UnlinkParams, file_map: &mut Uhy
 
 /// Handles an open syscall by opening a file on the host.
 pub 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) {
@@ -240,10 +261,7 @@ pub fn open(mem: &MmapMemory, sysopen: &mut OpenParams, file_map: &mut UhyveFile
 
 /// Handles an close syscall by closing the file on the host.
 pub 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,9 +1,4 @@
-use std::{
-	ffi::{CStr, CString, OsString},
-	os::unix::ffi::OsStrExt,
-	path::PathBuf,
-	sync::Arc,
-};
+use std::{ffi::CString, os::unix::ffi::OsStrExt, path::PathBuf, sync::Arc};
 
 #[cfg(target_os = "linux")]
 use libc::{O_DIRECT, O_SYNC};
@@ -124,10 +119,7 @@ impl UhyveFileMap {
 			let data: Arc<[u8]> = i.data().to_vec().into();
 
 			// UNWRAP: `tar_no_std::ArchiveEntry::filename` already truncates at the first null byte.
-			if !self.create_file(
-				&CString::new(filename).unwrap(),
-				UhyveTreeFile::Virtual(data),
-			) {
+			if !self.create_file(filename, UhyveTreeFile::Virtual(data)) {
 				return Err(HermitImageError::CreateFile(filename.to_string()));
 			}
 		}
@@ -138,17 +130,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<UhyveTreeFile> {
-		tree::resolve_guest_path(&self.root, guest_path.to_bytes())
+	pub fn get_host_path(&self, guest_path: &str) -> Option<UhyveTreeFile> {
+		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)
 	}
@@ -178,15 +170,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_file(&mut self, guest_path: &CStr, file: UhyveTreeFile) -> bool {
-		tree::create_file(&mut self.root, guest_path.to_bytes(), file)
+	pub fn create_file(&mut self, guest_path: &str, file: UhyveTreeFile) -> bool {
+		tree::create_file(&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();
@@ -198,7 +190,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/lib.rs

@@ -49,9 +49,6 @@ pub enum HypervisorError {
 	#[error("Invalid kernel path ({0})")]
 	InvalidKernelPath(PathBuf),
 
-	#[error("Path for {0} contained null bytes")]
-	PathContainedNullBytes(&'static str),
-
 	#[error(transparent)]
 	HermitImageError(#[from] crate::isolation::filemap::HermitImageError),
 

src/vm.rs

@@ -1,7 +1,5 @@
 use std::{
-	env,
-	ffi::CString,
-	fmt, fs, io,
+	env, fmt, fs, io,
 	mem::{drop, take},
 	num::NonZero,
 	os::unix::prelude::JoinHandleExt,
@@ -175,7 +173,7 @@ impl<VirtBackend: VirtualizationBackend> UhyveVm<VirtBackend> {
 
 				// TODO: make `hermit_entry::config::DEFAULT_CONFIG_NAME` public and use that here instead.
 				let config_data = if let Some(UhyveTreeFile::Virtual(data)) =
-					file_mapping.get_host_path(&CString::new("/hermit.toml").unwrap())
+					file_mapping.get_host_path("/hermit.toml")
 				{
 					data
 				} else {
@@ -249,9 +247,8 @@ impl<VirtBackend: VirtualizationBackend> UhyveVm<VirtBackend> {
 
 						// .kernel
 						if let Some(UhyveTreeFile::Virtual(data)) =
-							file_mapping.get_host_path(&CString::new(kernel.into_owned()).map_err(
-								|_| HypervisorError::PathContainedNullBytes("hermit image kernel"),
-							)?) {
+							file_mapping.get_host_path(&kernel)
+						{
 							data.to_vec()
 						} else {
 							error!("Unable to find kernel in Hermit image");
@@ -324,8 +321,7 @@ impl<VirtBackend: VirtualizationBackend> UhyveVm<VirtBackend> {
 		let mut mem = MmapMemory::new(memory_size, guest_address, false, false);
 
 		// TODO: file_mapping not in kernel_info
-		let mut mounts: Vec<_> = file_mapping.get_all_guest_dirs().collect();
-		mounts.dedup();
+		let mounts: Vec<_> = file_mapping.get_all_guest_dirs().collect();
 		let file_mapping = Mutex::new(file_mapping);
 
 		let serial = UhyveSerial::from_params(&params.output)?;
@@ -470,7 +466,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,