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 0d9399b commit cd57391
Show file tree
Hide file tree
Showing 10 changed files with 25 additions and 14 deletions.
5 changes: 3 additions & 2 deletions detcore/tests/lit/child_should_inherit_fds/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

use std::os::fd::AsRawFd;

use close_err::Closable;
use nix::sys::wait::waitpid;
use nix::sys::wait::WaitStatus;
use nix::unistd::fork;
Expand All @@ -25,7 +26,7 @@ fn main() {
let msg: [u8; 8] = [0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8];
match unsafe { fork().unwrap() } {
ForkResult::Parent { child, .. } => {
drop(fdwrite);
fdwrite.close().expect("close failed");
let mut buf: [u8; 8] = [0; 8];
// XXX: The following SYS_read would timeout due to detcore issue.
// add a 5 seconds timeout to abort.
Expand All @@ -36,7 +37,7 @@ fn main() {
unsafe { libc::syscall(libc::SYS_exit_group, 0) };
}
ForkResult::Child => {
drop(fdread);
fdread.close().expect("close failed");
assert_eq!(write(&fdwrite, &msg), Ok(8));
unsafe { libc::syscall(libc::SYS_exit_group, 0) };
}
Expand Down
5 changes: 3 additions & 2 deletions detcore/tests/lit/close_on_exec/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use std::os::fd::AsRawFd;
use std::ptr;

use close_err::Closable;
use nix::errno::Errno;
use nix::fcntl::OFlag;
use nix::sys::stat::fstat;
Expand Down Expand Up @@ -46,7 +47,7 @@ fn main() {

match unsafe { fork().unwrap() } {
ForkResult::Parent { child, .. } => {
drop(fdwrite);
fdwrite.close().expect("close failed");

let mut msg = [0u8; 4];

Expand All @@ -59,7 +60,7 @@ fn main() {
assert_eq!(waitpid(child, None), Ok(WaitStatus::Exited(child, 0)));
}
ForkResult::Child => {
drop(fdread);
fdread.close().expect("close failed");

let proc_self = "/proc/self/exe\0".as_ptr() as *const libc::c_char;

Expand Down
5 changes: 3 additions & 2 deletions detcore/tests/lit/dup2_existing_fd/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// RUN: %me
use std::os::fd::AsRawFd;

use close_err::Closable;
use nix::sys::wait::waitpid;
use nix::sys::wait::WaitStatus;
use nix::unistd::alarm;
Expand All @@ -26,7 +27,7 @@ fn main() {

match unsafe { fork().unwrap() } {
ForkResult::Parent { child, .. } => {
drop(fdwrite);
fdwrite.close().expect("close failed");
let mut buf = [0u8; 8];

// If the file descriptor newfd was previously open,
Expand All @@ -45,7 +46,7 @@ fn main() {
assert_eq!(waitpid(child, None), Ok(WaitStatus::Exited(child, 0)));
}
ForkResult::Child => {
drop(fdread);
fdread.close().expect("close failed");
assert_eq!(write(fdwrite, &msg), Ok(msg.len()));
}
}
Expand Down
5 changes: 3 additions & 2 deletions detcore/tests/lit/no_close_on_exec/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use std::os::fd::AsRawFd;
use std::os::fd::BorrowedFd;
use std::ptr;

use close_err::Closable;
use nix::fcntl::OFlag;
use nix::sys::wait::waitpid;
use nix::sys::wait::WaitStatus;
Expand Down Expand Up @@ -49,7 +50,7 @@ fn main() {

match unsafe { fork().unwrap() } {
ForkResult::Parent { child, .. } => {
drop(fdwrite);
fdwrite.close().expect("close failed");

let mut msg = [0u8; 6];

Expand All @@ -60,7 +61,7 @@ fn main() {
assert_eq!(waitpid(child, None), Ok(WaitStatus::Exited(child, 0)));
}
ForkResult::Child => {
drop(fdread);
fdread.close().expect("close failed");

let proc_self = "/proc/self/exe\0".as_ptr() as *const libc::c_char;

Expand Down
3 changes: 2 additions & 1 deletion detcore/tests/lit/openat_next_lowest_fd/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// RUN: %me
use std::os::fd::AsRawFd;

use close_err::Closable;
use nix::fcntl::openat;
use nix::fcntl::OFlag;
use nix::sys::stat::Mode;
Expand All @@ -18,7 +19,7 @@ fn main() {
assert_eq!(fd4.as_raw_fd(), fd3.as_raw_fd() + 1);

let fd3_raw = fd3.as_raw_fd();
drop(fd3);
fd3.close().expect("close failed");

assert_eq!(
openat(None, "/dev/null", OFlag::O_RDONLY, Mode::S_IRUSR),
Expand Down
5 changes: 3 additions & 2 deletions detcore/tests/lit/pipe_creates_valid_fds/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@
// RUN: %me
use std::os::fd::AsRawFd;

use close_err::Closable;
use nix::sys::stat::fstat;
use nix::unistd::pipe;

fn main() {
let (fd_read, fd_write) = pipe().unwrap();
assert!(fstat(fd_read.as_raw_fd()).is_ok());
assert!(fstat(fd_write.as_raw_fd()).is_ok());
drop(fd_write);
drop(fd_read);
fd_write.close().expect("close failed");
fd_read.close().expect("close failed");
}
1 change: 1 addition & 0 deletions tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ name = "standalone_stacktrace_events"
path = "standalone/stacktrace_events.rs"

[dependencies]
close-err = "1.0.2"
detcore = { version = "0.0.0", path = "../detcore" }
libc = "0.2.139"
nix = { version = "0.29.0", features = ["dir", "event", "hostname", "inotify", "ioctl", "mman", "mount", "net", "poll", "ptrace", "reboot", "resource", "sched", "term", "time", "user", "zerocopy"] }
Expand Down
1 change: 1 addition & 0 deletions tests/helpers.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ def hermit_rust_test(path, raw, run, no_sequentialize_threads, no_deterministic_
srcs = [path, paths.dirname(path) + "/test_utils/mod.rs"],
crate_root = path,
deps = [
"fbsource//third-party/rust:close-err",
"fbsource//third-party/rust:libc",
"fbsource//third-party/rust:nix",
"fbsource//third-party/rust:tempfile",
Expand Down
5 changes: 3 additions & 2 deletions tests/rust/pipe_basics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
use std::os::fd::AsRawFd;
use std::str;

use close_err::Closable;
use nix::unistd;

fn main() {
Expand All @@ -19,15 +20,15 @@ fn main() {
assert_eq!(unistd::read(fdread.as_raw_fd(), &mut buf), Ok(14));
println!("Child received message: {}", str::from_utf8(&buf).unwrap());
}
drop(fdread);
fdread.close().expect("close failed");
});

for ix in 10..20 {
println!("Parent writing to pipe..");
let msg = format!("hello world {}", ix);
assert_eq!(unistd::write(&fdwrite, msg.as_bytes()), Ok(14));
}
drop(fdwrite);
fdwrite.close().expect("close failed");

println!("Joining child..");
handle.join().unwrap();
Expand Down
4 changes: 3 additions & 1 deletion tests/rust/poll_spin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use std::sync::Arc;
use std::thread;
use std::time;

use close_err::Closable;
use nix::poll::poll;
use nix::poll::PollFd;
use nix::poll::PollFlags;
Expand Down Expand Up @@ -64,6 +65,7 @@ fn main() {
break;
}
}
fdread.close().expect("close failed");
});

while count2.load(Ordering::SeqCst) == 0 {
Expand All @@ -73,7 +75,7 @@ fn main() {
println!("Parent writing to pipe..");
let msg = "hello world\n";
assert_eq!(unistd::write(&fdwrite, msg.as_bytes()), Ok(12));
drop(fdwrite);
fdwrite.close().expect("close failed");

println!("Joining child..");
handle.join().unwrap();
Expand Down

0 comments on commit cd57391

Please sign in to comment.