Remove and prevent use of unwrap in hyperlight_host

Signed-off-by: Simon Davies <[email protected]>

Author: simongdavies

Cargo.lock

@@ -246,7 +246,11 @@ impl HypervisorHandler {
         #[cfg(target_os = "windows")]
         let in_process = sandbox_memory_manager.is_in_process();
 
-        *self.execution_variables.shm.try_lock().unwrap() = Some(sandbox_memory_manager);
+        *self
+            .execution_variables
+            .shm
+            .try_lock()
+            .map_err(|e| new_error!("Failed to lock shm: {}", e))? = Some(sandbox_memory_manager);
 
         // Other than running initialization and code execution, the handler thread also handles
         // cancellation. When we need to cancel the execution there are 2 possible cases
@@ -299,13 +303,13 @@ impl HypervisorHandler {
                             HypervisorHandlerAction::Initialise => {
                                 {
                                     hv = Some(set_up_hypervisor_partition(
-                                        execution_variables.shm.try_lock().unwrap().deref_mut().as_mut().unwrap(),
+                                        execution_variables.shm.try_lock().map_err(|e| new_error!("Failed to lock shm: {}", e))?.deref_mut().as_mut().ok_or_else(|| new_error!("shm not set"))?,
                                         configuration.outb_handler.clone(),
                                         #[cfg(gdb)]
                                         &debug_info,
                                     )?);
                                 }
-                                let hv = hv.as_mut().unwrap();
+                                let hv = hv.as_mut().ok_or_else(|| new_error!("Hypervisor not set"))?;
 
                                 #[cfg(target_os = "windows")]
                                 if !in_process {
@@ -386,7 +390,7 @@ impl HypervisorHandler {
                                 }
                             }
                             HypervisorHandlerAction::DispatchCallFromHost(function_name) => {
-                                let hv = hv.as_mut().unwrap();
+                                let hv = hv.as_mut().ok_or_else(|| new_error!("Hypervisor not initialized"))?;
 
                                 // Lock to indicate an action is being performed in the hypervisor
                                 execution_variables.running.store(true, Ordering::SeqCst);
@@ -647,6 +651,8 @@ impl HypervisorHandler {
                         // If the thread has finished, we try to join it and return the error if it has one
                         let res = handle.join();
                         if res.as_ref().is_ok_and(|inner_res| inner_res.is_err()) {
+                            #[allow(clippy::unwrap_used)]
+                            // We know that the thread has finished and that the inner result is an error, so we can safely unwrap the result and the contained err
                             return Err(res.unwrap().unwrap_err());
                         }
                         Err(HyperlightError::HypervisorHandlerMessageReceiveTimedout())
@@ -797,7 +803,8 @@ impl HypervisorHandler {
                 // partition handle only set when running in-hypervisor (not in-process)
                 unsafe {
                     WHvCancelRunVirtualProcessor(
-                        self.execution_variables.get_partition_handle()?.unwrap(), // safe unwrap
+                        #[allow(clippy::unwrap_used)]
+                        self.execution_variables.get_partition_handle()?.unwrap(), // safe unwrap as we checked is some
                         0,
                         0,
                     )

src/hyperlight_host/src/hypervisor/hypervisor_handler.rs

@@ -267,14 +267,26 @@ impl Drop for SurrogateProcessManager {
 lazy_static::lazy_static! {
     // see the large comment inside `SurrogateProcessManager` describing
     // our reasoning behind using `lazy_static`.
-    static ref SURROGATE_PROCESSES_MANAGER: SurrogateProcessManager =
-        SurrogateProcessManager::new().unwrap();
+    static ref SURROGATE_PROCESSES_MANAGER: std::result::Result<SurrogateProcessManager, &'static str> =
+        match SurrogateProcessManager::new() {
+            Ok(manager) => Ok(manager),
+            Err(e) => {
+                error!("Failed to create SurrogateProcessManager: {:?}", e);
+                Err("Failed to create SurrogateProcessManager")
+            }
+        };
 }
 
 /// Gets the singleton SurrogateProcessManager. This should be called when a new HyperV on Windows Driver is created.
 #[instrument(err(Debug), skip_all, parent = Span::current(), level= "Trace")]
 pub(crate) fn get_surrogate_process_manager() -> Result<&'static SurrogateProcessManager> {
-    Ok(&SURROGATE_PROCESSES_MANAGER)
+    match &*SURROGATE_PROCESSES_MANAGER {
+        Ok(manager) => Ok(manager),
+        Err(e) => {
+            error!("Failed to get SurrogateProcessManager: {:?}", e);
+            Err(new_error!("Failed to get SurrogateProcessManager {}", e))
+        }
+    }
 }
 
 // Creates a job object that will terminate all the surrogate processes when the struct instance is dropped.

src/hyperlight_host/src/hypervisor/surrogate_process_manager.rs

@@ -165,6 +165,8 @@ pub unsafe fn try_load_whv_map_gpa_range2() -> Result<WHvMapGpaRange2Func> {
         return Err(new_error!("{}", e));
     }
 
+    #[allow(clippy::unwrap_used)]
+    // We know this will succeed because we just checked for an error above
     let library = library.unwrap();
 
     let address = unsafe { GetProcAddress(library, s!("WHvMapGpaRange2")) };

src/hyperlight_host/src/hypervisor/windows_hypervisor_platform.rs

@@ -16,6 +16,7 @@ limitations under the License.
 
 #![cfg_attr(not(any(test, debug_assertions)), warn(clippy::panic))]
 #![cfg_attr(not(any(test, debug_assertions)), warn(clippy::expect_used))]
+#![cfg_attr(not(any(test, debug_assertions)), warn(clippy::unwrap_used))]
 
 use std::sync::Once;
 

src/hyperlight_host/src/lib.rs

@@ -52,20 +52,22 @@ impl ElfInfo {
         self.entry
     }
     pub(crate) fn get_base_va(&self) -> u64 {
+        #[allow(clippy::unwrap_used)] // guaranteed not to panic because of the check in new()
         let min_phdr = self
             .phdrs
             .iter()
             .find(|phdr| phdr.p_type == PT_LOAD)
-            .unwrap(); // guaranteed not to panic because of the check in new()
+            .unwrap();
         min_phdr.p_vaddr
     }
     pub(crate) fn get_va_size(&self) -> usize {
+        #[allow(clippy::unwrap_used)] // guaranteed not to panic because of the check in new()
         let max_phdr = self
             .phdrs
             .iter()
             .rev()
             .find(|phdr| phdr.p_type == PT_LOAD)
-            .unwrap(); // guaranteed not to panic because of the check in new()
+            .unwrap();
         (max_phdr.p_vaddr + max_phdr.p_memsz - self.get_base_va()) as usize
     }
     pub(crate) fn load_at(&self, load_addr: usize, target: &mut [u8]) -> Result<()> {

src/hyperlight_host/src/mem/elf.rs

@@ -54,7 +54,7 @@ impl LoadedLib {
         // arc reference is dropped, but before the destructor is executed. This however
         // is ok, as it means that the old library is not going to be used anymore and
         // we can use it instead.
-        let mut lock = LOADED_LIB.lock().unwrap();
+        let mut lock = LOADED_LIB.lock()?;
         if lock.upgrade().is_some() {
             // An owning copy of the loaded library still exists somewhere,
             // we can't load a new library yet

src/hyperlight_host/src/mem/loaded_lib.rs

@@ -202,6 +202,8 @@ impl MemoryRegionVecBuilder {
             return guest_end - self.guest_base_phys_addr;
         }
 
+        #[allow(clippy::unwrap_used)]
+        // we know this is safe because we check if the regions are empty above
         let last_region = self.regions.last().unwrap();
         let new_region = MemoryRegion {
             guest_region: last_region.guest_region.end..last_region.guest_region.end + size,

src/hyperlight_host/src/mem/memory_region.rs

@@ -300,6 +300,7 @@ where
         if last.is_none() {
             log_then_return!(NoMemorySnapshot);
         }
+        #[allow(clippy::unwrap_used)] // We know that last is not None because we checked it above
         let snapshot = last.unwrap();
         snapshot.restore_from_snapshot(&mut self.shared_mem)
     }

src/hyperlight_host/src/mem/mgr.rs

@@ -91,6 +91,7 @@ impl<'a> Iterator for BaseRelocations<'a> {
         let word = u16::from_le_bytes(block);
 
         // Shift the word over so that we are only reading a number from the first (most significant) 4 bits
+        #[allow(clippy::unwrap_used)] // this is safe as we are only reading 4 bits from a u16
         let typ = u8::try_from(word >> 12).unwrap();
 
         // Set the first 4 bits to 0 so that we only use the lower 12 bits for the location of the RVA in the PE file