Jelajahi Sumber

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.
Ali Mohammad Pur 3 tahun lalu
induk
melakukan
cf63447044
5 mengubah file dengan 19 tambahan dan 19 penghapusan
  1. 3 4
      Kernel/Process.cpp
  2. 6 0
      Kernel/Process.h
  3. 3 1
      Kernel/Syscalls/sigaction.cpp
  4. 6 7
      Kernel/Thread.cpp
  5. 1 7
      Kernel/Thread.h

+ 3 - 4
Kernel/Process.cpp

@@ -612,10 +612,9 @@ void Process::finalize()
     m_state.store(State::Dead, AK::MemoryOrder::memory_order_release);
     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);
         }
         }
     }
     }
 
 

+ 6 - 0
Kernel/Process.h

@@ -827,6 +827,12 @@ private:
     NonnullRefPtrVector<Thread> m_threads_for_coredump;
     NonnullRefPtrVector<Thread> m_threads_for_coredump;
 
 
     mutable RefPtr<ProcessProcFSTraits> m_procfs_traits;
     mutable RefPtr<ProcessProcFSTraits> m_procfs_traits;
+    struct SignalActionData {
+        VirtualAddress handler_or_sigaction;
+        int flags { 0 };
+        u32 mask { 0 };
+    };
+    Array<SignalActionData, NSIG> m_signal_action_data;
 
 
     static_assert(sizeof(ProtectedValues) < (PAGE_SIZE));
     static_assert(sizeof(ProtectedValues) < (PAGE_SIZE));
     alignas(4096) ProtectedValues m_protected_values;
     alignas(4096) ProtectedValues m_protected_values;

+ 3 - 1
Kernel/Syscalls/sigaction.cpp

@@ -58,15 +58,17 @@ ErrorOr<FlatPtr> Process::sys$sigaction(int signum, Userspace<const sigaction*>
         return EINVAL;
         return EINVAL;
 
 
     InterruptDisabler disabler; // FIXME: This should use a narrower lock. Maybe a way to ignore signals temporarily?
     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) {
     if (user_old_act) {
         sigaction old_act {};
         sigaction old_act {};
         old_act.sa_flags = action.flags;
         old_act.sa_flags = action.flags;
         old_act.sa_sigaction = reinterpret_cast<decltype(old_act.sa_sigaction)>(action.handler_or_sigaction.as_ptr());
         old_act.sa_sigaction = reinterpret_cast<decltype(old_act.sa_sigaction)>(action.handler_or_sigaction.as_ptr());
+        old_act.sa_mask = action.mask;
         TRY(copy_to_user(user_old_act, &old_act));
         TRY(copy_to_user(user_old_act, &old_act));
     }
     }
     if (user_act) {
     if (user_act) {
         auto act = TRY(copy_typed_from_user(user_act));
         auto act = TRY(copy_typed_from_user(user_act));
+        action.mask = act.sa_mask;
         action.flags = act.sa_flags;
         action.flags = act.sa_flags;
         action.handler_or_sigaction = VirtualAddress { reinterpret_cast<void*>(act.sa_sigaction) };
         action.handler_or_sigaction = VirtualAddress { reinterpret_cast<void*>(act.sa_sigaction) };
     }
     }

+ 6 - 7
Kernel/Thread.cpp

@@ -790,7 +790,7 @@ void Thread::reset_signals_for_exec()
     // The signal mask is preserved across execve(2).
     // The signal mask is preserved across execve(2).
     // The pending signal set is preserved across an 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_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
     // A successful call to execve(2) removes any existing alternate signal stack
     m_alternative_signal_stack = 0;
     m_alternative_signal_stack = 0;
     m_alternative_signal_stack_size = 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
 bool Thread::should_ignore_signal(u8 signal) const
 {
 {
     VERIFY(signal < 32);
     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())
     if (action.handler_or_sigaction.is_null())
         return default_signal_action(signal) == DefaultSignalAction::Ignore;
         return default_signal_action(signal) == DefaultSignalAction::Ignore;
     return ((sighandler_t)action.handler_or_sigaction.get() == SIG_IGN);
     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
 bool Thread::has_signal_handler(u8 signal) const
 {
 {
     VERIFY(signal < 32);
     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();
     return !action.handler_or_sigaction.is_null();
 }
 }
 
 
@@ -975,7 +975,7 @@ DispatchSignalResult Thread::dispatch_signal(u8 signal)
         return DispatchSignalResult::Deferred;
         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.
     // FIXME: Implement SA_SIGINFO signal handlers.
     VERIFY(!(action.flags & SA_SIGINFO));
     VERIFY(!(action.flags & SA_SIGINFO));
 
 
@@ -1045,7 +1045,7 @@ DispatchSignalResult Thread::dispatch_signal(u8 signal)
     ScopedAddressSpaceSwitcher switcher(m_process);
     ScopedAddressSpaceSwitcher switcher(m_process);
 
 
     u32 old_signal_mask = m_signal_mask;
     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)
     if ((action.flags & SA_NODEFER) == SA_NODEFER)
         new_signal_mask &= ~(1 << (signal - 1));
         new_signal_mask &= ~(1 << (signal - 1));
     else
     else
@@ -1182,8 +1182,7 @@ RegisterState& Thread::get_register_dump_from_stack()
 ErrorOr<NonnullRefPtr<Thread>> Thread::try_clone(Process& process)
 ErrorOr<NonnullRefPtr<Thread>> Thread::try_clone(Process& process)
 {
 {
     auto clone = TRY(Thread::try_create(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_signal_mask = m_signal_mask;
     clone->m_fpu_state = m_fpu_state;
     clone->m_fpu_state = m_fpu_state;
     clone->m_thread_specific_data = m_thread_specific_data;
     clone->m_thread_specific_data = m_thread_specific_data;

+ 1 - 7
Kernel/Thread.h

@@ -46,12 +46,6 @@ enum class DispatchSignalResult {
     Continue
     Continue
 };
 };
 
 
-struct SignalActionData {
-    VirtualAddress handler_or_sigaction;
-    u32 mask { 0 };
-    int flags { 0 };
-};
-
 struct ThreadSpecificData {
 struct ThreadSpecificData {
     ThreadSpecificData* self;
     ThreadSpecificData* self;
 };
 };
@@ -1225,7 +1219,7 @@ private:
     NonnullOwnPtr<Memory::Region> m_kernel_stack_region;
     NonnullOwnPtr<Memory::Region> m_kernel_stack_region;
     VirtualAddress m_thread_specific_data;
     VirtualAddress m_thread_specific_data;
     Optional<Memory::VirtualRange> m_thread_specific_range;
     Optional<Memory::VirtualRange> m_thread_specific_range;
-    Array<SignalActionData, NSIG> m_signal_action_data;
+    Array<Optional<u32>, NSIG> m_signal_action_masks;
     Blocker* m_blocker { nullptr };
     Blocker* m_blocker { nullptr };
     Kernel::Mutex* m_blocking_mutex { nullptr };
     Kernel::Mutex* m_blocking_mutex { nullptr };
     u32 m_lock_requested_count { 0 };
     u32 m_lock_requested_count { 0 };