Merge pull request #20466 from hrydgard/iso-error-check

ISO loading: Check CSO and CHD files "early"

Core/FileSystems/BlockDevices.cpp

@@ -24,6 +24,7 @@
 #include "Common/Swap.h"
 #include "Common/File/FileUtil.h"
 #include "Common/File/DirListing.h"
+#include "Common/StringUtils.h"
 #include "Core/Loaders.h"
 #include "Core/FileSystems/BlockDevices.h"
 #include "libchdr/chd.h"
@@ -35,12 +36,15 @@ extern "C"
 #include "ext/libkirk/kirk_engine.h"
 };
 
-BlockDevice *constructBlockDevice(FileLoader *fileLoader) {
+BlockDevice *ConstructBlockDevice(FileLoader *fileLoader, std::string *errorString) {
 	if (!fileLoader->Exists()) {
+		// Shouldn't get here really.
+		*errorString = "File doesn't exist";
 		return nullptr;
 	}
 	if (fileLoader->IsDirectory()) {
-		ERROR_LOG(Log::Loader, "Can't open directory directly as block device: %s", fileLoader->GetPath().c_str());
+		*errorString = "Can't open directory directly as block device: ";
+		*errorString += fileLoader->GetPath().ToString();
 		return nullptr;
 	}
 
@@ -48,23 +52,36 @@ BlockDevice *constructBlockDevice(FileLoader *fileLoader) {
 	size_t size = fileLoader->ReadAt(0, 1, 8, buffer);
 	if (size != 8) {
 		// Bad or empty file
+		*errorString = "File is empty";
 		return nullptr;
 	}
 
+	BlockDevice *device = nullptr;
+
 	// Check for CISO
 	if (!memcmp(buffer, "CISO", 4)) {
-		return new CISOFileBlockDevice(fileLoader);
+		device = new CISOFileBlockDevice(fileLoader);
 	} else if (!memcmp(buffer, "\x00PBP", 4)) {
 		uint32_t psarOffset = 0;
 		size = fileLoader->ReadAt(0x24, 1, 4, &psarOffset);
 		if (size == 4 && psarOffset < fileLoader->FileSize())
-			return new NPDRMDemoBlockDevice(fileLoader);
+			device = new NPDRMDemoBlockDevice(fileLoader);
 	} else if (!memcmp(buffer, "MComprHD", 8)) {
-		return new CHDFileBlockDevice(fileLoader);
+		device = new CHDFileBlockDevice(fileLoader);
 	}
 
-	// Should be just a regular ISO file. Let's open it as a plain block device and let the other systems take over.
-	return new FileBlockDevice(fileLoader);
+	// No check above passed, should be just a regular ISO file. Let's open it as a plain block device and let the other systems take over.
+	if (!device) {
+		device = new FileBlockDevice(fileLoader);
+	}
+
+	if (!device->IsOK()) {
+		*errorString = device->ErrorString();
+		delete device;
+		return nullptr;
+	}
+
+	return device;
 }
 
 void BlockDevice::NotifyReadError() {
@@ -80,8 +97,7 @@ FileBlockDevice::FileBlockDevice(FileLoader *fileLoader)
 	filesize_ = fileLoader->FileSize();
 }
 
-FileBlockDevice::~FileBlockDevice() {
-}
+FileBlockDevice::~FileBlockDevice() {}
 
 bool FileBlockDevice::ReadBlock(int blockNumber, u8 *outPtr, bool uncached) {
 	FileLoader::Flags flags = uncached ? FileLoader::Flags::HINT_UNCACHED : FileLoader::Flags::NONE;
@@ -90,7 +106,6 @@ bool FileBlockDevice::ReadBlock(int blockNumber, u8 *outPtr, bool uncached) {
 		DEBUG_LOG(Log::FileSystem, "Could not read 2048 byte block, at block offset %d. Only got %d bytes", blockNumber, (int)retval);
 		return false;
 	}
-
 	return true;
 }
 
@@ -141,17 +156,22 @@ CISOFileBlockDevice::CISOFileBlockDevice(FileLoader *fileLoader)
 	CISO_H hdr;
 	size_t readSize = fileLoader->ReadAt(0, sizeof(CISO_H), 1, &hdr);
 	if (readSize != 1 || memcmp(hdr.magic, "CISO", 4) != 0) {
-		WARN_LOG(Log::Loader, "Invalid CSO!");
+		errorString_ = "Invalid CSO!";
+		return;
 	}
 	if (hdr.ver > 1) {
-		WARN_LOG(Log::Loader, "CSO version too high!");
+		errorString_ = "CSO version too high!";
+		return;
 	}
 
 	frameSize = hdr.block_size;
-	if ((frameSize & (frameSize - 1)) != 0)
-		ERROR_LOG(Log::Loader, "CSO block size %i unsupported, must be a power of two", frameSize);
-	else if (frameSize < 0x800)
-		ERROR_LOG(Log::Loader, "CSO block size %i unsupported, must be at least one sector", frameSize);
+	if ((frameSize & (frameSize - 1)) != 0) {
+		errorString_ = StringFromFormat("CSO block size %i unsupported, must be a power of two", frameSize);
+		return;
+	} else if (frameSize < 0x800) {
+		errorString_ = StringFromFormat("CSO block size %i unsupported, must be at least one sector", frameSize);
+		return;
+	}
 
 	// Determine the translation from block to frame.
 	blockShift = 0;
@@ -203,10 +223,12 @@ CISOFileBlockDevice::CISOFileBlockDevice(FileLoader *fileLoader)
 	u64 lastIndexPos = index[indexSize - 1] & 0x7FFFFFFF;
 	u64 expectedFileSize = lastIndexPos << indexShift;
 	if (expectedFileSize > fileSize) {
-		ERROR_LOG(Log::Loader, "Expected CSO to at least be %lld bytes, but file is %lld bytes. File: '%s'",
-			expectedFileSize, fileSize, fileLoader->GetPath().c_str());
-		NotifyReadError();
+		errorString_ = StringFromFormat("Expected CSO to at least be %lld bytes, but file is %lld bytes", expectedFileSize, fileSize);
+		return;
 	}
+
+	// all ok.
+	_dbg_assert_(errorString_.empty());
 }
 
 CISOFileBlockDevice::~CISOFileBlockDevice()
@@ -389,7 +411,8 @@ NPDRMDemoBlockDevice::NPDRMDemoBlockDevice(FileLoader *fileLoader)
 	fileLoader_->ReadAt(0x24, 1, 4, &psarOffset);
 	size_t readSize = fileLoader_->ReadAt(psarOffset, 1, 256, &np_header);
 	if (readSize != 256){
-		ERROR_LOG(Log::Loader, "Invalid NPUMDIMG header!");
+		errorString_ = "Invalid NPUMDIMG header!";
+		return;
 	}
 
 	u32 psar_id;
@@ -400,6 +423,7 @@ NPDRMDemoBlockDevice::NPDRMDemoBlockDevice(FileLoader *fileLoader)
 	if (psar_id == 'SISP') {
 		lbaSize_ = 0;  // Mark invalid
 		ERROR_LOG(Log::Loader, "PSX not supported! Should have been caught earlier.");
+		errorString_ = "PSX ISOs not supported!";
 		return;
 	}
 
@@ -432,8 +456,7 @@ NPDRMDemoBlockDevice::NPDRMDemoBlockDevice(FileLoader *fileLoader)
 
 	// When we remove the above assert, let's just try to survive.
 	if (blockLBAs_ > 4096) {
-		ERROR_LOG(Log::Loader, "Bad blockLBAs in header: %08x (%s) psar: %s", blockLBAs_, fileLoader->GetPath().ToVisualString().c_str(), psarStr);
-		// We'll end up displaying an error message since ReadBlock will fail.
+		errorString_ = StringFromFormat("Bad blockLBAs in header: %08x (%s) psar: %s", blockLBAs_, fileLoader->GetPath().ToVisualString().c_str(), psarStr);
 		return;
 	}
 
@@ -450,7 +473,8 @@ NPDRMDemoBlockDevice::NPDRMDemoBlockDevice(FileLoader *fileLoader)
 
 	readSize = fileLoader_->ReadAt(psarOffset + tableOffset_, 1, tableSize_, table_);
 	if (readSize != tableSize_){
-		ERROR_LOG(Log::Loader, "Invalid NPUMDIMG table!");
+		errorString_ = "Invalid NPUMDIMG table!";
+		return;
 	}
 
 	u32 *p = (u32*)table_;
@@ -468,6 +492,7 @@ NPDRMDemoBlockDevice::NPDRMDemoBlockDevice(FileLoader *fileLoader)
 	}
 
 	currentBlock_ = -1;
+	_dbg_assert_(errorString_.empty());
 }
 
 NPDRMDemoBlockDevice::~NPDRMDemoBlockDevice() {
@@ -654,8 +679,7 @@ CHDFileBlockDevice::CHDFileBlockDevice(FileLoader *fileLoader)
 	chd_file *file = nullptr;
 	chd_error err = chd_open_core_file(&core_file_->core, CHD_OPEN_READ, NULL, &file);
 	if (err != CHDERR_NONE) {
-		ERROR_LOG(Log::Loader, "Error loading CHD '%s': %s", paths[depth].c_str(), chd_error_string(err));
-		NotifyReadError();
+		errorString_ = StringFromFormat("CHD error: %s", paths[depth].c_str(), chd_error_string(err));
 		return;
 	}
 
@@ -666,6 +690,8 @@ CHDFileBlockDevice::CHDFileBlockDevice(FileLoader *fileLoader)
 	currentHunk = -1;
 	blocksPerHunk = impl_->header->hunkbytes / impl_->header->unitbytes;
 	numBlocks = impl_->header->unitcount;
+
+	_dbg_assert_(errorString_.empty());
 }
 
 CHDFileBlockDevice::~CHDFileBlockDevice() {

Core/FileSystems/BlockDevices.h

@@ -55,9 +55,14 @@ public:
 
 	void NotifyReadError();
 
+	bool IsOK() const { return errorString_.empty(); }
+	const std::string &ErrorString() { return errorString_; }
+
 protected:
 	FileLoader *fileLoader_;
 	bool reportedError_ = false;
+
+	std::string errorString_;
 };
 
 class CISOFileBlockDevice : public BlockDevice {
@@ -163,4 +168,4 @@ private:
 	u32 numBlocks = 0;
 };
 
-BlockDevice *constructBlockDevice(FileLoader *fileLoader);
+BlockDevice *ConstructBlockDevice(FileLoader *fileLoader, std::string *errorString);

Core/Loaders.cpp

@@ -305,8 +305,8 @@ bool UmdReplace(const Path &filepath, FileLoader **fileLoader, std::string &erro
 	case IdentifiedFileType::PSP_ISO:
 	case IdentifiedFileType::PSP_ISO_NP:
 	case IdentifiedFileType::PSP_DISC_DIRECTORY:
-		if (!MountGameISO(loadedFile)) {
-			error = "mounting the new ISO failed";
+		if (!MountGameISO(loadedFile, &error)) {
+			error = "mounting the replaced ISO failed: " + error;
 			return false;
 		}
 		break;

Core/PSPLoaders.cpp

@@ -60,7 +60,7 @@ static void UseLargeMem(int memsize) {
 	}
 }
 
-bool MountGameISO(FileLoader *fileLoader) {
+bool MountGameISO(FileLoader *fileLoader, std::string *errorString) {
 	std::shared_ptr<IFileSystem> fileSystem;
 	std::shared_ptr<IFileSystem> blockSystem;
 
@@ -68,7 +68,7 @@ bool MountGameISO(FileLoader *fileLoader) {
 		fileSystem = std::make_shared<VirtualDiscFileSystem>(&pspFileSystem, fileLoader->GetPath());
 		blockSystem = fileSystem;
 	} else {
-		auto bd = constructBlockDevice(fileLoader);
+		auto bd = ConstructBlockDevice(fileLoader, errorString);
 		if (!bd) {
 			// Can only fail if the ISO is bad.
 			return false;
@@ -297,14 +297,16 @@ static Path NormalizePath(const Path &path) {
 bool Load_PSP_ELF_PBP(FileLoader *fileLoader, std::string *error_string) {
 	// This is really just for headless, might need tweaking later.
 	if (PSP_CoreParameter().mountIsoLoader != nullptr) {
-		auto bd = constructBlockDevice(PSP_CoreParameter().mountIsoLoader);
-		if (bd != NULL) {
+		auto bd = ConstructBlockDevice(PSP_CoreParameter().mountIsoLoader, error_string);
+		if (bd) {
 			auto umd2 = std::make_shared<ISOFileSystem>(&pspFileSystem, bd);
 			auto blockSystem = std::make_shared<ISOBlockSystem>(umd2);
 
 			pspFileSystem.Mount("umd1:", blockSystem);
 			pspFileSystem.Mount("disc0:", umd2);
 			pspFileSystem.Mount("umd:", blockSystem);
+		} else {
+			ERROR_LOG(Log::Loader, "mountIso failed: %s", error_string->c_str());
 		}
 	}
 

Core/PSPLoaders.h

@@ -25,9 +25,7 @@ bool Load_PSP_ISO(FileLoader *fileLoader, std::string *error_string);
 bool Load_PSP_ELF_PBP(FileLoader *fileLoader, std::string *error_string);
 bool Load_PSP_GE_Dump(FileLoader *fileLoader, std::string *error_string);
 
-bool MountGameISO(FileLoader *fileLoader);
+bool MountGameISO(FileLoader *fileLoader, std::string *errorString);
 bool LoadParamSFOFromDisc();
 bool LoadParamSFOFromPBP(FileLoader *fileLoader);
 void InitMemorySizeForGame();
-
-void PSPLoaders_Shutdown();

Core/Reporting.cpp

@@ -135,11 +135,15 @@ namespace Reporting
 		AndroidJNIThreadContext jniContext;
 
 		FileLoader *fileLoader = ResolveFileLoaderTarget(ConstructFileLoader(crcFilename));
-		BlockDevice *blockDevice = constructBlockDevice(fileLoader);
+
+		std::string errorString;
+		BlockDevice *blockDevice = ConstructBlockDevice(fileLoader, &errorString);
 
 		u32 crc = 0;
 		if (blockDevice) {
 			crc = CalculateCRC(blockDevice, &crcCancel);
+		} else {
+			ERROR_LOG(Log::Loader, "Failed to read from block device for CRC: %s", errorString.c_str());
 		}
 
 		delete blockDevice;

Core/RetroAchievements.cpp

@@ -1037,9 +1037,10 @@ void SetGame(const Path &path, IdentifiedFileType fileType, FileLoader *fileLoad
 		//
 		// TODO: Fish the block device out of the loading process somewhere else. Though, probably easier to just do it here,
 		// we need a temporary blockdevice anyway since it gets consumed by ComputePSPISOHash.
-		BlockDevice *blockDevice(constructBlockDevice(fileLoader));
+		std::string errorString;
+		BlockDevice *blockDevice(ConstructBlockDevice(fileLoader, &errorString));
 		if (!blockDevice) {
-			ERROR_LOG(Log::Achievements, "Failed to construct block device for '%s' - can't identify", path.c_str());
+			ERROR_LOG(Log::Achievements, "Failed to construct block device for '%s' - can't identify: %s", path.c_str(), errorString.c_str());
 			g_isIdentifying = false;
 			return;
 		}
@@ -1100,9 +1101,10 @@ void ChangeUMD(const Path &path, FileLoader *fileLoader) {
 		return;
 	}
 
-	BlockDevice *blockDevice = constructBlockDevice(fileLoader);
+	std::string errorString;
+	BlockDevice *blockDevice = ConstructBlockDevice(fileLoader, &errorString);
 	if (!blockDevice) {
-		ERROR_LOG(Log::Achievements, "Failed to construct block device for '%s' - can't identify", path.c_str());
+		ERROR_LOG(Log::Achievements, "Failed to construct block device for '%s' - can't identify: %s", path.c_str(), errorString.c_str());
 		return;
 	}
 

Core/System.cpp

@@ -282,8 +282,8 @@ static bool CPU_Init(FileLoader *fileLoader, IdentifiedFileType type, std::strin
 	case IdentifiedFileType::PSP_ISO_NP:
 	case IdentifiedFileType::PSP_DISC_DIRECTORY:
 		// Doesn't seem to take ownership of fileLoader?
-		if (!MountGameISO(fileLoader)) {
-			*errorString = "Failed to mount ISO file - invalid format?";
+		if (!MountGameISO(fileLoader, errorString)) {
+			*errorString = "Failed to mount ISO file: " + *errorString;
 			return false;
 		}
 		if (LoadParamSFOFromDisc()) {

Core/Util/GameManager.cpp

@@ -421,7 +421,8 @@ std::string GameManager::GetPBPGameID(FileLoader *loader) const {
 
 std::string GameManager::GetISOGameID(FileLoader *loader) const {
 	SequentialHandleAllocator handles;
-	BlockDevice *bd = constructBlockDevice(loader);
+	std::string errorString;
+	BlockDevice *bd = ConstructBlockDevice(loader, &errorString);
 	if (!bd) {
 		return "";
 	}

UI/GameInfoCache.cpp

@@ -198,7 +198,8 @@ u64 GameInfo::GetSizeUncompressedInBytes() {
 		return File::ComputeRecursiveDirectorySize(GetFileLoader()->GetPath());
 	default:
 	{
-		BlockDevice *blockDevice = constructBlockDevice(GetFileLoader().get());
+		std::string errorString;
+		BlockDevice *blockDevice = ConstructBlockDevice(GetFileLoader().get(), &errorString);
 		if (blockDevice) {
 			u64 size = blockDevice->GetUncompressedSize();
 			delete blockDevice;
@@ -762,9 +763,9 @@ handleELF:
 					info_->MarkReadyNoLock(flags_);
 					return;
 				}
-				BlockDevice *bd = constructBlockDevice(info_->GetFileLoader().get());
+				BlockDevice *bd = ConstructBlockDevice(info_->GetFileLoader().get(), &errorString);
 				if (!bd) {
-					ERROR_LOG(Log::Loader, "Failed constructing block device for ISO %s", info_->GetFilePath().ToVisualString().c_str());
+					ERROR_LOG(Log::Loader, "Failed constructing block device for ISO %s: %s", info_->GetFilePath().ToVisualString().c_str(), errorString.c_str());
 					std::unique_lock<std::mutex> lock(info_->lock);
 					info_->MarkReadyNoLock(flags_);
 					return;