From 95ac51101e22f5df2331568da369c30d91081070 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Mon, 11 Nov 2024 13:31:24 -0600 Subject: [PATCH] Use `Option` instead of `Option` Statically assert that the interior value is both positive and non-zero. --- src/builtins/disown.rs | 2 +- src/exec.rs | 11 ++++-- src/fork_exec/postfork.rs | 21 ++++++----- src/fork_exec/spawn.rs | 2 +- src/job_group.rs | 16 ++++---- src/proc.rs | 78 ++++++++++++++++++++++++++------------- 6 files changed, 80 insertions(+), 50 deletions(-) diff --git a/src/builtins/disown.rs b/src/builtins/disown.rs index 2e0a9a1dc..61905527a 100644 --- a/src/builtins/disown.rs +++ b/src/builtins/disown.rs @@ -24,7 +24,7 @@ fn disown_job(cmd: &wstr, streams: &mut IoStreams, j: &Job) { if j.is_stopped() { if let Some(pgid) = pgid { unsafe { - libc::killpg(pgid, SIGCONT); + libc::killpg(pgid.as_pid_t(), SIGCONT); } } streams.err.append(wgettext_fmt!( diff --git a/src/exec.rs b/src/exec.rs index 03c0d65dd..655382b65 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -698,7 +698,7 @@ fn fork_child_for_process( let narrow_cmd = wcs2zstring(job.command()); let narrow_argv0 = wcs2zstring(p.argv0().unwrap_or_default()); - let pid = execute_fork(); + let mut pid = execute_fork(); if pid < 0 { return Err(()); } @@ -709,7 +709,8 @@ fn fork_child_for_process( p.set_pid(if is_parent { pid } else { - unsafe { libc::getpid() } + pid = unsafe { libc::getpid() }; + pid }); if p.leads_pgrp { job.group().set_pgid(pid); @@ -895,7 +896,7 @@ fn exec_external_command( // In glibc, posix_spawn uses fork() and the pgid group is set on the child side; // therefore the parent may not have seen it be set yet. // Ensure it gets set. See #4715, also https://github.com/Microsoft/WSL/issues/2997. - execute_setpgid(pid.as_pid_t(), pid.as_pid_t(), true /* is parent */); + execute_setpgid(pid, pid, true /* is parent */); } return Ok(()); } @@ -1327,7 +1328,9 @@ fn exec_process_in_job( exec_external_command(parser, j, p, &process_net_io_chain)?; // It's possible (though unlikely) that this is a background process which recycled a // pid from another, previous background process. Forget any such old process. - parser.mut_wait_handles().remove_by_pid(p.pid().unwrap()); + parser + .mut_wait_handles() + .remove_by_pid(p.pid().unwrap().as_pid_t()); Ok(()) } ProcessType::exec => { diff --git a/src/fork_exec/postfork.rs b/src/fork_exec/postfork.rs index 9222d4dca..b2f267ce9 100644 --- a/src/fork_exec/postfork.rs +++ b/src/fork_exec/postfork.rs @@ -3,6 +3,7 @@ // That means no locking, no allocating, no freeing memory, etc! use super::flog_safe::FLOG_SAFE; use crate::nix::getpid; +use crate::proc::Pid; use crate::redirection::Dup2List; use crate::signal::signal_reset_handlers; use crate::{common::exit_without_destructors, wutil::fstat}; @@ -52,20 +53,20 @@ fn strlen_safe(s: *const libc::c_char) -> usize { pub(crate) fn report_setpgid_error( err: i32, is_parent: bool, - pid: pid_t, - desired_pgid: pid_t, + pid: Pid, + desired_pgid: Pid, job_id: i64, command: &CStr, argv0: &CStr, ) { - let cur_group = unsafe { libc::getpgid(pid) }; + let cur_group = unsafe { libc::getpgid(pid.as_pid_t()) }; FLOG_SAFE!( warning, "Could not send ", if is_parent { "child" } else { "self" }, " ", - pid, + pid.get(), ", '", argv0, "' in job ", @@ -75,35 +76,35 @@ pub(crate) fn report_setpgid_error( "' from group ", cur_group, " to group ", - desired_pgid, + desired_pgid.get(), ); match err { - libc::EACCES => FLOG_SAFE!(error, "setpgid: Process ", pid, " has already exec'd"), + libc::EACCES => FLOG_SAFE!(error, "setpgid: Process ", pid.get(), " has already exec'd"), libc::EINVAL => FLOG_SAFE!(error, "setpgid: pgid ", cur_group, " unsupported"), libc::EPERM => { FLOG_SAFE!( error, "setpgid: Process ", - pid, + pid.get(), " is a session leader or pgid ", cur_group, " does not match" ); } - libc::ESRCH => FLOG_SAFE!(error, "setpgid: Process ID ", pid, " does not match"), + libc::ESRCH => FLOG_SAFE!(error, "setpgid: Process ID ", pid.get(), " does not match"), _ => FLOG_SAFE!(error, "setpgid: Unknown error number ", err), } } /// Execute setpgid, moving pid into the given pgroup. /// Return the result of setpgid. -pub fn execute_setpgid(pid: pid_t, pgroup: pid_t, is_parent: bool) -> i32 { +pub fn execute_setpgid(pid: Pid, pgroup: Pid, is_parent: bool) -> i32 { // There is a comment "Historically we have looped here to support WSL." // TODO: stop looping. let mut eperm_count = 0; loop { - if unsafe { libc::setpgid(pid, pgroup) } == 0 { + if unsafe { libc::setpgid(pid.as_pid_t(), pgroup.as_pid_t()) } == 0 { return 0; } let err = errno::errno().0; diff --git a/src/fork_exec/spawn.rs b/src/fork_exec/spawn.rs index fd4e931a5..6283f2bd9 100644 --- a/src/fork_exec/spawn.rs +++ b/src/fork_exec/spawn.rs @@ -105,7 +105,7 @@ impl PosixSpawner { // desired_pgid tracks the pgroup for the process. If it is none, the pgroup is left unchanged. // If it is zero, create a new pgroup from the pid. If it is >0, join that pgroup. let desired_pgid = if let Some(pgid) = j.get_pgid() { - Some(pgid) + Some(pgid.get()) } else if j.processes()[0].leads_pgrp { Some(0) } else { diff --git a/src/job_group.rs b/src/job_group.rs index 944642e80..9bb9ed432 100644 --- a/src/job_group.rs +++ b/src/job_group.rs @@ -1,5 +1,5 @@ use crate::global_safety::RelaxedAtomicBool; -use crate::proc::JobGroupRef; +use crate::proc::{AtomicPid, JobGroupRef, Pid}; use crate::signal::Signal; use crate::wchar::prelude::*; use std::cell::RefCell; @@ -72,8 +72,8 @@ pub struct JobGroup { /// Whether we are in the foreground, meaning the user is waiting for this job to complete. pub is_foreground: RelaxedAtomicBool, /// The pgid leading our group. This is only ever set if [`job_control`](Self::JobControl) is - /// true. We ensure the value (when set) is always non-negative. - pgid: RefCell>, + /// true. We ensure the value (when set) is always non-negative and non-zero. + pgid: AtomicPid, /// The original command which produced this job tree. pub command: WString, /// Our job id, if any. `None` here should evaluate to `-1` for ffi purposes. @@ -149,14 +149,14 @@ impl JobGroup { "Should not set a pgid for a group that doesn't want job control!" ); assert!(pgid >= 0, "Invalid pgid!"); - assert!(self.pgid.borrow().is_none(), "JobGroup::pgid already set!"); - self.pgid.replace(Some(pgid)); + let old_pgid = self.pgid.swap(Pid::new(pgid).unwrap()); + assert!(old_pgid.is_none(), "JobGroup::pgid already set!"); } /// Returns the value of [`JobGroup::pgid`]. This is never fish's own pgid! - pub fn get_pgid(&self) -> Option { - *self.pgid.borrow() + pub fn get_pgid(&self) -> Option { + self.pgid.load() } } @@ -223,7 +223,7 @@ impl JobGroup { tmodes: RefCell::default(), signal: 0.into(), is_foreground: RelaxedAtomicBool::new(false), - pgid: RefCell::default(), + pgid: AtomicPid::default(), } } diff --git a/src/proc.rs b/src/proc.rs index 8c3b09f03..c429f7f61 100644 --- a/src/proc.rs +++ b/src/proc.rs @@ -361,11 +361,13 @@ impl TtyTransfer { // Get the pgid; we must have one if we want the terminal. let pgid = jg.get_pgid().unwrap(); - assert!(pgid >= 0, "Invalid pgid"); // It should never be fish's pgroup. let fish_pgrp = unsafe { libc::getpgrp() }; - assert!(pgid != fish_pgrp, "Job should not have fish's pgroup"); + assert!( + pgid.as_pid_t() != fish_pgrp, + "Job should not have fish's pgroup" + ); // Ok, we want to transfer to the child. // Note it is important to be very careful about calling tcsetpgrp()! @@ -385,10 +387,10 @@ impl TtyTransfer { if current_owner < 0 { // Case 1. return false; - } else if current_owner == pgid { + } else if current_owner == pgid.get() { // Case 2. return true; - } else if current_owner != pgid && current_owner != fish_pgrp { + } else if current_owner != pgid.get() && current_owner != fish_pgrp { // Case 3. return false; } @@ -403,7 +405,7 @@ impl TtyTransfer { // 4.4.0), EPERM does indeed disappear on retry. The important thing is that we can // guarantee the process isn't going to exit while we wait (which would cause us to possibly // block indefinitely). - while unsafe { libc::tcsetpgrp(STDIN_FILENO, pgid) } != 0 { + while unsafe { libc::tcsetpgrp(STDIN_FILENO, pgid.as_pid_t()) } != 0 { FLOGF!(proc_termowner, "tcsetpgrp failed: %d", errno::errno().0); // Before anything else, make sure that it's even necessary to call tcsetpgrp. @@ -429,7 +431,7 @@ impl TtyTransfer { } } } - if getpgrp_res == pgid { + if getpgrp_res == pgid.get() { FLOGF!( proc_termowner, "Process group %d already has control of terminal", @@ -447,7 +449,7 @@ impl TtyTransfer { } else if errno::errno().0 == EPERM { // Retry so long as this isn't because the process group is dead. let mut result: libc::c_int = 0; - let wait_result = unsafe { libc::waitpid(-pgid, &mut result, WNOHANG) }; + let wait_result = unsafe { libc::waitpid(-pgid.as_pid_t(), &mut result, WNOHANG) }; if wait_result == -1 { // Note that -1 is technically an "error" for waitpid in the sense that an // invalid argument was specified because no such process group exists any @@ -508,6 +510,7 @@ impl Drop for TtyTransfer { } } +/// A type-safe equivalent to [`libc::pid_t`]. #[repr(transparent)] #[derive(Clone, Copy, Debug, PartialEq)] pub struct Pid(NonZeroU32); @@ -540,6 +543,24 @@ impl Into for u32 { } } +impl std::fmt::Display for Pid { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Display::fmt(&self.get(), f) + } +} + +impl ToWString for Pid { + fn to_wstring(&self) -> WString { + self.get().to_wstring() + } +} + +impl fish_printf::ToArg<'static> for Pid { + fn to_arg(self) -> fish_printf::Arg<'static> { + fish_printf::Arg::UInt(self.get() as u64) + } +} + #[derive(Default, Debug)] pub struct AtomicPid(AtomicU32); @@ -666,13 +687,13 @@ impl Process { /// Retrieves the associated [`libc::pid_t`], `None` if unset. #[inline(always)] - pub fn pid(&self) -> Option { - self.pid.load().map(|pid| pid.as_pid_t()) + pub fn pid(&self) -> Option { + self.pid.load() } #[inline(always)] pub fn has_pid(&self) -> bool { - self.pid.load().is_some() + self.pid().is_some() } /// Sets the process' pid. Panics if a pid has already been set. @@ -759,7 +780,7 @@ impl Process { } else { if self.wait_handle.borrow().is_none() { self.wait_handle.replace(Some(WaitHandle::new( - self.pid().unwrap(), + self.pid().unwrap().get(), jid, wbasename(&self.actual_cmd.clone()).to_owned(), ))); @@ -903,17 +924,17 @@ impl Job { /// Return our pgid, or none if we don't have one, or are internal to fish /// This never returns fish's own pgroup. - // TODO: Return a type-safe result. - pub fn get_pgid(&self) -> Option { + pub fn get_pgid(&self) -> Option { self.group().get_pgid() } /// Return the pid of the last external process in the job. /// This may be none if the job consists of just internal fish functions or builtins. /// This will never be fish's own pid. - // TODO: Return a type-safe result. - pub fn get_last_pid(&self) -> Option { - self.processes().iter().filter_map(|proc| proc.pid()).last() + pub fn get_last_pid(&self) -> Option { + self.external_procs() + .last() + .and_then(|proc| proc.pid.load()) } /// The id of this job. @@ -1082,20 +1103,22 @@ impl Job { /// Return true on success, false on failure. pub fn signal(&self, signal: i32) -> bool { if let Some(pgid) = self.group().get_pgid() { - if unsafe { libc::killpg(pgid, signal) } == -1 { + if unsafe { libc::killpg(pgid.as_pid_t(), signal) } == -1 { let strsignal = unsafe { libc::strsignal(signal) }; let strsignal = if strsignal.is_null() { L!("(nil)").to_owned() } else { charptr2wcstring(strsignal) }; - wperror(&sprintf!("killpg(%d, %s)", pgid, strsignal)); + wperror(&sprintf!("killpg(%d, %s)", pgid.get(), strsignal)); return false; } } else { // This job lives in fish's pgroup and we need to signal procs individually. for p in self.external_procs() { - if !p.is_completed() && unsafe { libc::kill(p.pid().unwrap(), signal) } == -1 { + if !p.is_completed() + && unsafe { libc::kill(p.pid().unwrap().as_pid_t(), signal) } == -1 + { return false; } } @@ -1235,7 +1258,7 @@ pub fn print_exit_warning_for_jobs(jobs: &JobList) { // external processes always have a pid set. printf!( "%6d %ls\n", - j.external_procs().next().unwrap().pid().unwrap(), + j.external_procs().next().and_then(|p| p.pid()).unwrap(), j.command() ); } @@ -1354,7 +1377,7 @@ pub fn hup_jobs(jobs: &JobList) { let mut kill_list = Vec::new(); for j in jobs { let Some(pgid) = j.get_pgid() else { continue }; - if pgid != fish_pgrp && !j.is_completed() { + if pgid.as_pid_t() != fish_pgrp && !j.is_completed() { j.signal(SIGHUP); if j.is_stopped() { j.signal(SIGCONT); @@ -1391,7 +1414,7 @@ pub fn hup_jobs(jobs: &JobList) { pub fn add_disowned_job(j: &Job) { let mut disowned_pids = DISOWNED_PIDS.get().borrow_mut(); for process in j.external_procs() { - disowned_pids.push(process.pid().unwrap()); + disowned_pids.push(process.pid().unwrap().as_pid_t()); } } @@ -1475,7 +1498,7 @@ fn process_mark_finished_children(parser: &Parser, block_ok: bool) { let mut statusv: libc::c_int = -1; let pid = unsafe { libc::waitpid( - proc.pid().unwrap(), + proc.pid().unwrap().as_pid_t(), &mut statusv, WNOHANG | WUNTRACED | WCONTINUED, ) @@ -1483,7 +1506,10 @@ fn process_mark_finished_children(parser: &Parser, block_ok: bool) { if pid <= 0 { continue; } - assert!(pid == proc.pid().unwrap(), "Unexpected waitpid() return"); + assert!( + pid == proc.pid().unwrap().get(), + "Unexpected waitpid() return" + ); // The process has stopped or exited! Update its status. let status = ProcStatus::from_waitpid(statusv); @@ -1572,7 +1598,7 @@ fn generate_process_exit_events(j: &Job, out_evts: &mut Vec) { if p.is_completed() && !p.posted_proc_exit.load() { p.posted_proc_exit.store(true); out_evts.push(Event::process_exit( - p.pid().unwrap(), + p.pid().unwrap().as_pid_t(), p.status.status_value(), )); } @@ -1587,7 +1613,7 @@ fn generate_job_exit_events(j: &Job, out_evts: &mut Vec) { // job_exit events. if j.posts_job_exit_events() { if let Some(last_pid) = j.get_last_pid() { - out_evts.push(Event::job_exit(last_pid, j.internal_job_id)); + out_evts.push(Event::job_exit(last_pid.as_pid_t(), j.internal_job_id)); } } }