Kernel/ProcFS: Clean dead processes properly

Now we use WeakPtrs to break Ref-counting cycle. Also, we call the
prepare_for_deletion method to ensure deleted objects are ready for
deletion. This is necessary to ensure we don't keep dead processes,
which would become zombies.

In addition to that, add some debug prints to aid debug in the future.

Kernel/Bus/USB/UHCIController.cpp

@@ -144,9 +144,11 @@ KResultOr<size_t> ProcFSUSBBusFolder::entries_count() const
 KResult ProcFSUSBBusFolder::traverse_as_directory(unsigned fsid, Function<bool(const FS::DirectoryEntryView&)> 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() };

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);
+    auto process_folder = m_root_folder->process_folder_for(deleted_process).release_nonnull();
-    VERIFY(process_folder);
+    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> ProcFS::create()

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<ProcFSProcessInformation&>(*this).m_parent_folder->m_associated_process;
+    auto parent_folder = const_cast<ProcFSProcessInformation&>(*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<ProcFSExposedComponent> ProcFSExposedFolder::lookup(StringView name)
 KResult ProcFSExposedFolder::traverse_as_directory(unsigned fsid, Function<bool(const FS::DirectoryEntryView&)> 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() };

Kernel/ProcessExposed.h

@@ -92,7 +92,9 @@ private:
     InodeIndex m_component_index {};
 };
 
-class ProcFSExposedFolder : public ProcFSExposedComponent {
+class ProcFSExposedFolder
+    : public ProcFSExposedComponent
+    , public Weakable<ProcFSExposedFolder> {
     friend class ProcFSProcessFolder;
     friend class ProcFSComponentsRegistrar;
 
@@ -104,8 +106,9 @@ public:
 
     virtual void prepare_for_deletion() override
     {
-        m_components.clear();
+        for (auto& component : m_components) {
-        m_parent_folder.clear();
+            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<ProcFSExposedComponent> m_components;
-    RefPtr<ProcFSExposedFolder> m_parent_folder;
+    WeakPtr<ProcFSExposedFolder> 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<size_t> 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 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->m_associated_process->gid(); }
+    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<ProcFSProcessFolder> m_parent_folder;
+    WeakPtr<ProcFSProcessFolder> m_parent_folder;
     mutable SpinLock<u8> m_refresh_lock;
 };
 

Kernel/ProcessSpecificExposed.cpp

@@ -83,24 +83,29 @@ private:
         , m_process_folder(parent_folder)
     {
     }
-    RefPtr<ProcFSProcessFolder> m_process_folder;
+    WeakPtr<ProcFSProcessFolder> m_process_folder;
     mutable Lock m_lock;
 };
 
 KResultOr<size_t> ProcFSProcessStacks::entries_count() const
 {
     Locker locker(m_lock);
-    auto process = m_process_folder->m_associated_process;
+    auto parent_folder = m_process_folder.strong_ref();
-    return process->thread_count();
+    if (parent_folder.is_null())
+        return KResult(EINVAL);
+    return parent_folder->m_associated_process->thread_count();
 }
 
 KResult ProcFSProcessStacks::traverse_as_directory(unsigned fsid, Function<bool(const FS::DirectoryEntryView&)> 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<bool(
 RefPtr<ProcFSExposedComponent> 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<ProcFSThreadStack> 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<ProcFSProcessFolder> m_process_folder;
+    WeakPtr<ProcFSProcessFolder> m_process_folder;
     mutable Lock m_lock;
 };
 
 KResultOr<size_t> 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<bool(const FS::DirectoryEntryView&)> 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<ProcFSExposedComponent> 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<ProcFSProcessFileDescription> 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()) {
+        auto parent_folder = m_parent_folder.strong_ref();
-            dbgln("ProcFS: No perf events for {}", m_parent_folder->m_associated_process->pid());
+        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<ProcFSProcessFolder> m_parent_process_directory;
+    WeakPtr<ProcFSProcessFolder> 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<ProcFSProcessFolder> m_parent_process_directory;
+    WeakPtr<ProcFSProcessFolder> 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<ProcFSProcessFolder> m_parent_process_directory;
+    WeakPtr<ProcFSProcessFolder> m_parent_process_directory;
 };
 
 NonnullRefPtr<ProcFSProcessFolder> ProcFSProcessFolder::create(const Process& process)

Kernel/Thread.cpp

@@ -459,8 +459,12 @@ void Thread::finalize_dying_threads()
         });
     }
     for (auto* thread : dying_threads) {
+        RefPtr<Process> 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!