fix: do not double close file descriptors in unittests

With newer rust toolchains, rust will SIGABRT if an already closed file
descriptor is closed in the the `Drop` implementation for `File`. This
also applies to creating `File`s with invalid file descriptors. Thus fix
tests that accidentally double-close fds, and remove those that
explicitly construct `File`s with invalid file descriptors (they are
redundant now anyway, because Rust would abort if this ever happened).

Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

src/vmm/src/arch/aarch64/gic/gicv2/regs/dist_regs.rs

@@ -162,5 +162,8 @@ mod tests {
             format!("{:?}", res.unwrap_err()),
             "DeviceAttribute(Error(9), false, 1)"
         );
+
+        // dropping gic_fd would double close the gic fd, so leak it
+        std::mem::forget(gic_fd);
     }
 }

src/vmm/src/arch/aarch64/gic/gicv2/regs/icc_regs.rs

@@ -120,5 +120,8 @@ mod tests {
             format!("{:?}", res.unwrap_err()),
             "DeviceAttribute(Error(9), false, 2)"
         );
+
+        // dropping gic_fd would double close the gic fd, so leak it
+        std::mem::forget(gic_fd);
     }
 }

src/vmm/src/arch/aarch64/gic/gicv3/mod.rs

@@ -220,5 +220,8 @@ mod tests {
             format!("{:?}", res.unwrap_err()),
             "DeviceAttribute(Error(9), true, 4)"
         );
+
+        // dropping gic_fd would double close the gic fd, so leak it
+        std::mem::forget(gic);
     }
 }

src/vmm/src/arch/aarch64/gic/gicv3/regs/dist_regs.rs

@@ -159,6 +159,9 @@ mod tests {
             format!("{:?}", res.unwrap_err()),
             "DeviceAttribute(Error(9), false, 1)"
         );
+
+        // dropping gic_fd would double close the gic fd, so leak it
+        std::mem::forget(gic_fd);
     }
 
     #[test]

src/vmm/src/arch/aarch64/gic/gicv3/regs/icc_regs.rs

@@ -206,6 +206,9 @@ mod tests {
             format!("{:?}", res.unwrap_err()),
             "DeviceAttribute(Error(9), false, 6)"
         );
+
+        // dropping gic_fd would double close the gic fd, so leak it
+        std::mem::forget(gic_fd);
     }
 
     #[test]

src/vmm/src/arch/aarch64/gic/gicv3/regs/redist_regs.rs

@@ -120,5 +120,8 @@ mod tests {
             format!("{:?}", res.unwrap_err()),
             "DeviceAttribute(Error(9), false, 5)"
         );
+
+        // dropping gic_fd would double close the gic fd, so leak it
+        std::mem::forget(gic_fd);
     }
 }

src/vmm/src/arch/aarch64/vcpu.rs

@@ -299,5 +299,8 @@ mod tests {
 
         let res = set_mpstate(&vcpu, kvm_mp_state::default());
         assert!(matches!(res, Err(VcpuError::SetMp(_))), "{:?}", res);
+
+        // dropping vcpu would double close the fd, so leak it
+        std::mem::forget(vcpu);
     }
 }

src/vmm/src/devices/virtio/block/virtio/io/mod.rs

@@ -187,7 +187,6 @@ impl<T: Debug> FileEngine<T> {
 pub mod tests {
     #![allow(clippy::undocumented_unsafe_blocks)]
     use std::os::unix::ffi::OsStrExt;
-    use std::os::unix::io::FromRawFd;
 
     use vmm_sys_util::tempfile::TempFile;
 
@@ -201,20 +200,6 @@ pub mod tests {
     // 2 pages of memory should be enough to test read/write ops and also dirty tracking.
     const MEM_LEN: usize = 8192;
 
-    macro_rules! assert_err {
-        ($expression:expr, $($pattern:tt)+) => {
-            match $expression {
-                Err(UserDataError {
-                    user_data: _,
-                    error: $($pattern)+,
-                }) => (),
-                ref err => {
-                    panic!("expected `{}` but got `{:?}`", stringify!($($pattern)+), err);
-                }
-            }
-        };
-    }
-
     macro_rules! assert_sync_execution {
         ($expression:expr, $count:expr) => {
             match $expression {
@@ -265,17 +250,7 @@ pub mod tests {
 
     #[test]
     fn test_sync() {
-        // Check invalid file
         let mem = create_mem();
-        let file = unsafe { File::from_raw_fd(-2) };
-        let mut engine = FileEngine::from_file(file, FileEngineType::Sync).unwrap();
-        let res = engine.read(0, &mem, GuestAddress(0), 0, ());
-        assert_err!(res, BlockIoError::Sync(sync_io::SyncIoError::Seek(_e)));
-        let res = engine.write(0, &mem, GuestAddress(0), 0, ());
-        assert_err!(res, BlockIoError::Sync(sync_io::SyncIoError::Seek(_e)));
-        let res = engine.flush(());
-        assert_err!(res, BlockIoError::Sync(sync_io::SyncIoError::SyncAll(_e)));
-
         // Create backing file.
         let file = TempFile::new().unwrap().into_file();
         let mut engine = FileEngine::from_file(file, FileEngineType::Sync).unwrap();
@@ -342,10 +317,6 @@ pub mod tests {
 
     #[test]
     fn test_async() {
-        // Check invalid file
-        let file = unsafe { File::from_raw_fd(-2) };
-        FileEngine::<()>::from_file(file, FileEngineType::Async).unwrap_err();
-
         // Create backing file.
         let file = TempFile::new().unwrap().into_file();
         let mut engine = FileEngine::<()>::from_file(file, FileEngineType::Async).unwrap();

src/vmm/src/devices/virtio/net/device.rs

@@ -1808,6 +1808,9 @@ pub mod tests {
         assert_eq!(th.txq.used.idx.get(), 1);
         assert!(&th.net().irq_trigger.has_pending_irq(IrqType::Vring));
         th.txq.check_used_elem(0, 0, 0);
+
+        // dropping th would double close the tap fd, so leak it
+        std::mem::forget(th);
     }
 
     #[test]
@@ -2041,6 +2044,9 @@ pub mod tests {
             1,
             th.simulate_event(NetEvent::Tap)
         );
+
+        // dropping th would double close the tap fd, so leak it
+        std::mem::forget(th);
     }
 
     #[test]

src/vmm/src/devices/virtio/net/tap.rs

@@ -277,19 +277,6 @@ pub mod tests {
         let tap = Tap::open_named("").unwrap();
         tap.set_vnet_hdr_size(16).unwrap();
         tap.set_offload(0).unwrap();
-
-        let faulty_tap = Tap {
-            tap_file: unsafe { File::from_raw_fd(-2) },
-            if_name: [0x01; 16],
-        };
-        assert_eq!(
-            faulty_tap.set_vnet_hdr_size(16).unwrap_err().to_string(),
-            TapError::SetSizeOfVnetHdr(IoError::from_raw_os_error(9)).to_string()
-        );
-        assert_eq!(
-            faulty_tap.set_offload(0).unwrap_err().to_string(),
-            TapError::SetOffloadFlags(IoError::from_raw_os_error(9)).to_string()
-        );
     }
 
     #[test]

src/vmm/src/vstate/vcpu/aarch64.rs

@@ -329,6 +329,9 @@ mod tests {
             err.err().unwrap().to_string(),
             "Error creating vcpu: Bad file descriptor (os error 9)".to_string()
         );
+
+        // dropping vm would double close the gic fd, so leak it
+        std::mem::forget(vm);
     }
 
     #[test]
@@ -361,6 +364,9 @@ mod tests {
                 kvm_ioctls::Error::new(9)
             ))
         );
+
+        // dropping vcpu would double close the gic fd, so leak it
+        std::mem::forget(vcpu);
     }
 
     #[test]

src/vmm/src/vstate/vcpu/x86_64.rs

@@ -715,8 +715,6 @@ impl Debug for VcpuState {
 mod tests {
     #![allow(clippy::undocumented_unsafe_blocks)]
 
-    use std::os::unix::io::AsRawFd;
-
     use kvm_bindings::kvm_msr_entry;
     use kvm_ioctls::{Cap, Kvm};
 
@@ -874,7 +872,7 @@ mod tests {
         // Restore the state into the existing vcpu.
         let result1 = vcpu.restore_state(&state);
         assert!(result1.is_ok(), "{}", result1.unwrap_err());
-        unsafe { libc::close(vcpu.fd.as_raw_fd()) };
+        drop(vcpu);
 
         // Restore the state into a new vcpu.
         let (_vm, vcpu, _mem) = setup_vcpu(0x10000);