Use Option<Pid> instead of Option<pid_t>

Statically assert that the interior value is both positive and non-zero.

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!(

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 => {

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;

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 {

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<Option<libc::pid_t>>,
+    /// 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<libc::pid_t> {
-        *self.pgid.borrow()
+    pub fn get_pgid(&self) -> Option<Pid> {
+        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(),
         }
     }
 

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<Pid> 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<libc::pid_t> {
-        self.pid.load().map(|pid| pid.as_pid_t())
+    pub fn pid(&self) -> Option<Pid> {
+        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<libc::pid_t> {
+    pub fn get_pgid(&self) -> Option<Pid> {
         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<libc::pid_t> {
-        self.processes().iter().filter_map(|proc| proc.pid()).last()
+    pub fn get_last_pid(&self) -> Option<Pid> {
+        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<Event>) {
             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<Event>) {
         // 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));
             }
         }
     }