Wait built-in: ignore disowned jobs & add scripted tests (#335)

yash-builtin/src/jobs.rs

@@ -70,6 +70,10 @@
 //! jobs %sleep %sleep
 //! ```
 //!
+//! When the built-in is used in a subshell, the built-in reports not only jobs
+//! that were started in the subshell but also jobs that were started in the
+//! parent shell. This behavior is not portable and is subject to change.
+//!
 //! The POSIX standard only defines the `-l` and `-p` options. Other options are
 //! non-portable extensions.
 //!

yash-builtin/src/wait.rs

@@ -82,6 +82,11 @@
 //! that an unknown process ID be treated as a process that has already exited
 //! with exit status 127, but the behavior for other errors should not be
 //! considered portable.
+//!
+//! # Implementation notes
+//!
+//! The built-in treats disowned jobs as if they were finished with an exit
+//! status of 127.
 
 use crate::common::report_error;
 use crate::common::report_simple_failure;

yash-builtin/src/wait/status.rs

@@ -53,6 +53,10 @@ pub async fn wait_while_running(
 
 /// Returns a closure that tests if the job with the given index has finished.
 ///
+/// If the job at the given index is not found or is disowned, the closure
+/// returns [`ControlFlow::Break`] having [`ExitStatus::NOT_FOUND`].
+/// The disowned job is removed from the job set.
+///
 /// If the job has finished (either exited or signaled), the closure removes the
 /// job from the job set and returns [`ControlFlow::Break`] with the job's exit
 /// status. If `job_control` is `On` and the job has been stopped, the closure
@@ -67,6 +71,12 @@ pub fn job_status(
         let Some(job) = jobs.get(index) else {
             return ControlFlow::Break(ExitStatus::NOT_FOUND);
         };
+
+        if !job.is_owned {
+            jobs.remove(index);
+            return ControlFlow::Break(ExitStatus::NOT_FOUND);
+        }
+
         match job.status {
             WaitStatus::Exited(_pid, exit_status) => {
                 jobs.remove(index);
@@ -238,6 +248,20 @@ mod tests {
         assert_eq!(jobs.get(index).unwrap().pid, Pid::from_raw(456));
     }
 
+    #[test]
+    fn status_of_disowned_job() {
+        let mut jobs = JobSet::new();
+        let mut job = Job::new(Pid::from_raw(123));
+        job.is_owned = false;
+        let index = jobs.add(job);
+
+        assert_eq!(
+            job_status(index, Off)(&mut jobs),
+            ControlFlow::Break(ExitStatus::NOT_FOUND),
+        );
+        assert_eq!(jobs.get(index), None);
+    }
+
     #[test]
     fn any_job_is_running_with_no_job() {
         let mut jobs = JobSet::new();

yash-env/src/job.rs

@@ -90,6 +90,13 @@ pub struct Job {
     /// last reported to the user.
     pub status_changed: bool,
 
+    /// Whether this job is a true child of the current shell
+    ///
+    /// When a subshell is created, the jobs inherited from the parent shell are
+    /// marked as not owned by the current shell. The shell cannot wait for
+    /// these jobs to finish.
+    pub is_owned: bool,
+
     /// String representation of this process
     pub name: String,
 }
@@ -105,6 +112,7 @@ impl Job {
             job_controlled: false,
             status: WaitStatus::StillAlive,
             status_changed: true,
+            is_owned: true,
             name: String::new(),
         }
     }
@@ -579,6 +587,15 @@ impl JobSet {
         }
         index
     }
+
+    /// Disowns all jobs.
+    ///
+    /// This function sets the `is_owned` flag of all jobs to `false`.
+    pub fn disown_all(&mut self) {
+        for (_, job) in &mut self.jobs {
+            job.is_owned = false;
+        }
+    }
 }
 
 /// Error type for [`JobSet::set_current_job`].
@@ -828,6 +845,21 @@ mod tests {
         assert_eq!(set.get(i30).unwrap().status, WaitStatus::StillAlive);
     }
 
+    #[test]
+    #[allow(clippy::bool_assert_comparison)]
+    fn disowning_jobs() {
+        let mut set = JobSet::default();
+        let i10 = set.add(Job::new(Pid::from_raw(10)));
+        let i20 = set.add(Job::new(Pid::from_raw(20)));
+        let i30 = set.add(Job::new(Pid::from_raw(30)));
+
+        set.disown_all();
+
+        assert_eq!(set.get(i10).unwrap().is_owned, false);
+        assert_eq!(set.get(i20).unwrap().is_owned, false);
+        assert_eq!(set.get(i30).unwrap().is_owned, false);
+    }
+
     #[test]
     fn no_current_and_previous_job_in_empty_job_set() {
         let set = JobSet::default();

yash-env/src/subshell.rs

@@ -187,6 +187,7 @@ where
                         }
                     }
                 }
+                env.jobs.disown_all();
 
                 env.traps.enter_subshell(
                     &mut env.system,
@@ -308,6 +309,7 @@ impl<'a> Drop for MaskGuard<'a> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::job::Job;
     use crate::option::Option::{Interactive, Monitor};
     use crate::option::State::On;
     use crate::semantics::ExitStatus;
@@ -380,6 +382,23 @@ mod tests {
         });
     }
 
+    #[test]
+    fn jobs_disowned_in_subshell() {
+        in_virtual_system(|mut env, _state| async move {
+            let index = env.jobs.add(Job::new(Pid::from_raw(123)));
+            let subshell = Subshell::new(move |env, _job_control| {
+                Box::pin(async move {
+                    assert!(!env.jobs.get(index).unwrap().is_owned);
+                    Continue(())
+                })
+            });
+            let pid = subshell.start(&mut env).await.unwrap().0;
+            env.wait_for_subshell(pid).await.unwrap();
+
+            assert!(env.jobs.get(index).unwrap().is_owned);
+        });
+    }
+
     #[test]
     fn trap_reset_in_subshell() {
         in_virtual_system(|mut env, _state| async move {

yash/Cargo.toml

@@ -32,4 +32,5 @@ yash-syntax = { path = "../yash-syntax", version = "0.7.0" }
 [dev-dependencies]
 assert_matches = "1.5.0"
 fuzed-iterator = "1.0.0"
+nix = { version = "0.27.0", features = ["fs", "process", "term"] }
 tempfile = "3.8.0"

yash/tests/pty/mod.rs

@@ -0,0 +1,101 @@
+// This file is part of yash, an extended POSIX shell.
+// Copyright (C) 2023 WATANABE Yuki
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+//! Pseudo-terminal handling for scripted tests
+//!
+//! This module contains functionality to run a test subject in a
+//! pseudo-terminal and communicate with it via the master side of the
+//! pseudo-terminal. This is used for tests that depends on terminal
+//! facilities such as job control.
+
+use crate::run_with_preexec;
+use nix::fcntl::{open, OFlag};
+use nix::libc;
+use nix::pty::{grantpt, posix_openpt, ptsname, unlockpt, PtyMaster};
+use nix::sys::stat::Mode;
+use nix::unistd::{close, getpgrp, setsid, tcgetpgrp};
+use std::ffi::c_int;
+use std::os::fd::{AsRawFd as _, FromRawFd as _, OwnedFd};
+use std::path::{Path, PathBuf};
+use std::sync::Mutex;
+
+/// Runs a test subject in a pseudo-terminal.
+pub fn run_with_pty(name: &str) {
+    let master = prepare_pty_master();
+    let slave_path = pty_slave_path(&master);
+    let slave = open_pty_slave(&slave_path);
+    let raw_master = master.as_raw_fd();
+    let raw_slave = slave.as_raw_fd();
+
+    unsafe {
+        run_with_preexec(name, move || {
+            close(raw_master)?;
+            prepare_as_slave(&slave_path)?;
+            close(raw_slave)?;
+            Ok(())
+        });
+    }
+}
+
+/// Prepares the master side of a pseudo-terminal.
+fn prepare_pty_master() -> PtyMaster {
+    let master = posix_openpt(OFlag::O_RDWR | OFlag::O_NOCTTY).expect("posix_openpt failed");
+    grantpt(&master).expect("grantpt failed");
+    unlockpt(&master).expect("unlockpt failed");
+    master
+}
+
+/// Mutex to serialize access to `ptsname`, which is not thread-safe.
+static PTSNAME_MUTEX: Mutex<()> = Mutex::new(());
+
+/// Returns the path of the slave side of a pseudo-terminal.
+fn pty_slave_path(master: &PtyMaster) -> PathBuf {
+    let _lock = PTSNAME_MUTEX.lock().expect("PTSNAME_MUTEX poisoned");
+    unsafe { ptsname(master) }.expect("ptsname failed").into()
+}
+
+/// Opens the slave side of a pseudo-terminal.
+fn open_pty_slave(path: &Path) -> OwnedFd {
+    let raw_fd = open(path, OFlag::O_RDWR | OFlag::O_NOCTTY, Mode::empty()).expect("open failed");
+    unsafe { OwnedFd::from_raw_fd(raw_fd) }
+}
+
+/// Prepares the slave side of a pseudo-terminal.
+///
+/// No memory allocation or panicking is allowed in this function because it is
+/// called in a child process.
+fn prepare_as_slave(slave_path: &Path) -> nix::Result<()> {
+    setsid()?;
+
+    // How to become the controlling process of a slave pseudo-terminal is
+    // implementation-dependent. We support two implementation schemes:
+    // (1) A process automatically becomes the controlling process when it
+    // first opens the terminal.
+    // (2) A process needs to use the TIOCSCTTY ioctl system call.
+    // There is a race condition in both schemes: an unrelated process could
+    // become the controlling process before we do, in which case the slave is
+    // not our controlling terminal and therefore we should abort.
+    let raw_fd = open(slave_path, OFlag::O_RDWR, Mode::empty())?;
+    // Although TIOCSCTTY is available in many Unix-like systems, it may not be
+    // available in some systems. Please report if you encounter such a system.
+    unsafe { libc::ioctl(raw_fd, libc::TIOCSCTTY as _, 0 as c_int) };
+
+    if tcgetpgrp(raw_fd) == Ok(getpgrp()) {
+        Ok(())
+    } else {
+        Err(nix::Error::ENOTSUP)
+    }
+}

yash/tests/scripted_test.rs

@@ -19,29 +19,43 @@
 //! examines the results. Test cases are written in script files named with the
 //! `-p.sh` or `-y.sh` suffix.
 
+use pty::run_with_pty;
+use std::os::unix::process::CommandExt as _;
 use std::path::Path;
 use std::process::Command;
 use std::process::Stdio;
 
+mod pty;
+
 const BIN: &str = env!("CARGO_BIN_EXE_yash");
 const TMPDIR: &str = env!("CARGO_TARGET_TMPDIR");
 
-fn run(name: &str) {
+/// Runs a test subject.
+///
+/// You would normally not use this function directly. Instead, use one of the
+/// [`run`] or [`run_with_pty`] functions.
+unsafe fn run_with_preexec<F>(name: &str, pre_exec: F)
+where
+    F: FnMut() -> std::io::Result<()> + Send + Sync + 'static,
+{
     // TODO Reset signal blocking mask
 
     let mut log_file = Path::new(TMPDIR).join(name);
     log_file.set_extension("log");
 
-    let result = Command::new("sh")
+    let mut command = Command::new("sh");
+    command
         .env("TMPDIR", TMPDIR)
         .current_dir("tests/scripted_test")
         .stdin(Stdio::null())
         .arg("./run-test.sh")
         .arg(BIN)
         .arg(name)
-        .arg(&log_file)
-        .output()
-        .unwrap();
+        .arg(&log_file);
+    unsafe {
+        command.pre_exec(pre_exec);
+    }
+    let result = command.output().unwrap();
     assert!(result.status.success(), "{:?}", result);
 
     // The `run-test.sh` script returns a successful exit status even if there
@@ -51,6 +65,14 @@ fn run(name: &str) {
     assert!(!log.contains("FAILED"), "{}", log);
 }
 
+/// Runs a test subject.
+///
+/// This function runs the test subject in the current session. To run it in a
+/// separate session, use [`run_with_pty`].
+fn run(name: &str) {
+    unsafe { run_with_preexec(name, || Ok(())) }
+}
+
 #[test]
 fn alias() {
     run("alias-p.sh")
@@ -237,6 +259,11 @@ fn until_loop() {
     run("until-p.sh")
 }
 
+#[test]
+fn wait_builtin() {
+    run_with_pty("wait-p.sh")
+}
+
 #[test]
 fn while_loop() {
     run("while-p.sh")