From 4bbee00650ab6f79d79dd6ec155563d6b4dc29dd Mon Sep 17 00:00:00 2001 From: Tom Date: Tue, 8 Dec 2020 19:04:05 -0700 Subject: [PATCH] Kernel: disown should unblock any potential waiters This is necessary because if a process changes the state to Stopped or resumes from that state, a wait entry is created in the parent process. So, if a child process does this before disown is called, we need to clear those entries to avoid leaking references/zombies that won't be cleaned up until the former parent exits. This also should solve an even more unlikely corner case where another thread is waiting on a pid that is being disowned by another thread. --- Kernel/Process.cpp | 5 +++++ Kernel/Process.h | 1 + Kernel/Syscalls/disown.cpp | 1 + Kernel/Thread.h | 5 ++++- Kernel/ThreadBlockers.cpp | 44 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 55 insertions(+), 1 deletion(-) diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index d934791be85..620a17c615e 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -642,6 +642,11 @@ void Process::finalize(Thread& last_thread) m_wait_block_condition.finalize(); } +void Process::disowned_by_waiter(Process& process) +{ + m_wait_block_condition.disowned_by_waiter(process); +} + void Process::unblock_waiters(Thread& thread, Thread::WaitBlocker::UnblockFlags flags, u8 signal) { if (auto parent = Process::from_pid(ppid())) diff --git a/Kernel/Process.h b/Kernel/Process.h index 94c8181cf67..03f543d14da 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -488,6 +488,7 @@ public: KResultOr peek_user_data(Userspace address); KResult poke_user_data(Userspace address, u32 data); + void disowned_by_waiter(Process& process); void unblock_waiters(Thread&, Thread::WaitBlocker::UnblockFlags, u8 signal = 0); Thread::WaitBlockCondition& wait_block_condition() { return m_wait_block_condition; } diff --git a/Kernel/Syscalls/disown.cpp b/Kernel/Syscalls/disown.cpp index 6c6368aecdf..3c7ddf5eab4 100644 --- a/Kernel/Syscalls/disown.cpp +++ b/Kernel/Syscalls/disown.cpp @@ -37,6 +37,7 @@ int Process::sys$disown(ProcessID pid) if (process->ppid() != this->pid()) return -ECHILD; process->m_ppid = 0; + process->disowned_by_waiter(*this); return 0; } } diff --git a/Kernel/Thread.h b/Kernel/Thread.h index 4086d40621d..77aa4940c02 100644 --- a/Kernel/Thread.h +++ b/Kernel/Thread.h @@ -644,7 +644,8 @@ public: enum class UnblockFlags { Terminated, Stopped, - Continued + Continued, + Disowned }; WaitBlocker(int wait_options, idtype_t id_type, pid_t id, KResultOr& result); @@ -658,6 +659,7 @@ public: bool is_wait() const { return !(m_wait_options & WNOWAIT); } private: + void do_was_disowned(); void do_set_result(const siginfo_t&); const int m_wait_options; @@ -681,6 +683,7 @@ public: { } + void disowned_by_waiter(Process&); bool unblock(Thread&, WaitBlocker::UnblockFlags, u8); void try_unblock(WaitBlocker&); void finalize(); diff --git a/Kernel/ThreadBlockers.cpp b/Kernel/ThreadBlockers.cpp index ae31751bbe1..c46ef773977 100644 --- a/Kernel/ThreadBlockers.cpp +++ b/Kernel/ThreadBlockers.cpp @@ -412,8 +412,33 @@ void Thread::WaitBlockCondition::try_unblock(Thread::WaitBlocker& blocker) } } +void Thread::WaitBlockCondition::disowned_by_waiter(Process& process) +{ + ScopedSpinLock lock(m_lock); + if (m_finalized) + return; + for (size_t i = 0; i < m_threads.size();) { + auto& info = m_threads[i]; + if (&info.thread->process() == &process) { + do_unblock([&](Blocker& b, void*) { + ASSERT(b.blocker_type() == Blocker::Type::Wait); + auto& blocker = static_cast(b); + bool did_unblock = blocker.unblock(info.thread, WaitBlocker::UnblockFlags::Disowned, 0, false); + ASSERT(did_unblock); // disowning must unblock everyone + return true; + }); + m_threads.remove(i); + continue; + } + + i++; + } +} + bool Thread::WaitBlockCondition::unblock(Thread& thread, WaitBlocker::UnblockFlags flags, u8 signal) { + ASSERT(flags != WaitBlocker::UnblockFlags::Disowned); + bool did_unblock_any = false; bool did_wait = false; bool was_waited_already = false; @@ -565,6 +590,13 @@ void Thread::WaitBlocker::was_unblocked(bool) current_thread->try_dispatch_one_pending_signal(SIGCHLD); } +void Thread::WaitBlocker::do_was_disowned() +{ + ASSERT(!m_did_unblock); + m_did_unblock = true; + m_result = KResult(-ECHILD); +} + void Thread::WaitBlocker::do_set_result(const siginfo_t& result) { ASSERT(!m_did_unblock); @@ -599,6 +631,10 @@ bool Thread::WaitBlocker::unblock(Thread& thread, UnblockFlags flags, u8 signal, return false; break; case P_ALL: + if (flags == UnblockFlags::Disowned) { + // Generic waiter won't be unblocked by disown + return false; + } break; default: ASSERT_NOT_REACHED(); @@ -621,6 +657,12 @@ bool Thread::WaitBlocker::unblock(Thread& thread, UnblockFlags flags, u8 signal, if (!(m_wait_options & WUNTRACED) && !thread.is_traced()) return false; break; + case UnblockFlags::Disowned: + ScopedSpinLock lock(m_lock); + // Disowning must unblock anyone waiting for this process explicitly + if (!m_did_unblock) + do_was_disowned(); + return true; } if (flags == UnblockFlags::Terminated) { @@ -645,6 +687,7 @@ bool Thread::WaitBlocker::unblock(Thread& thread, UnblockFlags flags, u8 signal, switch (flags) { case UnblockFlags::Terminated: + case UnblockFlags::Disowned: ASSERT_NOT_REACHED(); case UnblockFlags::Stopped: siginfo.si_code = CLD_STOPPED; @@ -665,6 +708,7 @@ bool Thread::WaitBlocker::unblock(Thread& thread, UnblockFlags flags, u8 signal, if (!from_add_blocker) { // Only call unblock if we weren't called from within set_block_condition! + ASSERT(flags != UnblockFlags::Disowned); unblock_from_blocker(); } // Because this may be called from add_blocker, in which case we should