diff --git a/Kernel/Bus/USB/UHCIController.cpp b/Kernel/Bus/USB/UHCIController.cpp index 7c0a565ff4..e3d834c067 100644 --- a/Kernel/Bus/USB/UHCIController.cpp +++ b/Kernel/Bus/USB/UHCIController.cpp @@ -144,9 +144,11 @@ KResultOr ProcFSUSBBusFolder::entries_count() const KResult ProcFSUSBBusFolder::traverse_as_directory(unsigned fsid, Function callback) const { ScopedSpinLock lock(m_lock); - VERIFY(m_parent_folder); + auto parent_folder = m_parent_folder.strong_ref(); + // Note: if the parent folder is null, it means something bad happened as this should not happen for the USB folder. + VERIFY(parent_folder); callback({ ".", { fsid, component_index() }, 0 }); - callback({ "..", { fsid, m_parent_folder->component_index() }, 0 }); + callback({ "..", { fsid, parent_folder->component_index() }, 0 }); for (auto& device_node : m_device_nodes) { InodeIdentifier identifier = { fsid, device_node.component_index() }; diff --git a/Kernel/FileSystem/ProcFS.cpp b/Kernel/FileSystem/ProcFS.cpp index e90b96d064..6dac1946a0 100644 --- a/Kernel/FileSystem/ProcFS.cpp +++ b/Kernel/FileSystem/ProcFS.cpp @@ -56,9 +56,12 @@ void ProcFSComponentsRegistrar::register_new_process(Process& new_process) void ProcFSComponentsRegistrar::unregister_process(Process& deleted_process) { - auto process_folder = m_root_folder->process_folder_for(deleted_process); - VERIFY(process_folder); + auto process_folder = m_root_folder->process_folder_for(deleted_process).release_nonnull(); + process_folder->prepare_for_deletion(); process_folder->m_list_node.remove(); + dbgln_if(PROCFS_DEBUG, "ProcFSExposedFolder ref_count now: {}", process_folder->ref_count()); + // Note: Let's ensure we are the last holder of the ProcFSProcessFolder object before it can be deleted for good + VERIFY(process_folder->ref_count() == 1); } RefPtr ProcFS::create() diff --git a/Kernel/ProcessExposed.cpp b/Kernel/ProcessExposed.cpp index fb883cad7b..ef1100a3d2 100644 --- a/Kernel/ProcessExposed.cpp +++ b/Kernel/ProcessExposed.cpp @@ -157,7 +157,10 @@ KResult ProcFSProcessInformation::refresh_data(FileDescription& description) con // For process-specific inodes, hold the process's ptrace lock across refresh // and refuse to load data if the process is not dumpable. // Without this, files opened before a process went non-dumpable could still be used for dumping. - auto process = const_cast(*this).m_parent_folder->m_associated_process; + auto parent_folder = const_cast(*this).m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); + auto process = parent_folder->m_associated_process; process->ptrace_lock().lock(); if (!process->is_dumpable()) { process->ptrace_lock().unlock(); @@ -240,9 +243,11 @@ RefPtr ProcFSExposedFolder::lookup(StringView name) KResult ProcFSExposedFolder::traverse_as_directory(unsigned fsid, Function callback) const { Locker locker(ProcFSComponentsRegistrar::the().m_lock); - VERIFY(m_parent_folder); + auto parent_folder = m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); callback({ ".", { fsid, component_index() }, 0 }); - callback({ "..", { fsid, m_parent_folder->component_index() }, 0 }); + callback({ "..", { fsid, parent_folder->component_index() }, 0 }); for (auto& component : m_components) { InodeIdentifier identifier = { fsid, component.component_index() }; diff --git a/Kernel/ProcessExposed.h b/Kernel/ProcessExposed.h index d4cff9b8f1..0b3a3fd28f 100644 --- a/Kernel/ProcessExposed.h +++ b/Kernel/ProcessExposed.h @@ -92,7 +92,9 @@ private: InodeIndex m_component_index {}; }; -class ProcFSExposedFolder : public ProcFSExposedComponent { +class ProcFSExposedFolder + : public ProcFSExposedComponent + , public Weakable { friend class ProcFSProcessFolder; friend class ProcFSComponentsRegistrar; @@ -104,8 +106,9 @@ public: virtual void prepare_for_deletion() override { - m_components.clear(); - m_parent_folder.clear(); + for (auto& component : m_components) { + component.prepare_for_deletion(); + } } virtual mode_t required_mode() const override { return 0555; } @@ -115,7 +118,7 @@ protected: explicit ProcFSExposedFolder(StringView name); ProcFSExposedFolder(StringView name, const ProcFSExposedFolder& parent_folder); NonnullRefPtrVector m_components; - RefPtr m_parent_folder; + WeakPtr m_parent_folder; }; class ProcFSExposedLink : public ProcFSExposedComponent { @@ -134,7 +137,8 @@ protected: class ProcFSRootFolder; class ProcFSProcessInformation; -class ProcFSProcessFolder final : public ProcFSExposedFolder { +class ProcFSProcessFolder final + : public ProcFSExposedFolder { friend class ProcFSComponentsRegistrar; friend class ProcFSRootFolder; friend class ProcFSProcessInformation; @@ -235,8 +239,8 @@ public: virtual KResultOr read_bytes(off_t offset, size_t count, UserOrKernelBuffer& buffer, FileDescription* description) const override; - virtual uid_t owner_user() const override { return m_parent_folder->m_associated_process->uid(); } - virtual gid_t owner_group() const override { return m_parent_folder->m_associated_process->gid(); } + virtual uid_t owner_user() const override { return m_parent_folder.strong_ref()->m_associated_process->uid(); } + virtual gid_t owner_group() const override { return m_parent_folder.strong_ref()->m_associated_process->gid(); } protected: ProcFSProcessInformation(StringView name, const ProcFSProcessFolder& process_folder) @@ -248,7 +252,7 @@ protected: virtual KResult refresh_data(FileDescription&) const override; virtual bool output(KBufferBuilder& builder) = 0; - NonnullRefPtr m_parent_folder; + WeakPtr m_parent_folder; mutable SpinLock m_refresh_lock; }; diff --git a/Kernel/ProcessSpecificExposed.cpp b/Kernel/ProcessSpecificExposed.cpp index ab8fb29f6b..f711769edb 100644 --- a/Kernel/ProcessSpecificExposed.cpp +++ b/Kernel/ProcessSpecificExposed.cpp @@ -83,24 +83,29 @@ private: , m_process_folder(parent_folder) { } - RefPtr m_process_folder; + WeakPtr m_process_folder; mutable Lock m_lock; }; KResultOr ProcFSProcessStacks::entries_count() const { Locker locker(m_lock); - auto process = m_process_folder->m_associated_process; - return process->thread_count(); + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); + return parent_folder->m_associated_process->thread_count(); } KResult ProcFSProcessStacks::traverse_as_directory(unsigned fsid, Function callback) const { Locker locker(m_lock); + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); callback({ ".", { fsid, component_index() }, 0 }); - callback({ "..", { fsid, m_parent_folder->component_index() }, 0 }); + callback({ "..", { fsid, parent_folder->component_index() }, 0 }); - auto process = m_process_folder->m_associated_process; + auto process = parent_folder->m_associated_process; process->for_each_thread([&](const Thread& thread) { int tid = thread.tid().value(); InodeIdentifier identifier = { fsid, thread.global_procfs_inode_index() }; @@ -112,13 +117,16 @@ KResult ProcFSProcessStacks::traverse_as_directory(unsigned fsid, Function ProcFSProcessStacks::lookup(StringView name) { Locker locker(m_lock); - auto process = m_process_folder->m_associated_process; + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return nullptr; + auto process = parent_folder->m_associated_process; RefPtr procfd_stack; // FIXME: Try to exit the loop earlier process->for_each_thread([&](const Thread& thread) { int tid = thread.tid().value(); if (name == String::number(tid)) { - procfd_stack = ProcFSThreadStack::create(*m_process_folder, *this, thread); + procfd_stack = ProcFSThreadStack::create(*parent_folder, *this, thread); } }); return procfd_stack; @@ -177,24 +185,30 @@ private: , m_process_folder(parent_folder) { } - RefPtr m_process_folder; + WeakPtr m_process_folder; mutable Lock m_lock; }; KResultOr ProcFSProcessFileDescriptions::entries_count() const { Locker locker(m_lock); - return m_process_folder->m_associated_process->fds().open_count(); + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); + return parent_folder->m_associated_process->fds().open_count(); } KResult ProcFSProcessFileDescriptions::traverse_as_directory(unsigned fsid, Function callback) const { Locker locker(m_lock); + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); callback({ ".", { fsid, component_index() }, 0 }); - callback({ "..", { fsid, m_parent_folder->component_index() }, 0 }); + callback({ "..", { fsid, parent_folder->component_index() }, 0 }); - auto process = m_process_folder->m_associated_process; + auto process = parent_folder->m_associated_process; size_t count = 0; - m_process_folder->m_associated_process->fds().enumerate([&](auto& file_description_metadata) { + process->fds().enumerate([&](auto& file_description_metadata) { if (!file_description_metadata.is_valid()) { count++; return; @@ -208,11 +222,14 @@ KResult ProcFSProcessFileDescriptions::traverse_as_directory(unsigned fsid, Func RefPtr ProcFSProcessFileDescriptions::lookup(StringView name) { Locker locker(m_lock); - auto process = m_process_folder->m_associated_process; + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return nullptr; + auto process = parent_folder->m_associated_process; RefPtr procfd_fd; // FIXME: Try to exit the loop earlier size_t count = 0; - m_process_folder->m_associated_process->fds().enumerate([&](auto& file_description_metadata) { + process->fds().enumerate([&](auto& file_description_metadata) { if (!file_description_metadata.is_valid()) { count++; return; @@ -239,8 +256,11 @@ private: } virtual bool output(KBufferBuilder& builder) override { + auto parent_folder = m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return false; JsonArraySerializer array { builder }; - for (auto& unveiled_path : m_parent_folder->m_associated_process->unveiled_paths()) { + for (auto& unveiled_path : parent_folder->m_associated_process->unveiled_paths()) { if (!unveiled_path.was_explicitly_unveiled()) continue; auto obj = array.add_object(); @@ -278,11 +298,15 @@ private: virtual bool output(KBufferBuilder& builder) override { InterruptDisabler disabler; - if (!m_parent_folder->m_associated_process->perf_events()) { - dbgln("ProcFS: No perf events for {}", m_parent_folder->m_associated_process->pid()); + auto parent_folder = m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return false; + auto process = parent_folder->m_associated_process; + if (!process->perf_events()) { + dbgln("ProcFS: No perf events for {}", process->pid()); return false; } - return m_parent_folder->m_associated_process->perf_events()->to_json(builder); + return process->perf_events()->to_json(builder); } }; @@ -300,8 +324,11 @@ private: } virtual bool output(KBufferBuilder& builder) override { + auto parent_folder = m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return false; JsonArraySerializer array { builder }; - auto process = m_parent_folder->m_associated_process; + auto process = parent_folder->m_associated_process; if (process->fds().open_count() == 0) { array.finish(); return true; @@ -348,10 +375,13 @@ private: } virtual bool acquire_link(KBufferBuilder& builder) override { - builder.append_bytes(m_parent_process_directory->m_associated_process->root_directory_relative_to_global_root().absolute_path().to_byte_buffer()); + auto parent_folder = m_parent_process_directory.strong_ref(); + if (parent_folder.is_null()) + return false; + builder.append_bytes(parent_folder->m_associated_process->root_directory_relative_to_global_root().absolute_path().to_byte_buffer()); return true; } - NonnullRefPtr m_parent_process_directory; + WeakPtr m_parent_process_directory; }; class ProcFSProcessVirtualMemory final : public ProcFSProcessInformation { @@ -368,7 +398,10 @@ private: } virtual bool output(KBufferBuilder& builder) override { - auto process = m_parent_folder->m_associated_process; + auto parent_folder = m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return false; + auto process = parent_folder->m_associated_process; JsonArraySerializer array { builder }; { ScopedSpinLock lock(process->space().get_lock()); @@ -428,11 +461,14 @@ private: } virtual bool acquire_link(KBufferBuilder& builder) override { - builder.append_bytes(m_parent_process_directory->m_associated_process->current_directory().absolute_path().bytes()); + auto parent_folder = m_parent_process_directory.strong_ref(); + if (parent_folder.is_null()) + return false; + builder.append_bytes(parent_folder->m_associated_process->current_directory().absolute_path().bytes()); return true; } - NonnullRefPtr m_parent_process_directory; + WeakPtr m_parent_process_directory; }; class ProcFSProcessBinary final : public ProcFSExposedLink { @@ -444,7 +480,10 @@ public: virtual mode_t required_mode() const override { - if (!m_parent_process_directory->m_associated_process->executable()) + auto parent_folder = m_parent_process_directory.strong_ref(); + if (parent_folder.is_null()) + return false; + if (!parent_folder->m_associated_process->executable()) return 0; return ProcFSExposedComponent::required_mode(); } @@ -457,14 +496,17 @@ private: } virtual bool acquire_link(KBufferBuilder& builder) override { - auto* custody = m_parent_process_directory->m_associated_process->executable(); + auto parent_folder = m_parent_process_directory.strong_ref(); + if (parent_folder.is_null()) + return false; + auto* custody = parent_folder->m_associated_process->executable(); if (!custody) return false; builder.append(custody->absolute_path().bytes()); return true; } - NonnullRefPtr m_parent_process_directory; + WeakPtr m_parent_process_directory; }; NonnullRefPtr ProcFSProcessFolder::create(const Process& process) diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index 7794a2afdd..e11244ea8d 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -459,8 +459,12 @@ void Thread::finalize_dying_threads() }); } for (auto* thread : dying_threads) { + RefPtr process = thread->process(); + dbgln_if(PROCESS_DEBUG, "Before finalization, {} has {} refs and its process has {}", + *thread, thread->ref_count(), thread->process().ref_count()); thread->finalize(); - + dbgln_if(PROCESS_DEBUG, "After finalization, {} has {} refs and its process has {}", + *thread, thread->ref_count(), thread->process().ref_count()); // This thread will never execute again, drop the running reference // NOTE: This may not necessarily drop the last reference if anything // else is still holding onto this thread!