bring back close() error checking

Summary:
In stack ending at D66275420, several call sites were migrated from
RawFd with explicit `libc::close()` or `nix::unistd::close()` which
admit the possibility of failure. Now, `close()` failures are
unactionable -- you certainly cannot retry the close operation or risk
closing some other FD -- but it may be worth logging when they happen.

Bring back explicit close operations and error checking.

Reviewed By: jasonwhite

Differential Revision: D67157487

fbshipit-source-id: 254cace779c6f993117fbaaf6e0453df6bdc70e5
This commit is contained in:
Chad Austin 2024-12-18 10:53:38 -08:00 committed by Facebook GitHub Bot
parent 1a1bfa4c40
commit bd319a7711
3 changed files with 11 additions and 8 deletions

View file

@ -13,6 +13,7 @@ anyhow = "1.0.86"
async-trait = "0.1.71" async-trait = "0.1.71"
bincode = "1.3.3" bincode = "1.3.3"
bytes = { version = "1.6.0", features = ["serde"] } bytes = { version = "1.6.0", features = ["serde"] }
close-err = "1.0.2"
futures = { version = "0.3.30", features = ["async-await", "compat"] } futures = { version = "0.3.30", features = ["async-await", "compat"] }
goblin = "0.5.2" goblin = "0.5.2"
iced-x86 = "1.17.0" iced-x86 = "1.17.0"

View file

@ -16,6 +16,7 @@ use std::path::PathBuf;
use std::sync::Arc; use std::sync::Arc;
use anyhow::Context; use anyhow::Context;
use close_err::Closable;
use futures::future; use futures::future;
use futures::future::BoxFuture; use futures::future::BoxFuture;
use futures::future::Either; use futures::future::Either;
@ -592,13 +593,13 @@ where
// panicking, etc). We make a best-effort attempt to solve some of these issues. // panicking, etc). We make a best-effort attempt to solve some of these issues.
match unsafe { unistd::fork() }.expect("unistd::fork failed") { match unsafe { unistd::fork() }.expect("unistd::fork failed") {
ForkResult::Child => { ForkResult::Child => {
drop(read1); read1.close()?;
drop(read2); read2.close()?;
if capture_output { if capture_output {
unistd::dup2(write1.as_raw_fd(), 1).map_err(from_nix_error)?; unistd::dup2(write1.as_raw_fd(), 1).map_err(from_nix_error)?;
unistd::dup2(write2.as_raw_fd(), 2).map_err(from_nix_error)?; unistd::dup2(write2.as_raw_fd(), 2).map_err(from_nix_error)?;
drop(write1); write1.close()?;
drop(write2); write2.close()?;
} }
init_tracee(events.has_rdtsc()).expect("init_tracee failed"); init_tracee(events.has_rdtsc()).expect("init_tracee failed");
@ -625,8 +626,8 @@ where
let guest_pid = Pid::from(child); let guest_pid = Pid::from(child);
let child = Running::new(guest_pid); let child = Running::new(guest_pid);
drop(write1); write1.close()?;
drop(write2); write2.close()?;
let stdout = read1.into(); let stdout = read1.into();
let stderr = read2.into(); let stderr = read2.into();

View file

@ -17,6 +17,7 @@ use std::os::fd::AsRawFd;
use std::sync::atomic::AtomicU64; use std::sync::atomic::AtomicU64;
use std::sync::atomic::Ordering; use std::sync::atomic::Ordering;
use close_err::Closable;
#[allow(unused_imports)] #[allow(unused_imports)]
use nix::sys::wait; use nix::sys::wait;
#[allow(unused_imports)] #[allow(unused_imports)]
@ -267,7 +268,7 @@ fn child_should_inherit_fds() {
let msg: [u8; 8] = [0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8]; let msg: [u8; 8] = [0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8];
match unsafe { unistd::fork() } { match unsafe { unistd::fork() } {
Ok(ForkResult::Parent { child, .. }) => { Ok(ForkResult::Parent { child, .. }) => {
drop(fdwrite); fdwrite.close().expect("close failed");
let mut buf: [u8; 8] = [0; 8]; let mut buf: [u8; 8] = [0; 8];
assert_eq!(unistd::read(fdread.as_raw_fd(), &mut buf), Ok(8)); assert_eq!(unistd::read(fdread.as_raw_fd(), &mut buf), Ok(8));
assert_eq!(buf, msg); assert_eq!(buf, msg);
@ -276,7 +277,7 @@ fn child_should_inherit_fds() {
unreachable!(); unreachable!();
} }
Ok(ForkResult::Child) => { Ok(ForkResult::Child) => {
drop(fdread); fdread.close().expect("close failed");
assert_eq!(unistd::write(&fdwrite, &msg), Ok(8)); assert_eq!(unistd::write(&fdwrite, &msg), Ok(8));
unsafe { libc::syscall(libc::SYS_exit_group, 0) }; unsafe { libc::syscall(libc::SYS_exit_group, 0) };
unreachable!(); unreachable!();