Fix some `kill`-related behavior

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<UserSignal>, ctx: &Context) -> Result<()> {
 /// any signal.
 pub fn kill_group(pgid: Pgid, signal: Option<UserSignal>, 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<UserSignal>, ctx: &Context) -> Resu
 /// any signal.
 pub fn tgkill(tid: Tid, tgid: Pid, signal: Option<UserSignal>, 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,
+            Errno::ESRCH,
-            "the combination of tgid and pid is not valid"
+            "the combination of the TGID and the TID is not valid"
         );
     }
 
@@ -107,15 +114,20 @@ pub fn tgkill(tid: Tid, tgid: Pid, signal: Option<UserSignal>, 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<UserSignal>, 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<UserSignal>, ctx: &Context) -> Result<()> {
@@ -125,36 +137,57 @@ fn kill_process(process: &Process, signal: Option<UserSignal>, 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 {
+            continue;
-                // If `signal` is `None`, only permission check is required.
+        }
-                return Ok(());
-            };
 
-            if !posix_thread.has_signal_blocked(*signum) {
+        let Some(ref signum) = signum else {
-                // Send the signal to any thread that does not block the signal.
+            // If `signal` is `None`, only permission check is required.
-                permitted_thread = Some(posix_thread);
+            return Ok(());
-                break;
+        };
-            } else if permitted_thread.is_none() {
+
-                // If all threads block the signal, send the signal to the first permitted thread.
+        found_permitted_thread = true;
-                permitted_thread = Some(posix_thread);
+
-            }
+        // 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 {
+    if !found_permitted_thread {
-        return_errno_with_message!(Errno::EPERM, "cannot send signal to the target process");
+        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<UserSignal>, 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(())
 }

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<Self> {
         // Reference:
         // <https://man7.org/linux/man-pages/man2/waitpid.2.html>
         // <https://man7.org/linux/man-pages/man2/kill.2.html>
-        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()))
         }
     }
 }

kernel/src/syscall/kill.rs

@@ -14,7 +14,7 @@ use crate::{
 };
 
 pub fn sys_kill(process_filter: u64, sig_num: u64, ctx: &Context) -> Result<SyscallReturn> {
-    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 {

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<SyscallReturn> {
     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();

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() {

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

test/src/apps/signal_c/kill.c

@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: MPL-2.0
+
+#define _GNU_SOURCE
+
+#include <pthread.h>
+#include <signal.h>
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <sys/wait.h>
+
+#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()

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