Skip to content

Commit

Permalink
bring back close() error checking
Browse files Browse the repository at this point in the history
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
  • Loading branch information
chadaustin authored and facebook-github-bot committed Dec 18, 2024
1 parent 1a1bfa4 commit bd319a7
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 8 deletions.
1 change: 1 addition & 0 deletions reverie-ptrace/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ anyhow = "1.0.86"
async-trait = "0.1.71"
bincode = "1.3.3"
bytes = { version = "1.6.0", features = ["serde"] }
close-err = "1.0.2"
futures = { version = "0.3.30", features = ["async-await", "compat"] }
goblin = "0.5.2"
iced-x86 = "1.17.0"
Expand Down
13 changes: 7 additions & 6 deletions reverie-ptrace/src/tracer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use std::path::PathBuf;
use std::sync::Arc;

use anyhow::Context;
use close_err::Closable;
use futures::future;
use futures::future::BoxFuture;
use futures::future::Either;
Expand Down Expand Up @@ -592,13 +593,13 @@ where
// panicking, etc). We make a best-effort attempt to solve some of these issues.
match unsafe { unistd::fork() }.expect("unistd::fork failed") {
ForkResult::Child => {
drop(read1);
drop(read2);
read1.close()?;
read2.close()?;
if capture_output {
unistd::dup2(write1.as_raw_fd(), 1).map_err(from_nix_error)?;
unistd::dup2(write2.as_raw_fd(), 2).map_err(from_nix_error)?;
drop(write1);
drop(write2);
write1.close()?;
write2.close()?;
}

init_tracee(events.has_rdtsc()).expect("init_tracee failed");
Expand All @@ -625,8 +626,8 @@ where

let guest_pid = Pid::from(child);
let child = Running::new(guest_pid);
drop(write1);
drop(write2);
write1.close()?;
write2.close()?;

let stdout = read1.into();
let stderr = read2.into();
Expand Down
5 changes: 3 additions & 2 deletions tests/basics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use std::os::fd::AsRawFd;
use std::sync::atomic::AtomicU64;
use std::sync::atomic::Ordering;

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

0 comments on commit bd319a7

Please sign in to comment.