From c7e747d775567838cfd0101f22f106df9d3b38df Mon Sep 17 00:00:00 2001 From: JosJuice Date: Sat, 3 Oct 2015 13:24:13 +0200 Subject: [PATCH] DiscIO: Improve IVolume::Read32 error handling Callers can now check whether reads fail, either by checking the return value or by setting the buffer to a known bad value and seeing if it stays untouched. I've added error checks to FileSystemGCWii and Boot_BS2Emu, but not to Boot since it doesn't check any of its other reads either. --- Source/Core/Core/Boot/Boot.cpp | 17 ++--- Source/Core/Core/Boot/Boot_BS2Emu.cpp | 28 ++++---- Source/Core/DiscIO/FileSystemGCWii.cpp | 92 ++++++++++++++++---------- Source/Core/DiscIO/FileSystemGCWii.h | 2 +- Source/Core/DiscIO/Filesystem.h | 2 +- Source/Core/DiscIO/Volume.h | 11 +-- 6 files changed, 90 insertions(+), 62 deletions(-) diff --git a/Source/Core/Core/Boot/Boot.cpp b/Source/Core/Core/Boot/Boot.cpp index aeda7d9aef..d7f84b3ec6 100644 --- a/Source/Core/Core/Boot/Boot.cpp +++ b/Source/Core/Core/Boot/Boot.cpp @@ -60,17 +60,18 @@ void CBoot::Load_FST(bool _bIsWii) if (_bIsWii) shift = 2; - u32 fstOffset = volume.Read32(0x0424, _bIsWii) << shift; - u32 fstSize = volume.Read32(0x0428, _bIsWii) << shift; - u32 maxFstSize = volume.Read32(0x042c, _bIsWii) << shift; + u32 fst_offset, fst_size, max_fst_size; + volume.ReadSwapped(0x0424, &fst_offset, _bIsWii); + volume.ReadSwapped(0x0428, &fst_size, _bIsWii); + volume.ReadSwapped(0x042c, &max_fst_size, _bIsWii); - u32 arenaHigh = ROUND_DOWN(0x817FFFFF - maxFstSize, 0x20); - Memory::Write_U32(arenaHigh, 0x00000034); + u32 arena_high = ROUND_DOWN(0x817FFFFF - (max_fst_size << shift), 0x20); + Memory::Write_U32(arena_high, 0x00000034); // load FST - DVDRead(fstOffset, arenaHigh, fstSize, _bIsWii); - Memory::Write_U32(arenaHigh, 0x00000038); - Memory::Write_U32(maxFstSize, 0x0000003c); + DVDRead(fst_offset << shift, arena_high, fst_size << shift, _bIsWii); + Memory::Write_U32(arena_high, 0x00000038); + Memory::Write_U32(max_fst_size << shift, 0x0000003c); } void CBoot::UpdateDebugger_MapLoaded() diff --git a/Source/Core/Core/Boot/Boot_BS2Emu.cpp b/Source/Core/Core/Boot/Boot_BS2Emu.cpp index c8f6f0d270..4d81c6db32 100644 --- a/Source/Core/Core/Boot/Boot_BS2Emu.cpp +++ b/Source/Core/Core/Boot/Boot_BS2Emu.cpp @@ -90,15 +90,18 @@ bool CBoot::EmulatedBS2_GC(bool skipAppLoader) // Load Apploader to Memory - The apploader is hardcoded to begin at 0x2440 on the disc, // but the size can differ between discs. Compare with YAGCD chap 13. const DiscIO::IVolume& volume = DVDInterface::GetVolume(); - u32 iAppLoaderOffset = 0x2440; - u32 iAppLoaderEntry = volume.Read32(iAppLoaderOffset + 0x10, false); - u32 iAppLoaderSize = volume.Read32(iAppLoaderOffset + 0x14, false) + volume.Read32(iAppLoaderOffset + 0x18, false); - if ((iAppLoaderEntry == (u32)-1) || (iAppLoaderSize == (u32)-1) || skipAppLoader) + const u32 apploader_offset = 0x2440; + u32 apploader_entry, apploader_size, apploader_trailer; + if (skipAppLoader || + !volume.ReadSwapped(apploader_offset + 0x10, &apploader_entry, false) || + !volume.ReadSwapped(apploader_offset + 0x14, &apploader_size, false) || + !volume.ReadSwapped(apploader_offset + 0x18, &apploader_trailer, false) || + apploader_entry == (u32)-1 || apploader_size + apploader_trailer == (u32)-1) { INFO_LOG(BOOT, "GC BS2: Not running apploader!"); return false; } - DVDRead(iAppLoaderOffset + 0x20, 0x01200000, iAppLoaderSize, false); + DVDRead(apploader_offset + 0x20, 0x01200000, apploader_size + apploader_trailer, false); // Setup pointers like real BS2 does if (SConfig::GetInstance().bNTSC) @@ -122,7 +125,7 @@ bool CBoot::EmulatedBS2_GC(bool skipAppLoader) PowerPC::ppcState.gpr[3] = iAppLoaderFuncAddr + 0; PowerPC::ppcState.gpr[4] = iAppLoaderFuncAddr + 4; PowerPC::ppcState.gpr[5] = iAppLoaderFuncAddr + 8; - RunFunction(iAppLoaderEntry); + RunFunction(apploader_entry); u32 iAppLoaderInit = PowerPC::Read_U32(iAppLoaderFuncAddr + 0); u32 iAppLoaderMain = PowerPC::Read_U32(iAppLoaderFuncAddr + 4); u32 iAppLoaderClose = PowerPC::Read_U32(iAppLoaderFuncAddr + 8); @@ -365,18 +368,19 @@ bool CBoot::EmulatedBS2_Wii() PowerPC::ppcState.gpr[1] = 0x816ffff0; // StackPointer - u32 iAppLoaderOffset = 0x2440; // 0x1c40; + const u32 apploader_offset = 0x2440; // 0x1c40; // Load Apploader to Memory const DiscIO::IVolume& volume = DVDInterface::GetVolume(); - u32 iAppLoaderEntry = volume.Read32(iAppLoaderOffset + 0x10, true); - u32 iAppLoaderSize = volume.Read32(iAppLoaderOffset + 0x14, true); - if ((iAppLoaderEntry == (u32)-1) || (iAppLoaderSize == (u32)-1)) + u32 apploader_entry, apploader_size; + if (!volume.ReadSwapped(apploader_offset + 0x10, &apploader_entry, true) || + !volume.ReadSwapped(apploader_offset + 0x14, &apploader_size, true) || + apploader_entry == (u32)-1 || apploader_size == (u32)-1) { ERROR_LOG(BOOT, "Invalid apploader. Probably your image is corrupted."); return false; } - DVDRead(iAppLoaderOffset + 0x20, 0x01200000, iAppLoaderSize, true); + DVDRead(apploader_offset + 0x20, 0x01200000, apploader_size, true); //call iAppLoaderEntry DEBUG_LOG(BOOT, "Call iAppLoaderEntry"); @@ -385,7 +389,7 @@ bool CBoot::EmulatedBS2_Wii() PowerPC::ppcState.gpr[3] = iAppLoaderFuncAddr + 0; PowerPC::ppcState.gpr[4] = iAppLoaderFuncAddr + 4; PowerPC::ppcState.gpr[5] = iAppLoaderFuncAddr + 8; - RunFunction(iAppLoaderEntry); + RunFunction(apploader_entry); u32 iAppLoaderInit = PowerPC::Read_U32(iAppLoaderFuncAddr + 0); u32 iAppLoaderMain = PowerPC::Read_U32(iAppLoaderFuncAddr + 4); u32 iAppLoaderClose = PowerPC::Read_U32(iAppLoaderFuncAddr + 8); diff --git a/Source/Core/DiscIO/FileSystemGCWii.cpp b/Source/Core/DiscIO/FileSystemGCWii.cpp index 02fb360bbe..f721173346 100644 --- a/Source/Core/DiscIO/FileSystemGCWii.cpp +++ b/Source/Core/DiscIO/FileSystemGCWii.cpp @@ -127,20 +127,24 @@ bool CFileSystemGCWii::ExportFile(const std::string& _rFullPath, const std::stri bool CFileSystemGCWii::ExportApploader(const std::string& _rExportFolder) const { - u32 AppSize = m_rVolume->Read32(0x2440 + 0x14, m_Wii); // apploader size - AppSize += m_rVolume->Read32(0x2440 + 0x18, m_Wii); // + trailer size - AppSize += 0x20; // + header size - DEBUG_LOG(DISCIO,"AppSize -> %x", AppSize); + u32 apploader_size; + u32 trailer_size; + const u32 header_size = 0x20; + if (!m_rVolume->ReadSwapped(0x2440 + 0x14, &apploader_size, m_Wii) || + !m_rVolume->ReadSwapped(0x2440 + 0x18, &trailer_size, m_Wii)) + return false; + apploader_size += trailer_size + header_size; + DEBUG_LOG(DISCIO, "Apploader size -> %x", apploader_size); - std::vector buffer(AppSize); - if (m_rVolume->Read(0x2440, AppSize, &buffer[0], m_Wii)) + std::vector buffer(apploader_size); + if (m_rVolume->Read(0x2440, apploader_size, buffer.data(), m_Wii)) { std::string exportName(_rExportFolder + "/apploader.img"); File::IOFile AppFile(exportName, "wb"); if (AppFile) { - AppFile.WriteBytes(&buffer[0], AppSize); + AppFile.WriteBytes(buffer.data(), apploader_size); return true; } } @@ -148,13 +152,20 @@ bool CFileSystemGCWii::ExportApploader(const std::string& _rExportFolder) const return false; } -u32 CFileSystemGCWii::GetBootDOLSize() const +u64 CFileSystemGCWii::GetBootDOLOffset() const { - return GetBootDOLSize((u64)m_rVolume->Read32(0x420, m_Wii) << GetOffsetShift()); + u32 offset = 0; + m_rVolume->ReadSwapped(0x420, &offset, m_Wii); + return static_cast(offset) << GetOffsetShift(); } u32 CFileSystemGCWii::GetBootDOLSize(u64 dol_offset) const { + // The dol_offset value is usually obtained by calling GetBootDOLOffset. + // If GetBootDOLOffset fails by returning 0, GetBootDOLSize should also fail. + if (dol_offset == 0) + return 0; + u32 dol_size = 0; u32 offset = 0; u32 size = 0; @@ -162,16 +173,18 @@ u32 CFileSystemGCWii::GetBootDOLSize(u64 dol_offset) const // Iterate through the 7 code segments for (u8 i = 0; i < 7; i++) { - offset = m_rVolume->Read32(dol_offset + 0x00 + i * 4, m_Wii); - size = m_rVolume->Read32(dol_offset + 0x90 + i * 4, m_Wii); + if (!m_rVolume->ReadSwapped(dol_offset + 0x00 + i * 4, &offset, m_Wii) || + !m_rVolume->ReadSwapped(dol_offset + 0x90 + i * 4, &size, m_Wii)) + return 0; dol_size = std::max(offset + size, dol_size); } // Iterate through the 11 data segments for (u8 i = 0; i < 11; i++) { - offset = m_rVolume->Read32(dol_offset + 0x1c + i * 4, m_Wii); - size = m_rVolume->Read32(dol_offset + 0xac + i * 4, m_Wii); + if (!m_rVolume->ReadSwapped(dol_offset + 0x1c + i * 4, &offset, m_Wii) || + !m_rVolume->ReadSwapped(dol_offset + 0xac + i * 4, &size, m_Wii)) + return 0; dol_size = std::max(offset + size, dol_size); } @@ -180,8 +193,11 @@ u32 CFileSystemGCWii::GetBootDOLSize(u64 dol_offset) const bool CFileSystemGCWii::ExportDOL(const std::string& _rExportFolder) const { - u32 DolOffset = m_rVolume->Read32(0x420, m_Wii) << GetOffsetShift(); - u32 DolSize = GetBootDOLSize(); + u64 DolOffset = GetBootDOLOffset(); + u32 DolSize = GetBootDOLSize(DolOffset); + + if (DolOffset == 0 || DolSize == 0) + return false; std::vector buffer(DolSize); if (m_rVolume->Read(DolOffset, DolSize, &buffer[0], m_Wii)) @@ -234,12 +250,13 @@ const SFileInfo* CFileSystemGCWii::FindFileInfo(const std::string& _rFullPath) bool CFileSystemGCWii::DetectFileSystem() { - if (m_rVolume->Read32(0x18, false) == 0x5D1C9EA3) + u32 magic_bytes; + if (m_rVolume->ReadSwapped(0x18, &magic_bytes, false) && magic_bytes == 0x5D1C9EA3) { m_Wii = true; return true; } - else if (m_rVolume->Read32(0x1c, false) == 0xC2339F3D) + else if (m_rVolume->ReadSwapped(0x1c, &magic_bytes, false) && magic_bytes == 0xC2339F3D) { m_Wii = false; return true; @@ -254,26 +271,26 @@ void CFileSystemGCWii::InitFileSystem() u32 const shift = GetOffsetShift(); // read the whole FST - u64 FSTOffset = static_cast(m_rVolume->Read32(0x424, m_Wii)) << shift; - // u32 FSTSize = Read32(0x428); - // u32 FSTMaxSize = Read32(0x42C); - + u32 fst_offset_unshifted; + if (!m_rVolume->ReadSwapped(0x424, &fst_offset_unshifted, m_Wii)) + return; + u64 FSTOffset = static_cast(fst_offset_unshifted) << shift; // read all fileinfos - SFileInfo Root - { - m_rVolume->Read32(FSTOffset + 0x0, m_Wii), - static_cast(FSTOffset + 0x4) << shift, - m_rVolume->Read32(FSTOffset + 0x8, m_Wii) - }; + u32 name_offset, offset, size; + if (!m_rVolume->ReadSwapped(FSTOffset + 0x0, &name_offset, m_Wii) || + !m_rVolume->ReadSwapped(FSTOffset + 0x4, &offset, m_Wii) || + !m_rVolume->ReadSwapped(FSTOffset + 0x8, &size, m_Wii)) + return; + SFileInfo root = { name_offset, static_cast(offset) << shift, size }; - if (!Root.IsDirectory()) + if (!root.IsDirectory()) return; // 12 bytes (the size of a file entry) times 10 * 1024 * 1024 is 120 MiB, // more than total RAM in a Wii. No file system should use anywhere near that much. static const u32 ARBITRARY_FILE_SYSTEM_SIZE_LIMIT = 10 * 1024 * 1024; - if (Root.m_FileSize > ARBITRARY_FILE_SYSTEM_SIZE_LIMIT) + if (root.m_FileSize > ARBITRARY_FILE_SYSTEM_SIZE_LIMIT) { // Without this check, Dolphin can crash by trying to allocate too much // memory when loading the file systems of certain malformed disc images. @@ -286,14 +303,17 @@ void CFileSystemGCWii::InitFileSystem() PanicAlert("Wtf?"); u64 NameTableOffset = FSTOffset; - m_FileInfoVector.reserve((size_t)Root.m_FileSize); - for (u32 i = 0; i < Root.m_FileSize; i++) + m_FileInfoVector.reserve((size_t)root.m_FileSize); + for (u32 i = 0; i < root.m_FileSize; i++) { - u64 const read_offset = FSTOffset + (i * 0xC); - u64 const name_offset = m_rVolume->Read32(read_offset + 0x0, m_Wii); - u64 const offset = static_cast(m_rVolume->Read32(read_offset + 0x4, m_Wii)) << shift; - u64 const size = m_rVolume->Read32(read_offset + 0x8, m_Wii); - m_FileInfoVector.emplace_back(name_offset, offset, size); + const u64 read_offset = FSTOffset + (i * 0xC); + name_offset = 0; + m_rVolume->ReadSwapped(read_offset + 0x0, &name_offset, m_Wii); + offset = 0; + m_rVolume->ReadSwapped(read_offset + 0x4, &offset, m_Wii); + size = 0; + m_rVolume->ReadSwapped(read_offset + 0x8, &size, m_Wii); + m_FileInfoVector.emplace_back(name_offset, static_cast(offset) << shift, size); NameTableOffset += 0xC; } diff --git a/Source/Core/DiscIO/FileSystemGCWii.h b/Source/Core/DiscIO/FileSystemGCWii.h index 3b70e63437..f9bfa5f5b1 100644 --- a/Source/Core/DiscIO/FileSystemGCWii.h +++ b/Source/Core/DiscIO/FileSystemGCWii.h @@ -30,7 +30,7 @@ public: bool ExportFile(const std::string& _rFullPath, const std::string&_rExportFilename) override; bool ExportApploader(const std::string& _rExportFolder) const override; bool ExportDOL(const std::string& _rExportFolder) const override; - u32 GetBootDOLSize() const override; + u64 GetBootDOLOffset() const override; u32 GetBootDOLSize(u64 dol_offset) const override; private: diff --git a/Source/Core/DiscIO/Filesystem.h b/Source/Core/DiscIO/Filesystem.h index 9ee60ab6d5..36fb9916a8 100644 --- a/Source/Core/DiscIO/Filesystem.h +++ b/Source/Core/DiscIO/Filesystem.h @@ -49,7 +49,7 @@ public: virtual bool ExportApploader(const std::string& _rExportFolder) const = 0; virtual bool ExportDOL(const std::string& _rExportFolder) const = 0; virtual const std::string GetFileName(u64 _Address) = 0; - virtual u32 GetBootDOLSize() const = 0; + virtual u64 GetBootDOLOffset() const = 0; virtual u32 GetBootDOLSize(u64 dol_offset) const = 0; protected: diff --git a/Source/Core/DiscIO/Volume.h b/Source/Core/DiscIO/Volume.h index a09c282748..9003770e35 100644 --- a/Source/Core/DiscIO/Volume.h +++ b/Source/Core/DiscIO/Volume.h @@ -71,11 +71,14 @@ public: // decrypt parameter must be false if not reading a Wii disc virtual bool Read(u64 _Offset, u64 _Length, u8* _pBuffer, bool decrypt) const = 0; - virtual u32 Read32(u64 _Offset, bool decrypt) const + template + bool ReadSwapped(u64 offset, T* buffer, bool decrypt) const { - u32 temp; - Read(_Offset, sizeof(u32), (u8*)&temp, decrypt); - return Common::swap32(temp); + T temp; + if (!Read(offset, sizeof(T), reinterpret_cast(&temp), decrypt)) + return false; + *buffer = Common::FromBigEndian(temp); + return true; } virtual bool GetTitleID(u64*) const { return false; }