From bb0e7005ec28f89f0ce8508046223fa2628295fc Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Tue, 10 Dec 2024 11:40:22 +0000 Subject: [PATCH] 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 --- .../arch/aarch64/gic/gicv2/regs/dist_regs.rs | 3 ++ .../arch/aarch64/gic/gicv2/regs/icc_regs.rs | 3 ++ src/vmm/src/arch/aarch64/gic/gicv3/mod.rs | 3 ++ .../arch/aarch64/gic/gicv3/regs/dist_regs.rs | 3 ++ .../arch/aarch64/gic/gicv3/regs/icc_regs.rs | 3 ++ .../aarch64/gic/gicv3/regs/redist_regs.rs | 3 ++ src/vmm/src/arch/aarch64/vcpu.rs | 3 ++ .../src/devices/virtio/block/virtio/io/mod.rs | 29 ------------------- src/vmm/src/devices/virtio/net/device.rs | 6 ++++ src/vmm/src/devices/virtio/net/tap.rs | 13 --------- src/vmm/src/vstate/vcpu/aarch64.rs | 6 ++++ src/vmm/src/vstate/vcpu/x86_64.rs | 4 +-- 12 files changed, 34 insertions(+), 45 deletions(-) diff --git a/src/vmm/src/arch/aarch64/gic/gicv2/regs/dist_regs.rs b/src/vmm/src/arch/aarch64/gic/gicv2/regs/dist_regs.rs index 3ec73490038..a4179e895ae 100644 --- a/src/vmm/src/arch/aarch64/gic/gicv2/regs/dist_regs.rs +++ b/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); } } diff --git a/src/vmm/src/arch/aarch64/gic/gicv2/regs/icc_regs.rs b/src/vmm/src/arch/aarch64/gic/gicv2/regs/icc_regs.rs index aea6cb722b6..be963a8327e 100644 --- a/src/vmm/src/arch/aarch64/gic/gicv2/regs/icc_regs.rs +++ b/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); } } diff --git a/src/vmm/src/arch/aarch64/gic/gicv3/mod.rs b/src/vmm/src/arch/aarch64/gic/gicv3/mod.rs index 21f9b94218d..50d2e5130db 100644 --- a/src/vmm/src/arch/aarch64/gic/gicv3/mod.rs +++ b/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); } } diff --git a/src/vmm/src/arch/aarch64/gic/gicv3/regs/dist_regs.rs b/src/vmm/src/arch/aarch64/gic/gicv3/regs/dist_regs.rs index 6e0cc8aac23..5af3e9215c0 100644 --- a/src/vmm/src/arch/aarch64/gic/gicv3/regs/dist_regs.rs +++ b/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] diff --git a/src/vmm/src/arch/aarch64/gic/gicv3/regs/icc_regs.rs b/src/vmm/src/arch/aarch64/gic/gicv3/regs/icc_regs.rs index e455e76ba43..d242bce8433 100644 --- a/src/vmm/src/arch/aarch64/gic/gicv3/regs/icc_regs.rs +++ b/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] diff --git a/src/vmm/src/arch/aarch64/gic/gicv3/regs/redist_regs.rs b/src/vmm/src/arch/aarch64/gic/gicv3/regs/redist_regs.rs index 1909d9b453b..88af82e3cb2 100644 --- a/src/vmm/src/arch/aarch64/gic/gicv3/regs/redist_regs.rs +++ b/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); } } diff --git a/src/vmm/src/arch/aarch64/vcpu.rs b/src/vmm/src/arch/aarch64/vcpu.rs index 5c08f95351a..80fc5a339df 100644 --- a/src/vmm/src/arch/aarch64/vcpu.rs +++ b/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); } } diff --git a/src/vmm/src/devices/virtio/block/virtio/io/mod.rs b/src/vmm/src/devices/virtio/block/virtio/io/mod.rs index de970986da2..09e86b6968d 100644 --- a/src/vmm/src/devices/virtio/block/virtio/io/mod.rs +++ b/src/vmm/src/devices/virtio/block/virtio/io/mod.rs @@ -187,7 +187,6 @@ impl FileEngine { 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(); diff --git a/src/vmm/src/devices/virtio/net/device.rs b/src/vmm/src/devices/virtio/net/device.rs index a17c25d7248..99941f5796a 100755 --- a/src/vmm/src/devices/virtio/net/device.rs +++ b/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] diff --git a/src/vmm/src/devices/virtio/net/tap.rs b/src/vmm/src/devices/virtio/net/tap.rs index 0f5b7e2d788..776b5ba960e 100644 --- a/src/vmm/src/devices/virtio/net/tap.rs +++ b/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] diff --git a/src/vmm/src/vstate/vcpu/aarch64.rs b/src/vmm/src/vstate/vcpu/aarch64.rs index 2e2bb36fb07..4e006f196d0 100644 --- a/src/vmm/src/vstate/vcpu/aarch64.rs +++ b/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] diff --git a/src/vmm/src/vstate/vcpu/x86_64.rs b/src/vmm/src/vstate/vcpu/x86_64.rs index a1bb22d1bb7..4043691130d 100644 --- a/src/vmm/src/vstate/vcpu/x86_64.rs +++ b/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);