From 0661a0656b4b45b3fcefa6860900221a0cacdda5 Mon Sep 17 00:00:00 2001 From: Ruihan Li Date: Mon, 12 May 2025 11:22:47 +0800 Subject: [PATCH] Never queue an ignored signal --- kernel/src/process/kill.rs | 27 ++++++--- kernel/src/process/posix_thread/mod.rs | 41 ++++++++++--- kernel/src/process/process/mod.rs | 17 ++++-- kernel/src/process/signal/sig_action.rs | 16 +++++ kernel/src/syscall/rt_sigaction.rs | 13 +--- test/apps/scripts/process.sh | 1 + test/apps/signal_c/signal_test2.c | 80 +++++++++++++++++++++++++ 7 files changed, 164 insertions(+), 31 deletions(-) create mode 100644 test/apps/signal_c/signal_test2.c diff --git a/kernel/src/process/kill.rs b/kernel/src/process/kill.rs index c1c1ce2d8..e41743bbc 100644 --- a/kernel/src/process/kill.rs +++ b/kernel/src/process/kill.rs @@ -27,6 +27,7 @@ pub fn kill(pid: Pid, signal: Option, 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, 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, ctx: &Context) -> Result<()> { } fn kill_process(process: &Process, signal: Option, 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, 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. - let signal = signal.unwrap(); - posix_thread.enqueue_signal(Box::new(signal)); - return Ok(()); + // 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); } } @@ -151,11 +155,16 @@ fn kill_process(process: &Process, signal: Option, ctx: &Context) -> return_errno_with_message!(Errno::EPERM, "cannot send signal to the target process"); }; - // If signal is None, only permission check is required - let Some(signal) = signal else { return Ok(()) }; + // Since `permitted_thread` has been set, `signal` cannot be `None`. + let signal = signal.unwrap(); - // If all threads block the signal, send signal to the first thread. - permitted_thread.enqueue_signal(Box::new(signal)); + // 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 Ok(()); + } + + permitted_thread.enqueue_signal_locked(Box::new(signal), sig_dispositions); Ok(()) } diff --git a/kernel/src/process/posix_thread/mod.rs b/kernel/src/process/posix_thread/mod.rs index ae226c8a7..96d9a13db 100644 --- a/kernel/src/process/posix_thread/mod.rs +++ b/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 - /// signal and fault signal. + /// Enqueues a thread-directed 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) { - 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, + _sig_dispositions: MutexGuard, + ) { self.sig_queues.enqueue(signal); - if self.process().sig_dispositions().lock().get(signal_number) != SigAction::Ign - && let Some(waker) = &*self.signalled_waker.lock() - { + if let Some(waker) = &*self.signalled_waker.lock() { waker.wake_up(); } } diff --git a/kernel/src/process/process/mod.rs b/kernel/src/process/process/mod.rs index bfd24f3a6..5c37d1275 100644 --- a/kernel/src/process/process/mod.rs +++ b/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. diff --git a/kernel/src/process/signal/sig_action.rs b/kernel/src/process/signal/sig_action.rs index 1e9bb1f33..295a07b82 100644 --- a/kernel/src/process/signal/sig_action.rs +++ b/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! { diff --git a/kernel/src/syscall/rt_sigaction.rs b/kernel/src/syscall/rt_sigaction.rs index 5aceffcba..270ad7a15 100644 --- a/kernel/src/syscall/rt_sigaction.rs +++ b/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 { - SigAction::Dfl => { - let default_action = SigDefaultAction::from_signum(signum); - if default_action != SigDefaultAction::Ign { - return; - } - } - SigAction::Ign => {} - SigAction::User { .. } => return, + if !sig_action.will_ignore(signum) { + return; } let mask = SigSet::new_full() - signum; diff --git a/test/apps/scripts/process.sh b/test/apps/scripts/process.sh index b7c39359f..0074b7d9d 100755 --- a/test/apps/scripts/process.sh +++ b/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} diff --git a/test/apps/signal_c/signal_test2.c b/test/apps/signal_c/signal_test2.c new file mode 100644 index 000000000..e004cef57 --- /dev/null +++ b/test/apps/signal_c/signal_test2.c @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: MPL-2.0 + +#include "../network/test.h" + +#include +#include + +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()