Filesystem: Do more validity checking in the constructor

This makes it possible to catch errors earlier so that file systems
simply fail to load instead of behaving oddly. It also makes it possible
to check for errors that weren't checkable before, like the end of a
directory being after the end of the parent directory.

Source/Core/DiscIO/FileSystemGCWii.cpp

@@ -88,18 +88,7 @@ u32 FileInfoGCWii::Get(EntryProperty entry_property) const
 
 u32 FileInfoGCWii::GetSize() const
 {
-  u32 result = Get(EntryProperty::FILE_SIZE);
-
-  if (IsDirectory() && result <= m_index)
-  {
-    // For directories, GetSize is supposed to return the index of the next entry.
-    // If a file system is malformed and instead has an index that isn't after this one,
-    // we act as if the directory is empty to avoid strange behavior.
-    ERROR_LOG(DISCIO, "Invalid folder end in file system");
-    return m_index + 1;
-  }
-
-  return result;
+  return Get(EntryProperty::FILE_SIZE);
 }
 
 u64 FileInfoGCWii::GetOffset() const
@@ -114,16 +103,20 @@ bool FileInfoGCWii::IsDirectory() const
 
 u32 FileInfoGCWii::GetTotalChildren() const
 {
-  return GetSize() - (m_index + 1);
+  return Get(EntryProperty::FILE_SIZE) - (m_index + 1);
+}
+
+u64 FileInfoGCWii::GetNameOffset() const
+{
+  return static_cast<u64>(FST_ENTRY_SIZE) * m_total_file_infos +
+         (Get(EntryProperty::NAME_OFFSET) & 0xFFFFFF);
 }
 
 std::string FileInfoGCWii::GetName() const
 {
   // TODO: Should we really always use SHIFT-JIS?
   // Some names in Pikmin (NTSC-U) don't make sense without it, but is it correct?
-  u32 name_offset = Get(EntryProperty::NAME_OFFSET) & 0xFFFFFF;
-  const u8* name = m_fst + FST_ENTRY_SIZE * m_total_file_infos + name_offset;
-  return SHIFTJISToUTF8(reinterpret_cast<const char*>(name));
+  return SHIFTJISToUTF8(reinterpret_cast<const char*>(m_fst + GetNameOffset()));
 }
 
 std::string FileInfoGCWii::GetPath() const
@@ -135,37 +128,63 @@ std::string FileInfoGCWii::GetPath() const
   if (IsDirectory())
   {
     u32 parent_directory_index = Get(EntryProperty::FILE_OFFSET);
-    if (parent_directory_index >= m_index)
-    {
-      // The index of the parent directory is supposed to be smaller than
-      // the current index. If an FST is malformed and breaks that rule,
-      // there's a risk that parent directory pointers form a loop.
-      // To avoid stack overflows, this method returns.
-      ERROR_LOG(DISCIO, "Invalid parent offset in file system");
-      return "";
-    }
     return FileInfoGCWii(*this, parent_directory_index).GetPath() + GetName() + "/";
   }
   else
   {
     // The parent directory can be found by searching backwards
-    // for a directory that contains this file.
-
+    // for a directory that contains this file. The search cannot fail,
+    // because the root directory at index 0 contains all files.
     FileInfoGCWii potential_parent(*this, m_index - 1);
-    while (!(potential_parent.IsDirectory() && potential_parent.GetSize() > m_index))
+    while (!(potential_parent.IsDirectory() &&
+             potential_parent.Get(EntryProperty::FILE_SIZE) > m_index))
     {
-      if (potential_parent.m_index == 0)
-      {
-        // This can happen if an FST has a root with a size that's too small
-        ERROR_LOG(DISCIO, "The parent of %s couldn't be found", GetName().c_str());
-        return "";
-      }
       potential_parent = FileInfoGCWii(*this, potential_parent.m_index - 1);
     }
     return potential_parent.GetPath() + GetName();
   }
 }
 
+bool FileInfoGCWii::IsValid(u64 fst_size, const FileInfoGCWii& parent_directory) const
+{
+  if (GetNameOffset() >= fst_size)
+  {
+    ERROR_LOG(DISCIO, "Impossibly large name offset in file system");
+    return false;
+  }
+
+  if (IsDirectory())
+  {
+    if (Get(EntryProperty::FILE_OFFSET) != parent_directory.m_index)
+    {
+      ERROR_LOG(DISCIO, "Incorrect parent offset in file system");
+      return false;
+    }
+
+    u32 size = Get(EntryProperty::FILE_SIZE);
+
+    if (size <= m_index)
+    {
+      ERROR_LOG(DISCIO, "Impossibly small directory size in file system");
+      return false;
+    }
+
+    if (size > parent_directory.Get(EntryProperty::FILE_SIZE))
+    {
+      ERROR_LOG(DISCIO, "Impossibly large directory size in file system");
+      return false;
+    }
+
+    for (const FileInfo& child : *this)
+    {
+      if (!static_cast<const FileInfoGCWii&>(child).IsValid(fst_size, *this))
+        return false;
+    }
+  }
+
+  return true;
+}
+
 FileSystemGCWii::FileSystemGCWii(const Volume* _rVolume, const Partition& partition)
     : FileSystem(_rVolume, partition), m_Valid(false), m_offset_shift(0), m_root(nullptr, 0, 0, 0)
 {
@@ -217,8 +236,20 @@ FileSystemGCWii::FileSystemGCWii(const Volume* _rVolume, const Partition& partit
     return;
   }
 
-  // If we haven't returned yet, everything succeeded
-  m_Valid = true;
+  if (FST_ENTRY_SIZE * m_root.GetSize() > fst_size)
+  {
+    ERROR_LOG(DISCIO, "File system has too many entries for its size");
+    return;
+  }
+
+  // If the FST's final byte isn't 0, CFileInfoGCWii::GetName() can read past the end
+  if (m_file_system_table[fst_size - 1] != 0)
+  {
+    ERROR_LOG(DISCIO, "File system does not end with a null byte");
+    return;
+  }
+
+  m_Valid = m_root.IsValid(fst_size, m_root);
 }
 
 FileSystemGCWii::~FileSystemGCWii()

Source/Core/DiscIO/FileSystemGCWii.h

@@ -46,6 +46,8 @@ public:
   std::string GetName() const override;
   std::string GetPath() const override;
 
+  bool IsValid(u64 fst_size, const FileInfoGCWii& parent_directory) const;
+
 protected:
   uintptr_t GetAddress() const override;
   FileInfo& operator++() override;
@@ -71,6 +73,8 @@ private:
   // Returns one of the three properties of this FST entry.
   // Read the comments in EntryProperty for details.
   u32 Get(EntryProperty entry_property) const;
+  // Returns the name offset, excluding the directory identification byte
+  u64 GetNameOffset() const;
 
   const u8* m_fst;
   u8 m_offset_shift;