From ab7023dbe551d5fdfbc458b2a40cd11ccd4dcab9 Mon Sep 17 00:00:00 2001
From: Sam Atkins <atkinssj@gmail.com>
Date: Sat, 19 Jun 2021 14:12:11 +0100
Subject: [PATCH] Kernel: Ensure Ext2FSInode's lookup is populated before using
 it

This fixes #8133.

Ext2FSInode::remove_child() searches the lookup cache, so if it's not
initialized, removing the child fails. If the child was a directory,
this led to it being corrupted and having 0 children.

I also added populate_lookup_cache to add_child. I hadn't seen any
bugs there, but if the cache wasn't populated before, adding that
one entry would make it think it was populated, so that would cause
bugs later.
---
 Kernel/FileSystem/Ext2FileSystem.cpp | 22 ++++++++++++++--------
 Kernel/FileSystem/Ext2FileSystem.h   |  2 +-
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/Kernel/FileSystem/Ext2FileSystem.cpp b/Kernel/FileSystem/Ext2FileSystem.cpp
index c04f4940ab..888757974d 100644
--- a/Kernel/FileSystem/Ext2FileSystem.cpp
+++ b/Kernel/FileSystem/Ext2FileSystem.cpp
@@ -1209,6 +1209,9 @@ KResult Ext2FSInode::add_child(Inode& child, const StringView& name, mode_t mode
     if (result.is_error())
         return result;
 
+    if (auto populate_result = populate_lookup_cache(); populate_result.is_error())
+        return populate_result;
+
     m_lookup_cache.set(name, child.index());
     did_add_child(child.identifier(), name);
     return KSuccess;
@@ -1220,6 +1223,9 @@ KResult Ext2FSInode::remove_child(const StringView& name)
     dbgln_if(EXT2_DEBUG, "Ext2FSInode[{}]::remove_child(): Removing '{}'", identifier(), name);
     VERIFY(is_directory());
 
+    if (auto populate_result = populate_lookup_cache(); populate_result.is_error())
+        return populate_result;
+
     auto it = m_lookup_cache.find(name);
     if (it == m_lookup_cache.end())
         return ENOENT;
@@ -1591,11 +1597,11 @@ KResultOr<NonnullRefPtr<Inode>> Ext2FS::create_inode(Ext2FSInode& parent_inode,
     return new_inode.release_nonnull();
 }
 
-bool Ext2FSInode::populate_lookup_cache() const
+KResult Ext2FSInode::populate_lookup_cache() const
 {
     Locker locker(m_lock);
     if (!m_lookup_cache.is_empty())
-        return true;
+        return KSuccess;
     HashMap<String, InodeIndex> children;
 
     KResult result = traverse_as_directory([&children](auto& entry) {
@@ -1604,19 +1610,18 @@ bool Ext2FSInode::populate_lookup_cache() const
     });
 
     if (!result.is_success())
-        return false;
+        return result;
 
-    if (!m_lookup_cache.is_empty())
-        return false;
+    VERIFY(m_lookup_cache.is_empty());
     m_lookup_cache = move(children);
-    return true;
+    return KSuccess;
 }
 
 RefPtr<Inode> Ext2FSInode::lookup(StringView name)
 {
     VERIFY(is_directory());
     dbgln_if(EXT2_DEBUG, "Ext2FSInode[{}]:lookup(): Looking up '{}'", identifier(), name);
-    if (!populate_lookup_cache())
+    if (populate_lookup_cache().is_error())
         return {};
     Locker locker(m_lock);
     auto it = m_lookup_cache.find(name.hash(), [&](auto& entry) { return entry.key == name; });
@@ -1702,7 +1707,8 @@ KResultOr<size_t> Ext2FSInode::directory_entry_count() const
 {
     VERIFY(is_directory());
     Locker locker(m_lock);
-    populate_lookup_cache();
+    if (auto result = populate_lookup_cache(); result.is_error())
+        return KResultOr<size_t>(result);
     return m_lookup_cache.size();
 }
 
diff --git a/Kernel/FileSystem/Ext2FileSystem.h b/Kernel/FileSystem/Ext2FileSystem.h
index 738b506e30..da4584d8de 100644
--- a/Kernel/FileSystem/Ext2FileSystem.h
+++ b/Kernel/FileSystem/Ext2FileSystem.h
@@ -59,7 +59,7 @@ private:
     virtual KResultOr<int> get_block_address(int) override;
 
     KResult write_directory(Vector<Ext2FSDirectoryEntry>&);
-    bool populate_lookup_cache() const;
+    KResult populate_lookup_cache() const;
     KResult resize(u64);
     KResult write_indirect_block(BlockBasedFS::BlockIndex, Span<BlockBasedFS::BlockIndex>);
     KResult grow_doubly_indirect_block(BlockBasedFS::BlockIndex, size_t, Span<BlockBasedFS::BlockIndex>, Vector<BlockBasedFS::BlockIndex>&, unsigned&);