From 74f23ce23dc01e7b7697b810d2d7da1100a8d858 Mon Sep 17 00:00:00 2001 From: Ruihan Li Date: Thu, 2 Oct 2025 23:51:36 +0800 Subject: [PATCH] Fix some `kill`-related behavior --- kernel/src/process/kill.rs | 95 +++++++++++++++++--------- kernel/src/process/process_filter.rs | 15 ++-- kernel/src/syscall/kill.rs | 2 +- kernel/src/syscall/tgkill.rs | 7 +- kernel/src/syscall/wait4.rs | 2 +- test/src/apps/scripts/process.sh | 1 + test/src/apps/signal_c/kill.c | 95 ++++++++++++++++++++++++++ test/src/syscall/ltp/testcases/all.txt | 6 +- 8 files changed, 179 insertions(+), 44 deletions(-) create mode 100644 test/src/apps/signal_c/kill.c diff --git a/kernel/src/process/kill.rs b/kernel/src/process/kill.rs index 9b4d531cf..f4c07a07b 100644 --- a/kernel/src/process/kill.rs +++ b/kernel/src/process/kill.rs @@ -10,7 +10,10 @@ use super::{ }, Pgid, Pid, Process, Sid, Uid, }; -use crate::{prelude::*, thread::Tid}; +use crate::{ + prelude::*, + thread::{AsThread, Tid}, +}; /// Sends a signal to a process, using the current process as the sender. /// @@ -53,14 +56,19 @@ pub fn kill(pid: Pid, signal: Option, ctx: &Context) -> Result<()> { /// any signal. pub fn kill_group(pgid: Pgid, signal: Option, ctx: &Context) -> Result<()> { let process_group = process_table::get_process_group(&pgid) - .ok_or_else(|| Error::with_message(Errno::ESRCH, "target group does not exist"))?; + .ok_or_else(|| Error::with_message(Errno::ESRCH, "the target group does not exist"))?; + + let mut result = Ok(()); let inner = process_group.lock(); for process in inner.iter() { - kill_process(process, signal, ctx)?; + let res = kill_process(process, signal, ctx); + if res.is_err_and(|err| err.error() != Errno::EPERM) { + result = res; + } } - Ok(()) + result } /// Sends a signal to a target thread, using the current process @@ -70,20 +78,19 @@ pub fn kill_group(pgid: Pgid, signal: Option, ctx: &Context) -> Resu /// any signal. pub fn tgkill(tid: Tid, tgid: Pid, signal: Option, ctx: &Context) -> Result<()> { let thread = thread_table::get_thread(tid) - .ok_or_else(|| Error::with_message(Errno::ESRCH, "target thread does not exist"))?; - + .ok_or_else(|| Error::with_message(Errno::ESRCH, "the target thread does not exist"))?; if thread.is_exited() { return Ok(()); } let posix_thread = thread.as_posix_thread().unwrap(); - // Check tgid + // Check the TGID let pid = posix_thread.process().pid(); if pid != tgid { return_errno_with_message!( - Errno::EINVAL, - "the combination of tgid and pid is not valid" + Errno::ESRCH, + "the combination of the TGID and the TID is not valid" ); } @@ -107,15 +114,20 @@ pub fn tgkill(tid: Tid, tgid: Pid, signal: Option, ctx: &Context) -> /// The credentials of the current process will be checked to determine /// if it is authorized to send the signal to the target group. pub fn kill_all(signal: Option, ctx: &Context) -> Result<()> { + let mut result = Ok(()); + for process in process_table::process_table_mut().iter() { if Arc::ptr_eq(&ctx.process, process) || process.is_init_process() { continue; } - kill_process(process, signal, ctx)?; + let res = kill_process(process, signal, ctx); + if res.is_err_and(|err| err.error() != Errno::EPERM) { + result = res; + } } - Ok(()) + result } fn kill_process(process: &Process, signal: Option, ctx: &Context) -> Result<()> { @@ -125,36 +137,57 @@ fn kill_process(process: &Process, signal: Option, ctx: &Context) -> let signum = signal.map(|signal| signal.num()); let sender_ids = SignalSenderIds::for_current_thread(ctx, signum); - let mut permitted_thread = None; + let mut found_permitted_thread = false; + let mut thread_to_enqueue = None; for task in tasks.as_slice() { - let posix_thread = task.as_posix_thread().unwrap(); + let thread = task.as_thread().unwrap(); + let posix_thread = thread.as_posix_thread().unwrap(); - // First check permission + // Check permission if posix_thread .check_signal_perm(signum.as_ref(), &sender_ids) - .is_ok() + .is_err() { - let Some(ref signum) = signum else { - // If `signal` is `None`, only permission check is required. - return Ok(()); - }; + continue; + } - if !posix_thread.has_signal_blocked(*signum) { - // Send the signal to any thread that does not block the signal. - permitted_thread = Some(posix_thread); - break; - } else if permitted_thread.is_none() { - // If all threads block the signal, send the signal to the first permitted thread. - permitted_thread = Some(posix_thread); - } + let Some(ref signum) = signum else { + // If `signal` is `None`, only permission check is required. + return Ok(()); + }; + + found_permitted_thread = true; + + // FIXME: If the thread is exiting concurrently, it may miss the signal queued on it. + if thread.is_exited() { + continue; + } + + if !posix_thread.has_signal_blocked(*signum) { + // Send the signal to any alive thread that does not block the signal. + thread_to_enqueue = Some(posix_thread); + break; + } else if thread_to_enqueue.is_none() { + // If all threads block the signal, send it to the first permitted and alive thread. + // FIXME: If it exits later with the signals still blocked, it will miss the signal + // queued on it. + thread_to_enqueue = Some(posix_thread); } } - let Some(permitted_thread) = permitted_thread else { - return_errno_with_message!(Errno::EPERM, "cannot send signal to the target process"); + if !found_permitted_thread { + return_errno_with_message!( + Errno::EPERM, + "the signal cannot be sent to the target process" + ); + } + + let Some(thread_to_enqueue) = thread_to_enqueue else { + // All threads have exited. This is a zombie process. + return Ok(()); }; - // Since `permitted_thread` has been set, `signal` cannot be `None`. + // Since `thread_to_enqueue` has been set, `signal` cannot be `None`. let signal = signal.unwrap(); // Drop the signal if it's ignored. See explanation at `enqueue_signal_locked`. @@ -163,7 +196,7 @@ fn kill_process(process: &Process, signal: Option, ctx: &Context) -> return Ok(()); } - permitted_thread.enqueue_signal_locked(Box::new(signal), sig_dispositions); + thread_to_enqueue.enqueue_signal_locked(Box::new(signal), sig_dispositions); Ok(()) } diff --git a/kernel/src/process/process_filter.rs b/kernel/src/process/process_filter.rs index 9dc2907b7..21e3e5e99 100644 --- a/kernel/src/process/process_filter.rs +++ b/kernel/src/process/process_filter.rs @@ -40,25 +40,28 @@ impl ProcessFilter { } // For `wait4` and `kill`. - pub fn from_id(wait_pid: i32) -> Self { + pub fn from_id(wait_pid: i32) -> Result { // Reference: // // - if wait_pid < -1 { + if wait_pid == i32::MIN { + // Note that performing `-wait_pid` will overflow, so we return an error directly. + return_errno_with_message!(Errno::ESRCH, "the target group does not exist"); + } else if wait_pid < -1 { // "wait for any child process whose process group ID is equal to the absolute value of // `pid`" - ProcessFilter::WithPgid((-wait_pid).cast_unsigned()) + Ok(ProcessFilter::WithPgid((-wait_pid).cast_unsigned())) } else if wait_pid == -1 { // "wait for any child process" - ProcessFilter::Any + Ok(ProcessFilter::Any) } else if wait_pid == 0 { // "wait for any child process whose process group ID is equal to that of the calling // process at the time of the call to `waitpid()`" let pgid = current!().pgid(); - ProcessFilter::WithPgid(pgid) + Ok(ProcessFilter::WithPgid(pgid)) } else { // "wait for the child whose process ID is equal to the value of `pid`" - ProcessFilter::WithPid(wait_pid.cast_unsigned()) + Ok(ProcessFilter::WithPid(wait_pid.cast_unsigned())) } } } diff --git a/kernel/src/syscall/kill.rs b/kernel/src/syscall/kill.rs index 0b491ff34..c7729d41d 100644 --- a/kernel/src/syscall/kill.rs +++ b/kernel/src/syscall/kill.rs @@ -14,7 +14,7 @@ use crate::{ }; pub fn sys_kill(process_filter: u64, sig_num: u64, ctx: &Context) -> Result { - let process_filter = ProcessFilter::from_id(process_filter as _); + let process_filter = ProcessFilter::from_id(process_filter as _)?; let sig_num = if sig_num == 0 { None } else { diff --git a/kernel/src/syscall/tgkill.rs b/kernel/src/syscall/tgkill.rs index 71d077b6c..d54c92132 100644 --- a/kernel/src/syscall/tgkill.rs +++ b/kernel/src/syscall/tgkill.rs @@ -13,16 +13,19 @@ use crate::{ thread::Tid, }; -/// tgkill send a signal to a thread with pid as its thread id, and tgid as its thread group id. +/// Sends a signal to a thread with `tid` as its thread ID, and `tgid` as its thread group ID. pub fn sys_tgkill(tgid: Pid, tid: Tid, sig_num: u8, ctx: &Context) -> Result { let sig_num = if sig_num == 0 { None } else { Some(SigNum::try_from(sig_num)?) }; - debug!("tgid = {}, pid = {}, sig_num = {:?}", tgid, tid, sig_num); + if tgid.cast_signed() < 0 || tid.cast_signed() < 0 { + return_errno_with_message!(Errno::EINVAL, "negative TGIDs or TIDs are not valid"); + } + let signal = sig_num.map(|sig_num| { let pid = ctx.process.pid(); let uid = ctx.posix_thread.credentials().ruid(); diff --git a/kernel/src/syscall/wait4.rs b/kernel/src/syscall/wait4.rs index 58a28a13f..9db8a6e5f 100644 --- a/kernel/src/syscall/wait4.rs +++ b/kernel/src/syscall/wait4.rs @@ -20,7 +20,7 @@ pub fn sys_wait4( wait_pid as i32, status_ptr, wait_options ); debug!("wait4 current pid = {}", ctx.process.pid()); - let process_filter = ProcessFilter::from_id(wait_pid as _); + let process_filter = ProcessFilter::from_id(wait_pid as _)?; let wait_status = do_wait(process_filter, wait_options, ctx).map_err(|err| match err.error() { diff --git a/test/src/apps/scripts/process.sh b/test/src/apps/scripts/process.sh index 46745f5d9..8707c1ece 100755 --- a/test/src/apps/scripts/process.sh +++ b/test/src/apps/scripts/process.sh @@ -49,6 +49,7 @@ pty/pty_blocking sched/sched_attr sched/sched_attr_idle shm/posix_shm +signal_c/kill signal_c/parent_death_signal signal_c/sigaltstack signal_c/signal_fpu diff --git a/test/src/apps/signal_c/kill.c b/test/src/apps/signal_c/kill.c new file mode 100644 index 000000000..860649dd1 --- /dev/null +++ b/test/src/apps/signal_c/kill.c @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: MPL-2.0 + +#define _GNU_SOURCE + +#include +#include +#include +#include +#include + +#include "../test.h" + +FN_SETUP(setpgrp) +{ + CHECK(setpgrp()); +} +END_SETUP() + +FN_TEST(kill_dead_process) +{ + pid_t ppid, cpid; + int status; + + ppid = TEST_SUCC(getpid()); + cpid = TEST_SUCC(fork()); + if (cpid == 0) { + exit(EXIT_SUCCESS); + } + usleep(200 * 1000); + + TEST_SUCC(kill(ppid, SIGCHLD)); + TEST_SUCC(kill(-ppid, SIGCHLD)); + + // Killing dead processes will succeed. + TEST_SUCC(kill(cpid, SIGCHLD)); + TEST_ERRNO(kill(-cpid, SIGCHLD), ESRCH); + + TEST_RES(wait(&status), + _ret == cpid && WIFEXITED(status) && WEXITSTATUS(status) == 0); +} +END_TEST() + +FN_TEST(kill_dead_group) +{ + pid_t ppid, cpid; + int status; + + ppid = TEST_SUCC(getpid()); + cpid = TEST_SUCC(fork()); + if (cpid == 0) { + CHECK(setpgrp()); + exit(EXIT_SUCCESS); + } + usleep(200 * 1000); + + TEST_SUCC(kill(ppid, SIGCHLD)); + TEST_SUCC(kill(-ppid, SIGCHLD)); + + // Killing dead process groups will succeed. + TEST_SUCC(kill(cpid, SIGCHLD)); + TEST_SUCC(kill(-cpid, SIGCHLD)); + + TEST_RES(wait(&status), + _ret == cpid && WIFEXITED(status) && WEXITSTATUS(status) == 0); +} +END_TEST() + +static void *background_thread(void *data) +{ + sleep(3600); + return NULL; +} + +FN_TEST(kill_dead_thread) +{ + pid_t cpid; + int status; + + cpid = TEST_SUCC(fork()); + if (cpid == 0) { + pthread_t tid; + CHECK(pthread_create(&tid, NULL, background_thread, NULL)); + syscall(SYS_exit, 0); + } + usleep(200 * 1000); + + // Killing dead threads will succeed. + TEST_SUCC(kill(cpid, SIGCHLD)); + TEST_SUCC(tgkill(cpid, cpid, SIGCHLD)); + + TEST_SUCC(kill(cpid, SIGKILL)); + TEST_RES(wait(&status), _ret == cpid && WIFSIGNALED(status) && + WTERMSIG(status) == SIGKILL); +} +END_TEST() diff --git a/test/src/syscall/ltp/testcases/all.txt b/test/src/syscall/ltp/testcases/all.txt index 857d5431e..73d1fc48b 100644 --- a/test/src/syscall/ltp/testcases/all.txt +++ b/test/src/syscall/ltp/testcases/all.txt @@ -687,7 +687,7 @@ ioprio_set02 # kcmp03 kill02 -# kill03 +kill03 # kill05 kill06 # kill07 @@ -1647,9 +1647,9 @@ sysinfo02 # syslog11 # syslog12 -# tgkill01 +tgkill01 # tgkill02 -# tgkill03 +tgkill03 time01