From cf63447044f7f26bc955d6dbfceb10eedfbd3112 Mon Sep 17 00:00:00 2001 From: Ali Mohammad Pur Date: Thu, 24 Feb 2022 22:25:49 +0330 Subject: [PATCH] Kernel: Move signal handlers from being thread state to process state POSIX requires that sigaction() and friends set a _process-wide_ signal handler, so move signal handlers and flags inside Process. This also fixes a "pid/tid confusion" FIXME, as we can now send the signal to the process and let that decide which thread should get the signal (which is the thread with tid==pid, but that's now the Process's problem). Note that each thread still retains its signal mask, as that is local to each thread. --- Kernel/Process.cpp | 7 +++---- Kernel/Process.h | 6 ++++++ Kernel/Syscalls/sigaction.cpp | 4 +++- Kernel/Thread.cpp | 13 ++++++------- Kernel/Thread.h | 8 +------- 5 files changed, 19 insertions(+), 19 deletions(-) diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 2f637473fe2ba4..85d3e658021240 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -612,10 +612,9 @@ void Process::finalize() m_state.store(State::Dead, AK::MemoryOrder::memory_order_release); { - // FIXME: PID/TID BUG - if (auto parent_thread = Thread::from_tid(ppid().value())) { - if (parent_thread->process().is_user_process() && (parent_thread->m_signal_action_data[SIGCHLD].flags & SA_NOCLDWAIT) != SA_NOCLDWAIT) - parent_thread->send_signal(SIGCHLD, this); + if (auto parent_process = Process::from_pid(ppid())) { + if (parent_process->is_user_process() && (parent_process->m_signal_action_data[SIGCHLD].flags & SA_NOCLDWAIT) != SA_NOCLDWAIT) + (void)parent_process->send_signal(SIGCHLD, this); } } diff --git a/Kernel/Process.h b/Kernel/Process.h index 7a89ac9891fef8..97300c5d66606b 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -827,6 +827,12 @@ class Process final NonnullRefPtrVector m_threads_for_coredump; mutable RefPtr m_procfs_traits; + struct SignalActionData { + VirtualAddress handler_or_sigaction; + int flags { 0 }; + u32 mask { 0 }; + }; + Array m_signal_action_data; static_assert(sizeof(ProtectedValues) < (PAGE_SIZE)); alignas(4096) ProtectedValues m_protected_values; diff --git a/Kernel/Syscalls/sigaction.cpp b/Kernel/Syscalls/sigaction.cpp index 79450ca9741d1f..80e14c44a87ffe 100644 --- a/Kernel/Syscalls/sigaction.cpp +++ b/Kernel/Syscalls/sigaction.cpp @@ -58,15 +58,17 @@ ErrorOr Process::sys$sigaction(int signum, Userspace return EINVAL; InterruptDisabler disabler; // FIXME: This should use a narrower lock. Maybe a way to ignore signals temporarily? - auto& action = Thread::current()->m_signal_action_data[signum]; + auto& action = m_signal_action_data[signum]; if (user_old_act) { sigaction old_act {}; old_act.sa_flags = action.flags; old_act.sa_sigaction = reinterpret_cast(action.handler_or_sigaction.as_ptr()); + old_act.sa_mask = action.mask; TRY(copy_to_user(user_old_act, &old_act)); } if (user_act) { auto act = TRY(copy_typed_from_user(user_act)); + action.mask = act.sa_mask; action.flags = act.sa_flags; action.handler_or_sigaction = VirtualAddress { reinterpret_cast(act.sa_sigaction) }; } diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index ca7038f50de2e8..7286b87c9556b0 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -790,7 +790,7 @@ void Thread::reset_signals_for_exec() // The signal mask is preserved across execve(2). // The pending signal set is preserved across an execve(2). m_have_any_unmasked_pending_signals.store(false, AK::memory_order_release); - m_signal_action_data.fill({}); + m_signal_action_masks.fill({}); // A successful call to execve(2) removes any existing alternate signal stack m_alternative_signal_stack = 0; m_alternative_signal_stack_size = 0; @@ -902,7 +902,7 @@ static DefaultSignalAction default_signal_action(u8 signal) bool Thread::should_ignore_signal(u8 signal) const { VERIFY(signal < 32); - auto const& action = m_signal_action_data[signal]; + auto const& action = m_process->m_signal_action_data[signal]; if (action.handler_or_sigaction.is_null()) return default_signal_action(signal) == DefaultSignalAction::Ignore; return ((sighandler_t)action.handler_or_sigaction.get() == SIG_IGN); @@ -911,7 +911,7 @@ bool Thread::should_ignore_signal(u8 signal) const bool Thread::has_signal_handler(u8 signal) const { VERIFY(signal < 32); - auto const& action = m_signal_action_data[signal]; + auto const& action = m_process->m_signal_action_data[signal]; return !action.handler_or_sigaction.is_null(); } @@ -975,7 +975,7 @@ DispatchSignalResult Thread::dispatch_signal(u8 signal) return DispatchSignalResult::Deferred; } - auto& action = m_signal_action_data[signal]; + auto& action = m_process->m_signal_action_data[signal]; // FIXME: Implement SA_SIGINFO signal handlers. VERIFY(!(action.flags & SA_SIGINFO)); @@ -1045,7 +1045,7 @@ DispatchSignalResult Thread::dispatch_signal(u8 signal) ScopedAddressSpaceSwitcher switcher(m_process); u32 old_signal_mask = m_signal_mask; - u32 new_signal_mask = action.mask; + u32 new_signal_mask = m_signal_action_masks[signal].value_or(action.mask); if ((action.flags & SA_NODEFER) == SA_NODEFER) new_signal_mask &= ~(1 << (signal - 1)); else @@ -1182,8 +1182,7 @@ RegisterState& Thread::get_register_dump_from_stack() ErrorOr> Thread::try_clone(Process& process) { auto clone = TRY(Thread::try_create(process)); - auto signal_action_data_span = m_signal_action_data.span(); - signal_action_data_span.copy_to(clone->m_signal_action_data.span()); + m_signal_action_masks.span().copy_to(clone->m_signal_action_masks); clone->m_signal_mask = m_signal_mask; clone->m_fpu_state = m_fpu_state; clone->m_thread_specific_data = m_thread_specific_data; diff --git a/Kernel/Thread.h b/Kernel/Thread.h index 11d33638482ca8..a0b5324300be2f 100644 --- a/Kernel/Thread.h +++ b/Kernel/Thread.h @@ -46,12 +46,6 @@ enum class DispatchSignalResult { Continue }; -struct SignalActionData { - VirtualAddress handler_or_sigaction; - u32 mask { 0 }; - int flags { 0 }; -}; - struct ThreadSpecificData { ThreadSpecificData* self; }; @@ -1225,7 +1219,7 @@ class Thread NonnullOwnPtr m_kernel_stack_region; VirtualAddress m_thread_specific_data; Optional m_thread_specific_range; - Array m_signal_action_data; + Array, NSIG> m_signal_action_masks; Blocker* m_blocker { nullptr }; Kernel::Mutex* m_blocking_mutex { nullptr }; u32 m_lock_requested_count { 0 };