diff --git a/kernel/src/process/process/mod.rs b/kernel/src/process/process/mod.rs index 47c25533..08f04795 100644 --- a/kernel/src/process/process/mod.rs +++ b/kernel/src/process/process/mod.rs @@ -316,27 +316,6 @@ impl Process { .map_or(0, |session| session.sid()) } - /// Returns the process group and the session if the process is a session leader. - /// - /// Note that once a process becomes a session leader, it remains a session leader forever. - /// Specifically, it can no longer be moved to a new process group or a new session. So this - /// method is race-free. - // - // FIXME: If we call this method on a non-current process without holding the process table - // lock, the session may be dead if the process is reaped at the same time. - fn leading_session(&self) -> Option<(Arc, Arc)> { - let process_group_mut = self.process_group.lock(); - - let process_group = process_group_mut.upgrade().unwrap(); - let session = process_group.session().unwrap(); - - if session.is_leader(self) { - Some((process_group, session)) - } else { - None - } - } - /// Returns the controlling terminal of the process, if any. pub fn terminal(&self) -> Option> { self.process_group diff --git a/kernel/src/process/process/terminal.rs b/kernel/src/process/process/terminal.rs index 708e66fc..d5874738 100644 --- a/kernel/src/process/process/terminal.rs +++ b/kernel/src/process/process/terminal.rs @@ -92,7 +92,13 @@ impl dyn Terminal { /// Sets the terminal to be the controlling terminal of the process. pub(super) fn set_control(self: Arc, process: &Process) -> Result<()> { - let Some((process_group, session)) = process.leading_session() else { + // Lock order: group of process -> session inner -> job control + let process_group_mut = process.process_group.lock(); + + let process_group = process_group_mut.upgrade().unwrap(); + let session = process_group.session().unwrap(); + + if !session.is_leader(process) { return_errno_with_message!( Errno::EPERM, "the process who sets the controlling terminal is not a session leader" @@ -119,13 +125,25 @@ impl dyn Terminal { /// Unsets the terminal from the controlling terminal of the process. fn unset_control(self: Arc, process: &Process) -> Result<()> { - self.is_control_and(process, |_, session_inner| { + // Lock order: group of process -> session inner -> job control + self.is_control_and(process, |session, session_inner| { + if !session.is_leader(process) { + // TODO: The Linux kernel keeps track of the controlling terminal of each process + // in `current->signal->tty`. So even if we're not the session leader, this may + // still succeed in releasing the controlling terminal of the current process. Note + // that the controlling terminal of the session will never be released in this + // case. We cannot mimic the exact Linux behavior, so we just return `Ok(())` here. + return Ok(()); + } + session_inner.set_terminal(None); if let Some(foreground) = self.job_control().unset_session() { use crate::process::signal::{ constants::{SIGCONT, SIGHUP}, signals::kernel::KernelSignal, }; + // FIXME: Correct the lock order here. We cannot lock the group inner after locking + // the session inner. foreground.broadcast_signal(KernelSignal::new(SIGHUP)); foreground.broadcast_signal(KernelSignal::new(SIGCONT)); } @@ -136,7 +154,7 @@ impl dyn Terminal { /// Sets the foreground process group of the terminal. fn set_foreground(self: Arc, pgid: Pgid, process: &Process) -> Result<()> { - // Take this lock before calling `is_control_and` to avoid dead locks. + // Lock order: group table -> group of process -> session inner -> job control let group_table_mut = process_table::group_table_mut(); self.is_control_and(process, |session, _| { @@ -162,19 +180,16 @@ impl dyn Terminal { /// Runs `op` when the process controls the terminal. /// - /// Note that this requires that _both_ of the following two conditions are met: - /// * the process is a session leader, and - /// * the terminal is the controlling terminal of the session. + /// Note that this requires that the terminal is the controlling terminal of the session, but + /// does _not_ require that the process is a session leader. fn is_control_and(self: &Arc, process: &Process, op: F) -> Result where F: FnOnce(&Arc, &mut SessionGuard) -> Result, { - let Some((_, session)) = process.leading_session() else { - return_errno_with_message!( - Errno::ENOTTY, - "the process who operates the terminal is not a session leader" - ); - }; + let process_group_mut = process.process_group.lock(); + + let process_group = process_group_mut.upgrade().unwrap(); + let session = process_group.session().unwrap(); let mut session_inner = session.lock(); diff --git a/test/apps/process/job_control.c b/test/apps/process/job_control.c index f590280f..a97e160c 100644 --- a/test/apps/process/job_control.c +++ b/test/apps/process/job_control.c @@ -26,7 +26,11 @@ FN_SETUP(run_in_new_session) } sid = CHECK(setsid()); +} +END_SETUP() +FN_SETUP(run_child) +{ if ((child = CHECK(fork())) == 0) { child = getpid(); @@ -144,19 +148,30 @@ FN_TEST(leader_set_unset_tty) } END_TEST() -BARRIER() +// TODO: Next we do `kill_child` and `run_child_with_tty` because the +// Linux kernel keeps track of the controlling terminal of each process +// in `current->signal->tty`. So without killing and restarting the +// child, the controlling terminal won't be visible to it. This problem +// does not exist in Asterinas, but we may want to fix this behavioral +// difference later. -FN_TEST(nonleader_unset_tty) +FN_SETUP(kill_child) { - if (is_leader()) - return; + int status; - // Only session leaders can use `TIOCNOTTY`, but we're not the leader - TEST_ERRNO(ioctl(master, TIOCNOTTY), ENOTTY); - TEST_ERRNO(ioctl(slave, TIOCNOTTY), ENOTTY); - TEST_ERRNO(ioctl(STDIN_FILENO, TIOCNOTTY), ENOTTY); + if (!is_leader()) + exit(__total_failures ? EXIT_FAILURE : EXIT_SUCCESS); + + CHECK_WITH(wait(&status), + WIFEXITED(status) && WEXITSTATUS(status) == 0); } -END_TEST() +END_SETUP() + +FN_SETUP(run_child_with_tty) +{ + setup_run_child(); +} +END_SETUP() FN_TEST(query_foreground) { @@ -165,24 +180,23 @@ FN_TEST(query_foreground) // All processes can use `TIOCGPGRP` on the master PTY TEST_RES(ioctl(master, TIOCGPGRP, &arg), arg == sid); - // Only session leaders can use `TIOCGPGRP` on the slave PTY - if (is_leader()) - TEST_RES(ioctl(slave, TIOCGPGRP, &arg), arg == sid); - else - TEST_ERRNO(ioctl(slave, TIOCGPGRP, &arg), ENOTTY); + // The slave PTY is our controlling terminal, so `TIOCGPGRP` works + TEST_RES(ioctl(slave, TIOCGPGRP, &arg), arg == sid); } END_TEST() BARRIER() -FN_TEST(leader_set_foreground) +FN_TEST(set_foreground) { pid_t arg; + // Make sure the tests won't be run concurrently if (!is_leader()) - return; + TEST_SUCC(usleep(100 * 1000)); - // Only session leaders can use `TIOCSPGRP`, and we're the leader + // All processes can use `TIOCSPGRP` on the master PTY + // The slave PTY is our controlling terminal, so `TIOSGPGRP` works arg = child; TEST_SUCC(ioctl(master, TIOCSPGRP, &arg)); @@ -203,33 +217,15 @@ FN_TEST(leader_set_foreground) TEST_SUCC(ioctl(slave, TIOCSPGRP, &arg)); arg = 0xdeadbeef; TEST_RES(ioctl(master, TIOCGPGRP, &arg), arg == sid); -} -END_TEST() - -BARRIER() - -FN_TEST(nonleader_set_foreground) -{ - pid_t arg = child; + // Make sure the tests won't be run concurrently if (is_leader()) - return; - - // Only session leaders can use `TIOCSPGRP`, but we're not the leader - - TEST_ERRNO(ioctl(master, TIOCSPGRP, &arg), ENOTTY); - TEST_ERRNO(ioctl(slave, TIOCSPGRP, &arg), ENOTTY); + TEST_SUCC(usleep(100 * 1000)); } END_TEST() FN_SETUP(cleanup) { - int status; - - if (!is_leader()) - return; - - CHECK_WITH(wait(&status), - WIFEXITED(status) && WEXITSTATUS(status) == 0); + setup_kill_child(); } END_SETUP() diff --git a/test/syscall_test/blocklists/pty_test b/test/syscall_test/blocklists/pty_test index fb772ef7..9a1b84db 100644 --- a/test/syscall_test/blocklists/pty_test +++ b/test/syscall_test/blocklists/pty_test @@ -24,9 +24,5 @@ PtyTest.SwitchTwiceMultiline JobControlTest.SetTTYBadArg JobControlTest.SetTTYDifferentSession JobControlTest.ReleaseTTYSignals -# FIXME: `ReleaseTTYNonLeader` does pass in Linux, but is disabled because we -# cannot mimic the exact Linux behavior due to some design differences. -# See . -JobControlTest.ReleaseTTYNonLeader JobControlTest.SetForegroundProcessGroup JobControlTest.SetForegroundProcessGroupEmptyProcessGroup