Never queue an ignored signal

kernel/src/process/kill.rs

@@ -27,6 +27,7 @@ pub fn kill(pid: Pid, signal: Option<UserSignal>, ctx: &Context) -> Result<()> {
         };
 
         if !ctx.posix_thread.has_signal_blocked(signal.num()) {
+            // Killing the current thread does not raise any permission issues.
             ctx.posix_thread.enqueue_signal(Box::new(signal));
             return Ok(());
         }
@@ -92,6 +93,8 @@ pub fn tgkill(tid: Tid, tgid: Pid, signal: Option<UserSignal>, ctx: &Context) ->
     posix_thread.check_signal_perm(signum.as_ref(), &sender)?;
 
     if let Some(signal) = signal {
+        // We've checked the permission issues above.
+        // FIXME: We should take some lock while checking the permission to avoid race conditions.
         posix_thread.enqueue_signal(Box::new(signal));
     }
 
@@ -117,6 +120,7 @@ pub fn kill_all(signal: Option<UserSignal>, ctx: &Context) -> Result<()> {
 }
 
 fn kill_process(process: &Process, signal: Option<UserSignal>, ctx: &Context) -> Result<()> {
+    let sig_dispositions = process.sig_dispositions().lock();
     let tasks = process.tasks().lock();
 
     let signum = signal.map(|signal| signal.num());
@@ -132,16 +136,16 @@ fn kill_process(process: &Process, signal: Option<UserSignal>, ctx: &Context) ->
             .is_ok()
         {
             let Some(ref signum) = signum else {
-                // If signal is None, only permission check is required
+                // If `signal` is `None`, only permission check is required.
                 return Ok(());
             };
 
             if !posix_thread.has_signal_blocked(*signum) {
-                // Send signal to any thread that does not blocks the signal.
+                // Send the signal to any thread that does not block the signal.
-                let signal = signal.unwrap();
+                permitted_thread = Some(posix_thread);
-                posix_thread.enqueue_signal(Box::new(signal));
+                break;
-                return Ok(());
             } 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);
             }
         }
@@ -151,11 +155,16 @@ fn kill_process(process: &Process, signal: Option<UserSignal>, ctx: &Context) ->
         return_errno_with_message!(Errno::EPERM, "cannot send signal to the target process");
     };
 
-    // If signal is None, only permission check is required
+    // Since `permitted_thread` has been set, `signal` cannot be `None`.
-    let Some(signal) = signal else { return Ok(()) };
+    let signal = signal.unwrap();
 
-    // If all threads block the signal, send signal to the first thread.
+    // Drop the signal if it's ignored. See explanation at `enqueue_signal_locked`.
-    permitted_thread.enqueue_signal(Box::new(signal));
+    let signum = signal.num();
+    if sig_dispositions.get(signum).will_ignore(signum) {
+        return Ok(());
+    }
+
+    permitted_thread.enqueue_signal_locked(Box::new(signal), sig_dispositions);
 
     Ok(())
 }

kernel/src/process/posix_thread/mod.rs

@@ -8,7 +8,7 @@ use ostd::sync::{RoArc, Waker};
 use super::{
     kill::SignalSenderIds,
     signal::{
-        sig_action::SigAction,
+        sig_disposition::SigDispositions,
         sig_mask::{AtomicSigMask, SigMask, SigSet},
         sig_num::SigNum,
         sig_queues::SigQueues,
@@ -199,14 +199,41 @@ impl PosixThread {
         *self.signalled_waker.lock() = None;
     }
 
-    /// Enqueues a thread-directed signal. This method should only be used for enqueue kernel
+    /// Enqueues a thread-directed signal.
-    /// signal and fault signal.
+    ///
+    /// This method does not perform permission checks on user signals. Therefore, unless the
+    /// caller can ensure that there are no permission issues, this method should be used for
+    /// enqueue kernel signals or fault signals.
     pub fn enqueue_signal(&self, signal: Box<dyn Signal>) {
-        let signal_number = signal.num();
+        let process = self.process();
+        let sig_dispositions = process.sig_dispositions().lock();
+
+        let signum = signal.num();
+        if sig_dispositions.get(signum).will_ignore(signum) {
+            return;
+        }
+
+        self.enqueue_signal_locked(signal, sig_dispositions);
+    }
+
+    /// Enqueues a thread-directed signal with locked dispositions.
+    ///
+    /// By locking dispositions, the caller should have already checked the signal is not to be
+    /// ignored.
+    //
+    // FIXME: According to Linux behavior, we should enqueue ignored signals blocked by all
+    // threads, as a thread may change the signal handler and unblock them in the future. However,
+    // achieving this behavior properly without maintaining a process-wide signal queue is
+    // difficult. For instance, if we randomly select a thread-wide signal queue, the thread that
+    // modifies the signal handler and unblocks the signal may not be the same one. Consequently,
+    // the current implementation uses a simpler mechanism that never enqueues any ignored signals.
+    pub(in crate::process) fn enqueue_signal_locked(
+        &self,
+        signal: Box<dyn Signal>,
+        _sig_dispositions: MutexGuard<SigDispositions>,
+    ) {
         self.sig_queues.enqueue(signal);
-        if self.process().sig_dispositions().lock().get(signal_number) != SigAction::Ign
+        if let Some(waker) = &*self.signalled_waker.lock() {
-            && let Some(waker) = &*self.signalled_waker.lock()
-        {
             waker.wake_up();
         }
     }

kernel/src/process/process/mod.rs

@@ -614,22 +614,29 @@ impl Process {
             return;
         }
 
-        // TODO: check that the signal is not user signal
+        let sig_dispositions = self.sig_dispositions.lock();
+
+        // Drop the signal if it's ignored. See explanation at `enqueue_signal_locked`.
+        let signum = signal.num();
+        if sig_dispositions.get(signum).will_ignore(signum) {
+            return;
+        }
 
-        // Enqueue signal to the first thread that does not block the signal
         let threads = self.tasks.lock();
+
+        // Enqueue the signal to the first thread that does not block the signal.
         for thread in threads.as_slice() {
             let posix_thread = thread.as_posix_thread().unwrap();
             if !posix_thread.has_signal_blocked(signal.num()) {
-                posix_thread.enqueue_signal(Box::new(signal));
+                posix_thread.enqueue_signal_locked(Box::new(signal), sig_dispositions);
                 return;
             }
         }
 
-        // If all threads block the signal, enqueue signal to the main thread
+        // If all threads block the signal, enqueue the signal to the main thread.
         let thread = threads.main();
         let posix_thread = thread.as_posix_thread().unwrap();
-        posix_thread.enqueue_signal(Box::new(signal));
+        posix_thread.enqueue_signal_locked(Box::new(signal), sig_dispositions);
     }
 
     /// Clears the parent death signal.

kernel/src/process/signal/sig_action.rs

@@ -77,6 +77,22 @@ impl SigAction {
             },
         }
     }
+
+    /// Returns whether signals will be ignored.
+    ///
+    /// Signals will be ignored because either
+    ///  * the signal action is explicitly set to ignore the signals, or
+    ///  * the signal action is default and the default action is to ignore the signals.
+    pub fn will_ignore(&self, signum: SigNum) -> bool {
+        match self {
+            SigAction::Dfl => {
+                let default_action = SigDefaultAction::from_signum(signum);
+                matches!(default_action, SigDefaultAction::Ign)
+            }
+            SigAction::Ign => true,
+            SigAction::User { .. } => false,
+        }
+    }
 }
 
 bitflags! {

kernel/src/syscall/rt_sigaction.rs

@@ -8,7 +8,7 @@ use crate::{
         signal::{
             c_types::sigaction_t,
             constants::{SIGKILL, SIGSTOP},
-            sig_action::{SigAction, SigDefaultAction},
+            sig_action::SigAction,
             sig_mask::SigSet,
             sig_num::SigNum,
         },
@@ -77,15 +77,8 @@ pub fn sys_rt_sigaction(
 // (for example, SIGCHLD), shall cause the pending signal to
 // be discarded, whether or not it is blocked
 fn discard_signals_if_ignored(ctx: &Context, signum: SigNum, sig_action: &SigAction) {
-    match sig_action {
+    if !sig_action.will_ignore(signum) {
-        SigAction::Dfl => {
+        return;
-            let default_action = SigDefaultAction::from_signum(signum);
-            if default_action != SigDefaultAction::Ign {
-                return;
-            }
-        }
-        SigAction::Ign => {}
-        SigAction::User { .. } => return,
     }
 
     let mask = SigSet::new_full() - signum;

test/apps/scripts/process.sh

@@ -40,6 +40,7 @@ sched/sched_attr
 shm/posix_shm
 signal_c/parent_death_signal
 signal_c/signal_test
+signal_c/signal_test2
 "
 
 for testcase in ${tests}

test/apps/signal_c/signal_test2.c

@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: MPL-2.0
+
+#include "../network/test.h"
+
+#include <unistd.h>
+#include <signal.h>
+
+static volatile int received_signals;
+
+static void signal_handler(int signum)
+{
+	++received_signals;
+}
+
+static sigset_t sigs;
+
+FN_SETUP(sigs)
+{
+	CHECK(sigemptyset(&sigs));
+	CHECK(sigaddset(&sigs, SIGCHLD));
+}
+END_SETUP()
+
+FN_TEST(kill_blocked_and_ignored)
+{
+	signal(SIGCHLD, SIG_DFL);
+
+	received_signals = 0;
+	TEST_RES(sigprocmask(SIG_BLOCK, &sigs, NULL), received_signals == 0);
+
+	received_signals = 0;
+	TEST_RES(kill(getpid(), SIGCHLD), received_signals == 0);
+
+	signal(SIGCHLD, &signal_handler);
+
+	// FIXME: Currently, Asterinas never queues an ignored signal, so this test
+	// will fail. See the comments at `PosixThread::enqueue_signal_locked` for
+	// more details.
+	//
+	// received_signals = 0;
+	// TEST_RES(sigprocmask(SIG_UNBLOCK, &sigs, NULL), received_signals == 1);
+	//
+	sigprocmask(SIG_UNBLOCK, &sigs, NULL);
+}
+END_TEST()
+
+FN_TEST(kill_blocked_not_ignored)
+{
+	signal(SIGCHLD, SIG_DFL);
+
+	received_signals = 0;
+	TEST_RES(sigprocmask(SIG_BLOCK, &sigs, NULL), received_signals == 0);
+
+	signal(SIGCHLD, &signal_handler);
+
+	received_signals = 0;
+	TEST_RES(kill(getpid(), SIGCHLD), received_signals == 0);
+
+	received_signals = 0;
+	TEST_RES(sigprocmask(SIG_UNBLOCK, &sigs, NULL), received_signals == 1);
+}
+END_TEST()
+
+FN_TEST(change_blocked_to_ignored)
+{
+	signal(SIGCHLD, &signal_handler);
+
+	received_signals = 0;
+	TEST_RES(sigprocmask(SIG_BLOCK, &sigs, NULL), received_signals == 0);
+
+	received_signals = 0;
+	TEST_RES(kill(getpid(), SIGCHLD), received_signals == 0);
+
+	signal(SIGCHLD, SIG_IGN);
+	signal(SIGCHLD, &signal_handler);
+
+	received_signals = 0;
+	TEST_RES(sigprocmask(SIG_UNBLOCK, &sigs, NULL), received_signals == 0);
+}
+END_TEST()