From 05c592b814f4f7cc87a4c70f6265be2639fc709a Mon Sep 17 00:00:00 2001 From: Ethan Pailes Date: Tue, 2 Apr 2024 15:11:53 +0000 Subject: [PATCH] add borrow_fd method to Slave and Master This should allow users to get a safer type to use for shipping around an fd. This is required in order to use the most modern nix version for some of the APIs that shpool uses. Also adding CI in this patch since it is missing. --- .github/workflows/nightly.yml | 20 +++++++++++++ .github/workflows/presubmit.yml | 51 ++++++++++++++++++++++++++++++++ deny.toml | 18 ++++++++++++ examples/main.rs | 8 ++--- examples/new.rs | 21 ++++++++----- rustfmt.toml | 2 -- src/descriptor/mod.rs | 16 ++++++---- src/fork/err.rs | 2 +- src/fork/mod.rs | 52 ++++++++++++++++----------------- src/fork/pty/master/err.rs | 3 +- src/fork/pty/master/mod.rs | 22 ++++++++------ src/fork/pty/slave/err.rs | 2 +- src/fork/pty/slave/mod.rs | 14 +++++++-- src/lib.rs | 20 ++----------- tests/it_can_read_write.rs | 10 +++---- tests/it_fork_with_new_pty.rs | 16 +++++----- tests/lib.rs | 2 +- 17 files changed, 187 insertions(+), 92 deletions(-) create mode 100644 .github/workflows/nightly.yml create mode 100644 .github/workflows/presubmit.yml create mode 100644 deny.toml delete mode 100644 rustfmt.toml diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml new file mode 100644 index 0000000..6b7ab5c --- /dev/null +++ b/.github/workflows/nightly.yml @@ -0,0 +1,20 @@ +name: nightly +on: + schedule: + - cron: '04 05 * * *' + +jobs: + deny: + name: cargo deny --all-features check + runs-on: ubuntu-22.04 + steps: + - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 + - uses: moonrepo/setup-rust@b8edcc56aab474d90c7cf0bb8beeaf8334c15e9f + with: + channel: '1.74.0' + bins: cargo-deny + - run: sudo apt-get install libpam0g-dev + - run: cargo deny --all-features check + + postsubmit: + uses: ./.github/workflows/presubmit.yml diff --git a/.github/workflows/presubmit.yml b/.github/workflows/presubmit.yml new file mode 100644 index 0000000..8ce2ead --- /dev/null +++ b/.github/workflows/presubmit.yml @@ -0,0 +1,51 @@ +name: presubmit +on: [pull_request, workflow_call, workflow_dispatch] + +jobs: + test: + name: cargo test --all-features + runs-on: ubuntu-22.04 + steps: + - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 + - uses: moonrepo/setup-rust@b8edcc56aab474d90c7cf0bb8beeaf8334c15e9f + with: + channel: '1.74.0' + - run: sudo apt-get install libpam0g-dev + - run: cargo test --all-features + - run: cargo test --no-default-features + + rustfmt: + name: cargo +nightly fmt -- --check + runs-on: ubuntu-22.04 + steps: + - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 + - uses: moonrepo/setup-rust@b8edcc56aab474d90c7cf0bb8beeaf8334c15e9f + with: + components: rustfmt + channel: nightly + - run: cargo +nightly fmt -- --check + + cranky: + name: cargo +nightly cranky --all-targets -- -D warnings + runs-on: ubuntu-22.04 + steps: + - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 + - uses: moonrepo/setup-rust@b8edcc56aab474d90c7cf0bb8beeaf8334c15e9f + with: + components: clippy + bins: cargo-cranky@0.3.0 + channel: nightly + - run: sudo apt-get install libpam0g-dev + - run: cargo +nightly cranky --all-targets -- -D warnings + + deny: + name: cargo deny --all-features check licenses + runs-on: ubuntu-22.04 + steps: + - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 + - uses: moonrepo/setup-rust@b8edcc56aab474d90c7cf0bb8beeaf8334c15e9f + with: + channel: '1.74.0' + bins: cargo-deny + - run: sudo apt-get install libpam0g-dev + - run: cargo deny --all-features check licenses diff --git a/deny.toml b/deny.toml new file mode 100644 index 0000000..0ba9b55 --- /dev/null +++ b/deny.toml @@ -0,0 +1,18 @@ +[graph] +all-features = true + +[advisories] +version = 2 +db-path = "~/.cargo/advisory-db" +db-urls = ["https://github.com/rustsec/advisory-db"] +yanked = "deny" +ignore = [ +] + +[licenses] +allow = [ + "Apache-2.0", + "MIT", + "Unicode-DFS-2016", +] +confidence-threshold = 1.0 diff --git a/examples/main.rs b/examples/main.rs index 1becaff..21d6152 100644 --- a/examples/main.rs +++ b/examples/main.rs @@ -1,15 +1,15 @@ -extern crate shpool_pty; -extern crate libc; extern crate errno; +extern crate libc; +extern crate shpool_pty; use shpool_pty::fork::*; use std::io::Read; -use std::process::{Command}; +use std::process::Command; fn main() { let fork = Fork::from_ptmx().unwrap(); - if let Some(mut master) = fork.is_parent().ok() { + if let Ok(mut master) = fork.is_parent() { // Read output via PTY master let mut output = String::new(); diff --git a/examples/new.rs b/examples/new.rs index 51b3931..d78bc9e 100644 --- a/examples/new.rs +++ b/examples/new.rs @@ -1,5 +1,5 @@ -extern crate shpool_pty; extern crate libc; +extern crate shpool_pty; use self::shpool_pty::prelude::*; @@ -9,10 +9,12 @@ use std::process::{Command, Stdio}; fn main() { let fork = Fork::from_ptmx().unwrap(); - if let Some(mut master) = fork.is_parent().ok() { + if let Ok(mut master) = fork.is_parent() { let mut string = String::new(); - master.read_to_string(&mut string).unwrap_or_else(|e| panic!("{}", e)); + master + .read_to_string(&mut string) + .unwrap_or_else(|e| panic!("{}", e)); let output = Command::new("tty") .stdin(Stdio::inherit()) @@ -24,12 +26,17 @@ fn main() { let parent_tty = output_str.trim(); let child_tty = string.trim(); - println!("child_tty(\"{}\")[{}] != \"{}\" => {}", child_tty, child_tty.len(), "", child_tty != ""); - assert!(child_tty != ""); + println!( + "child_tty(\"{}\")[{}] != \"\" => {}", + child_tty, + child_tty.len(), + !child_tty.is_empty() + ); + assert!(!child_tty.is_empty()); assert!(child_tty != parent_tty); - let mut parent_tty_dir: Vec<&str> = parent_tty.split("/").collect(); - let mut child_tty_dir: Vec<&str> = child_tty.split("/").collect(); + let mut parent_tty_dir: Vec<&str> = parent_tty.split('/').collect(); + let mut child_tty_dir: Vec<&str> = child_tty.split('/').collect(); parent_tty_dir.pop(); child_tty_dir.pop(); diff --git a/rustfmt.toml b/rustfmt.toml deleted file mode 100644 index 5df0356..0000000 --- a/rustfmt.toml +++ /dev/null @@ -1,2 +0,0 @@ -tabs_spaces=4 -hard_tabs=false diff --git a/src/descriptor/mod.rs b/src/descriptor/mod.rs index df5d3f8..ce92d85 100644 --- a/src/descriptor/mod.rs +++ b/src/descriptor/mod.rs @@ -1,21 +1,27 @@ mod err; -use ::libc; +use libc; pub use self::err::DescriptorError; use std::ffi::CStr; use std::os::unix::io::RawFd; +/// # Safety +/// +/// Implmentations MUST only return valid fds from +/// these methods. Callers MUST NOT use the fds once the +/// returning object has fallen out of scope. pub unsafe trait Descriptor { /// If the descriptor has a valid fd, return it fn take_raw_fd(&mut self) -> Option; /// The constructor function `open` opens the path /// and returns the fd. - fn open(path: &CStr, - flag: libc::c_int, - mode: Option) - -> Result { + fn open( + path: &CStr, + flag: libc::c_int, + mode: Option, + ) -> Result { // Safety: we've just ensured that path is non-null and the // other params are valid by construction. unsafe { diff --git a/src/fork/err.rs b/src/fork/err.rs index 60c3aae..f245cd3 100644 --- a/src/fork/err.rs +++ b/src/fork/err.rs @@ -1,4 +1,4 @@ -use ::descriptor::DescriptorError; +use descriptor::DescriptorError; use std::error::Error; use std::fmt; diff --git a/src/fork/mod.rs b/src/fork/mod.rs index b441303..5fffd9b 100644 --- a/src/fork/mod.rs +++ b/src/fork/mod.rs @@ -1,12 +1,12 @@ -mod pty; mod err; +mod pty; -use ::descriptor::Descriptor; +use descriptor::Descriptor; pub use self::err::{ForkError, Result}; pub use self::pty::{Master, MasterError}; pub use self::pty::{Slave, SlaveError}; -use ::libc; +use libc; use std::ffi::CStr; use std::ffi::CString; @@ -47,13 +47,14 @@ impl Fork { // Safety: ptsname_r returns a valid c string when it returns a 0 // (success) code, and we make double extra sure there is a null // terminator by adding one ourselves. - let name: &CStr = unsafe { CStr::from_ptr(name_ptr as *const libc::c_char) }; + let name: &CStr = + unsafe { CStr::from_ptr(name_ptr as *const libc::c_char) }; Fork::from_pts(name) } pid => Ok(Fork::Parent(pid, master)), } } - }, + } } } @@ -68,11 +69,11 @@ impl Fork { match Slave::new(ptsname) { Err(cause) => Err(ForkError::BadSlave(cause)), Ok(slave) => { - if let Some(cause) = slave.dup2(libc::STDIN_FILENO) + if let Some(cause) = slave.dup2(libc::STDIN_FILENO).err().or(slave + .dup2(libc::STDOUT_FILENO) .err() - .or(slave.dup2(libc::STDOUT_FILENO) - .err() - .or(slave.dup2(libc::STDERR_FILENO).err())) { + .or(slave.dup2(libc::STDERR_FILENO).err())) + { Err(ForkError::BadSlave(cause)) } else { Ok(Fork::Child(slave)) @@ -98,24 +99,22 @@ impl Fork { pub fn wait_for_exit(&self) -> Result<(libc::pid_t, Option)> { match *self { Fork::Child(_) => Err(ForkError::IsChild), - Fork::Parent(pid, _) => { - loop { - unsafe { - let mut status = 0; - match libc::waitpid(pid, &mut status, 0) { - 0 => continue, - -1 => return Err(ForkError::WaitpidFail), - _ => { - if libc::WIFEXITED(status) { - return Ok((pid, Some(libc::WEXITSTATUS(status)))); - } else { - return Ok((pid, None)); - } + Fork::Parent(pid, _) => loop { + unsafe { + let mut status = 0; + match libc::waitpid(pid, &mut status, 0) { + 0 => continue, + -1 => return Err(ForkError::WaitpidFail), + _ => { + if libc::WIFEXITED(status) { + return Ok((pid, Some(libc::WEXITSTATUS(status)))); + } else { + return Ok((pid, None)); } } } } - } + }, } } @@ -134,7 +133,7 @@ impl Fork { pub fn is_parent(&self) -> Result { match *self { Fork::Child(_) => Err(ForkError::IsChild), - Fork::Parent(_, ref master) => Ok(master.clone()), + Fork::Parent(_, ref master) => Ok(*master), } } @@ -150,9 +149,8 @@ impl Fork { impl Drop for Fork { fn drop(&mut self) { - match self { - Fork::Parent(_, master) => Descriptor::drop(master), - _ => {} + if let Fork::Parent(_, master) = self { + Descriptor::drop(master) } } } diff --git a/src/fork/pty/master/err.rs b/src/fork/pty/master/err.rs index 19c331e..0f63fd6 100644 --- a/src/fork/pty/master/err.rs +++ b/src/fork/pty/master/err.rs @@ -1,4 +1,4 @@ -use ::descriptor::DescriptorError; +use descriptor::DescriptorError; use std::error::Error; use std::fmt; @@ -34,7 +34,6 @@ impl Error for MasterError { MasterError::UnlockptError => "the `grantpt` has a error, errnois set appropriately.", MasterError::PtsnameError => "the `ptsname` has a error", MasterError::NoFdError => "already closed, no fd", - } } diff --git a/src/fork/pty/master/mod.rs b/src/fork/pty/master/mod.rs index 6927626..3332836 100644 --- a/src/fork/pty/master/mod.rs +++ b/src/fork/pty/master/mod.rs @@ -2,11 +2,12 @@ mod err; use libc; -use ::descriptor::Descriptor; +use descriptor::Descriptor; pub use self::err::{MasterError, Result}; use std::ffi::CStr; use std::io; +use std::os::fd::BorrowedFd; use std::os::unix::io::RawFd; #[derive(Debug, Copy, Clone)] @@ -27,6 +28,13 @@ impl Master { &self.pty } + /// Borrow the raw fd + pub fn borrow_fd(&self) -> Option> { + // Safety: we only ever close on drop, so this will be + // live the whole time. + self.pty.map(|fd| unsafe { BorrowedFd::borrow_raw(fd) }) + } + /// Change UID and GID of slave pty associated with master pty whose /// fd is provided, to the real UID and real GID of the calling thread. pub fn grantpt(&self) -> Result { @@ -58,7 +66,7 @@ impl Master { /// Returns a pointer to a static buffer, which will be overwritten on /// subsequent calls. - pub fn ptsname_r(&self, buf: &mut Vec) -> Result<()> { + pub fn ptsname_r(&self, buf: &mut [u8]) -> Result<()> { if let Some(fd) = self.pty { // Safety: the vector's memory is valid for the duration // of the call @@ -66,7 +74,7 @@ impl Master { let data: *mut u8 = &mut buf[0]; match libc::ptsname_r(fd, data as *mut libc::c_char, buf.len()) { 0 => Ok(()), - _ => Err(MasterError::PtsnameError), // should probably capture errno + _ => Err(MasterError::PtsnameError), // should probably capture errno } } } else { @@ -85,9 +93,7 @@ impl io::Read for Master { fn read(&mut self, buf: &mut [u8]) -> io::Result { if let Some(fd) = self.pty { unsafe { - match libc::read(fd, - buf.as_mut_ptr() as *mut libc::c_void, - buf.len()) { + match libc::read(fd, buf.as_mut_ptr() as *mut libc::c_void, buf.len()) { -1 => Ok(0), len => Ok(len as usize), } @@ -102,9 +108,7 @@ impl io::Write for Master { fn write(&mut self, buf: &[u8]) -> io::Result { if let Some(fd) = self.pty { unsafe { - match libc::write(fd, - buf.as_ptr() as *const libc::c_void, - buf.len()) { + match libc::write(fd, buf.as_ptr() as *const libc::c_void, buf.len()) { -1 => Err(io::Error::last_os_error()), ret => Ok(ret as usize), } diff --git a/src/fork/pty/slave/err.rs b/src/fork/pty/slave/err.rs index 4498acd..88cc272 100644 --- a/src/fork/pty/slave/err.rs +++ b/src/fork/pty/slave/err.rs @@ -1,4 +1,4 @@ -use ::descriptor::DescriptorError; +use descriptor::DescriptorError; use std::error::Error; use std::fmt; diff --git a/src/fork/pty/slave/mod.rs b/src/fork/pty/slave/mod.rs index 6f1ab01..7bf8179 100644 --- a/src/fork/pty/slave/mod.rs +++ b/src/fork/pty/slave/mod.rs @@ -1,10 +1,11 @@ mod err; -use ::descriptor::Descriptor; -use ::libc; +use descriptor::Descriptor; +use libc; -pub use self::err::{SlaveError, Result}; +pub use self::err::{Result, SlaveError}; use std::ffi::CStr; +use std::os::fd::BorrowedFd; use std::os::unix::io::RawFd; #[derive(Debug, Clone)] @@ -26,6 +27,13 @@ impl Slave { &self.pty } + /// Borrow the raw fd + pub fn borrow_fd(&self) -> Option> { + // Safety: we only ever close on drop, so this will be + // live the whole time. + self.pty.map(|fd| unsafe { BorrowedFd::borrow_raw(fd) }) + } + pub fn dup2(&self, std: libc::c_int) -> Result { if let Some(fd) = self.pty { unsafe { diff --git a/src/lib.rs b/src/lib.rs index 728d83d..04ea011 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -71,28 +71,12 @@ //! } //! ``` -#![crate_type = "lib"] - -#![cfg_attr(feature = "nightly", feature(plugin))] -#![cfg_attr(feature = "lints", plugin(clippy))] -#![cfg_attr(feature = "lints", deny(warnings))] -#![deny( - missing_debug_implementations, - missing_copy_implementations, - trivial_casts, - trivial_numeric_casts, - unstable_features, - unused_import_braces, - unused_features, - unused_qualifications, -)] - -extern crate libc; extern crate errno; +extern crate libc; extern crate log; mod descriptor; pub mod fork; pub mod prelude; -const DEFAULT_PTMX: &'static str = "/dev/ptmx"; +const DEFAULT_PTMX: &str = "/dev/ptmx"; diff --git a/tests/it_can_read_write.rs b/tests/it_can_read_write.rs index 90558dc..91da56f 100644 --- a/tests/it_can_read_write.rs +++ b/tests/it_can_read_write.rs @@ -1,17 +1,17 @@ -extern crate shpool_pty; extern crate libc; +extern crate shpool_pty; use self::shpool_pty::prelude::*; use std::io::prelude::*; -use std::string::String; use std::process::Command; +use std::string::String; -fn read_line(master:&mut Master) -> String { +fn read_line(master: &mut Master) -> String { let mut buf = [0]; let mut res = String::new(); while buf[0] as char != '\n' { - master.read(&mut buf).expect("cannot read 1 byte"); + master.read_exact(&mut buf).expect("cannot read 1 byte"); res.push(buf[0] as char) } res @@ -21,7 +21,7 @@ fn read_line(master:&mut Master) -> String { fn it_can_read_write() { let fork = Fork::from_ptmx().unwrap(); - if let Some(mut master) = fork.is_parent().ok() { + if let Ok(mut master) = fork.is_parent() { let _ = master.write("echo readme!\n".to_string().as_bytes()); read_line(&mut master); // this is the "echo readme!" we just sent diff --git a/tests/it_fork_with_new_pty.rs b/tests/it_fork_with_new_pty.rs index 65d0373..b8a5c14 100644 --- a/tests/it_fork_with_new_pty.rs +++ b/tests/it_fork_with_new_pty.rs @@ -1,6 +1,6 @@ -extern crate shpool_pty; -extern crate libc; extern crate errno; +extern crate libc; +extern crate shpool_pty; use self::shpool_pty::prelude::*; @@ -12,10 +12,12 @@ use std::string::String; fn it_fork_with_new_pty() { let fork = Fork::from_ptmx().unwrap(); - if let Some(mut master) = fork.is_parent().ok() { + if let Ok(mut master) = fork.is_parent() { let mut string = String::new(); - master.read_to_string(&mut string).unwrap_or_else(|e| panic!("{}", e)); + master + .read_to_string(&mut string) + .unwrap_or_else(|e| panic!("{}", e)); let output = Command::new("tty") .stdin(Stdio::inherit()) @@ -27,14 +29,14 @@ fn it_fork_with_new_pty() { let parent_tty = output_str.trim(); let child_tty = string.trim(); - assert!(child_tty != ""); + assert!(!child_tty.is_empty()); assert!(child_tty != parent_tty); // only compare if parent is tty // travis runs the tests without tty if parent_tty != "not a tty" { - let mut parent_tty_dir: Vec<&str> = parent_tty.split("/").collect(); - let mut child_tty_dir: Vec<&str> = child_tty.split("/").collect(); + let mut parent_tty_dir: Vec<&str> = parent_tty.split('/').collect(); + let mut child_tty_dir: Vec<&str> = child_tty.split('/').collect(); parent_tty_dir.pop(); child_tty_dir.pop(); diff --git a/tests/lib.rs b/tests/lib.rs index 51b0338..9c047bb 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -1,2 +1,2 @@ -mod it_fork_with_new_pty; mod it_can_read_write; +mod it_fork_with_new_pty;