Skip to content

Commit 7d67d73

Browse files
hrydgardhrydgard
authored
Merge pull request #20466 from hrydgard/iso-error-check
ISO loading: Check CSO and CHD files "early"
2 parents 409382c + 64680a4 commit 7d67d73

10 files changed

Lines changed: 85 additions & 46 deletions

File tree

‎Core/FileSystems/BlockDevices.cpp‎

Lines changed: 51 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include "Common/Swap.h"
2525
#include "Common/File/FileUtil.h"
2626
#include "Common/File/DirListing.h"
27+
#include "Common/StringUtils.h"
2728
#include "Core/Loaders.h"
2829
#include "Core/FileSystems/BlockDevices.h"
2930
#include "libchdr/chd.h"
@@ -35,36 +36,52 @@ extern "C"
3536
#include "ext/libkirk/kirk_engine.h"
3637
};
3738

38-
BlockDevice *constructBlockDevice(FileLoader *fileLoader) {
39+
BlockDevice *ConstructBlockDevice(FileLoader *fileLoader, std::string *errorString) {
3940
if (!fileLoader->Exists()) {
41+
// Shouldn't get here really.
42+
*errorString = "File doesn't exist";
4043
return nullptr;
4144
}
4245
if (fileLoader->IsDirectory()) {
43-
ERROR_LOG(Log::Loader, "Can't open directory directly as block device: %s", fileLoader->GetPath().c_str());
46+
*errorString = "Can't open directory directly as block device: ";
47+
*errorString += fileLoader->GetPath().ToString();
4448
return nullptr;
4549
}
4650

4751
char buffer[8]{};
4852
size_t size = fileLoader->ReadAt(0, 1, 8, buffer);
4953
if (size != 8) {
5054
// Bad or empty file
55+
*errorString = "File is empty";
5156
return nullptr;
5257
}
5358

59+
BlockDevice *device = nullptr;
60+
5461
// Check for CISO
5562
if (!memcmp(buffer, "CISO", 4)) {
56-
return new CISOFileBlockDevice(fileLoader);
63+
device = new CISOFileBlockDevice(fileLoader);
5764
} else if (!memcmp(buffer, "\x00PBP", 4)) {
5865
uint32_t psarOffset = 0;
5966
size = fileLoader->ReadAt(0x24, 1, 4, &psarOffset);
6067
if (size == 4 && psarOffset < fileLoader->FileSize())
61-
return new NPDRMDemoBlockDevice(fileLoader);
68+
device = new NPDRMDemoBlockDevice(fileLoader);
6269
} else if (!memcmp(buffer, "MComprHD", 8)) {
63-
return new CHDFileBlockDevice(fileLoader);
70+
device = new CHDFileBlockDevice(fileLoader);
71+
}
72+
73+
// 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.
74+
if (!device) {
75+
device = new FileBlockDevice(fileLoader);
76+
}
77+
78+
if (!device->IsOK()) {
79+
*errorString = device->ErrorString();
80+
delete device;
81+
return nullptr;
6482
}
6583

66-
// Should be just a regular ISO file. Let's open it as a plain block device and let the other systems take over.
67-
return new FileBlockDevice(fileLoader);
84+
return device;
6885
}
6986

7087
void BlockDevice::NotifyReadError() {
@@ -80,8 +97,7 @@ FileBlockDevice::FileBlockDevice(FileLoader *fileLoader)
8097
filesize_ = fileLoader->FileSize();
8198
}
8299

83-
FileBlockDevice::~FileBlockDevice() {
84-
}
100+
FileBlockDevice::~FileBlockDevice() {}
85101

86102
bool FileBlockDevice::ReadBlock(int blockNumber, u8 *outPtr, bool uncached) {
87103
FileLoader::Flags flags = uncached ? FileLoader::Flags::HINT_UNCACHED : FileLoader::Flags::NONE;
@@ -90,7 +106,6 @@ bool FileBlockDevice::ReadBlock(int blockNumber, u8 *outPtr, bool uncached) {
90106
DEBUG_LOG(Log::FileSystem, "Could not read 2048 byte block, at block offset %d. Only got %d bytes", blockNumber, (int)retval);
91107
return false;
92108
}
93-
94109
return true;
95110
}
96111

@@ -141,17 +156,22 @@ CISOFileBlockDevice::CISOFileBlockDevice(FileLoader *fileLoader)
141156
CISO_H hdr;
142157
size_t readSize = fileLoader->ReadAt(0, sizeof(CISO_H), 1, &hdr);
143158
if (readSize != 1 || memcmp(hdr.magic, "CISO", 4) != 0) {
144-
WARN_LOG(Log::Loader, "Invalid CSO!");
159+
errorString_ = "Invalid CSO!";
160+
return;
145161
}
146162
if (hdr.ver > 1) {
147-
WARN_LOG(Log::Loader, "CSO version too high!");
163+
errorString_ = "CSO version too high!";
164+
return;
148165
}
149166

150167
frameSize = hdr.block_size;
151-
if ((frameSize & (frameSize - 1)) != 0)
152-
ERROR_LOG(Log::Loader, "CSO block size %i unsupported, must be a power of two", frameSize);
153-
else if (frameSize < 0x800)
154-
ERROR_LOG(Log::Loader, "CSO block size %i unsupported, must be at least one sector", frameSize);
168+
if ((frameSize & (frameSize - 1)) != 0) {
169+
errorString_ = StringFromFormat("CSO block size %i unsupported, must be a power of two", frameSize);
170+
return;
171+
} else if (frameSize < 0x800) {
172+
errorString_ = StringFromFormat("CSO block size %i unsupported, must be at least one sector", frameSize);
173+
return;
174+
}
155175

156176
// Determine the translation from block to frame.
157177
blockShift = 0;
@@ -203,10 +223,12 @@ CISOFileBlockDevice::CISOFileBlockDevice(FileLoader *fileLoader)
203223
u64 lastIndexPos = index[indexSize - 1] & 0x7FFFFFFF;
204224
u64 expectedFileSize = lastIndexPos << indexShift;
205225
if (expectedFileSize > fileSize) {
206-
ERROR_LOG(Log::Loader, "Expected CSO to at least be %lld bytes, but file is %lld bytes. File: '%s'",
207-
expectedFileSize, fileSize, fileLoader->GetPath().c_str());
208-
NotifyReadError();
226+
errorString_ = StringFromFormat("Expected CSO to at least be %lld bytes, but file is %lld bytes", expectedFileSize, fileSize);
227+
return;
209228
}
229+
230+
// all ok.
231+
_dbg_assert_(errorString_.empty());
210232
}
211233

212234
CISOFileBlockDevice::~CISOFileBlockDevice()
@@ -389,7 +411,8 @@ NPDRMDemoBlockDevice::NPDRMDemoBlockDevice(FileLoader *fileLoader)
389411
fileLoader_->ReadAt(0x24, 1, 4, &psarOffset);
390412
size_t readSize = fileLoader_->ReadAt(psarOffset, 1, 256, &np_header);
391413
if (readSize != 256){
392-
ERROR_LOG(Log::Loader, "Invalid NPUMDIMG header!");
414+
errorString_ = "Invalid NPUMDIMG header!";
415+
return;
393416
}
394417

395418
u32 psar_id;
@@ -400,6 +423,7 @@ NPDRMDemoBlockDevice::NPDRMDemoBlockDevice(FileLoader *fileLoader)
400423
if (psar_id == 'SISP') {
401424
lbaSize_ = 0; // Mark invalid
402425
ERROR_LOG(Log::Loader, "PSX not supported! Should have been caught earlier.");
426+
errorString_ = "PSX ISOs not supported!";
403427
return;
404428
}
405429

@@ -432,8 +456,7 @@ NPDRMDemoBlockDevice::NPDRMDemoBlockDevice(FileLoader *fileLoader)
432456

433457
// When we remove the above assert, let's just try to survive.
434458
if (blockLBAs_ > 4096) {
435-
ERROR_LOG(Log::Loader, "Bad blockLBAs in header: %08x (%s) psar: %s", blockLBAs_, fileLoader->GetPath().ToVisualString().c_str(), psarStr);
436-
// We'll end up displaying an error message since ReadBlock will fail.
459+
errorString_ = StringFromFormat("Bad blockLBAs in header: %08x (%s) psar: %s", blockLBAs_, fileLoader->GetPath().ToVisualString().c_str(), psarStr);
437460
return;
438461
}
439462

@@ -450,7 +473,8 @@ NPDRMDemoBlockDevice::NPDRMDemoBlockDevice(FileLoader *fileLoader)
450473

451474
readSize = fileLoader_->ReadAt(psarOffset + tableOffset_, 1, tableSize_, table_);
452475
if (readSize != tableSize_){
453-
ERROR_LOG(Log::Loader, "Invalid NPUMDIMG table!");
476+
errorString_ = "Invalid NPUMDIMG table!";
477+
return;
454478
}
455479

456480
u32 *p = (u32*)table_;
@@ -468,6 +492,7 @@ NPDRMDemoBlockDevice::NPDRMDemoBlockDevice(FileLoader *fileLoader)
468492
}
469493

470494
currentBlock_ = -1;
495+
_dbg_assert_(errorString_.empty());
471496
}
472497

473498
NPDRMDemoBlockDevice::~NPDRMDemoBlockDevice() {
@@ -654,8 +679,7 @@ CHDFileBlockDevice::CHDFileBlockDevice(FileLoader *fileLoader)
654679
chd_file *file = nullptr;
655680
chd_error err = chd_open_core_file(&core_file_->core, CHD_OPEN_READ, NULL, &file);
656681
if (err != CHDERR_NONE) {
657-
ERROR_LOG(Log::Loader, "Error loading CHD '%s': %s", paths[depth].c_str(), chd_error_string(err));
658-
NotifyReadError();
682+
errorString_ = StringFromFormat("CHD error: %s", paths[depth].c_str(), chd_error_string(err));
659683
return;
660684
}
661685

@@ -666,6 +690,8 @@ CHDFileBlockDevice::CHDFileBlockDevice(FileLoader *fileLoader)
666690
currentHunk = -1;
667691
blocksPerHunk = impl_->header->hunkbytes / impl_->header->unitbytes;
668692
numBlocks = impl_->header->unitcount;
693+
694+
_dbg_assert_(errorString_.empty());
669695
}
670696

671697
CHDFileBlockDevice::~CHDFileBlockDevice() {

‎Core/FileSystems/BlockDevices.h‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,14 @@ class BlockDevice {
5555

5656
void NotifyReadError();
5757

58+
bool IsOK() const { return errorString_.empty(); }
59+
const std::string &ErrorString() { return errorString_; }
60+
5861
protected:
5962
FileLoader *fileLoader_;
6063
bool reportedError_ = false;
64+
65+
std::string errorString_;
6166
};
6267

6368
class CISOFileBlockDevice : public BlockDevice {
@@ -163,4 +168,4 @@ class CHDFileBlockDevice : public BlockDevice {
163168
u32 numBlocks = 0;
164169
};
165170

166-
BlockDevice *constructBlockDevice(FileLoader *fileLoader);
171+
BlockDevice *ConstructBlockDevice(FileLoader *fileLoader, std::string *errorString);

‎Core/Loaders.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -305,8 +305,8 @@ bool UmdReplace(const Path &filepath, FileLoader **fileLoader, std::string &erro
305305
case IdentifiedFileType::PSP_ISO:
306306
case IdentifiedFileType::PSP_ISO_NP:
307307
case IdentifiedFileType::PSP_DISC_DIRECTORY:
308-
if (!MountGameISO(loadedFile)) {
309-
error = "mounting the new ISO failed";
308+
if (!MountGameISO(loadedFile, &error)) {
309+
error = "mounting the replaced ISO failed: " + error;
310310
return false;
311311
}
312312
break;

‎Core/PSPLoaders.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -60,15 +60,15 @@ static void UseLargeMem(int memsize) {
6060
}
6161
}
6262

63-
bool MountGameISO(FileLoader *fileLoader) {
63+
bool MountGameISO(FileLoader *fileLoader, std::string *errorString) {
6464
std::shared_ptr<IFileSystem> fileSystem;
6565
std::shared_ptr<IFileSystem> blockSystem;
6666

6767
if (fileLoader->IsDirectory()) {
6868
fileSystem = std::make_shared<VirtualDiscFileSystem>(&pspFileSystem, fileLoader->GetPath());
6969
blockSystem = fileSystem;
7070
} else {
71-
auto bd = constructBlockDevice(fileLoader);
71+
auto bd = ConstructBlockDevice(fileLoader, errorString);
7272
if (!bd) {
7373
// Can only fail if the ISO is bad.
7474
return false;
@@ -297,14 +297,16 @@ static Path NormalizePath(const Path &path) {
297297
bool Load_PSP_ELF_PBP(FileLoader *fileLoader, std::string *error_string) {
298298
// This is really just for headless, might need tweaking later.
299299
if (PSP_CoreParameter().mountIsoLoader != nullptr) {
300-
auto bd = constructBlockDevice(PSP_CoreParameter().mountIsoLoader);
301-
if (bd != NULL) {
300+
auto bd = ConstructBlockDevice(PSP_CoreParameter().mountIsoLoader, error_string);
301+
if (bd) {
302302
auto umd2 = std::make_shared<ISOFileSystem>(&pspFileSystem, bd);
303303
auto blockSystem = std::make_shared<ISOBlockSystem>(umd2);
304304

305305
pspFileSystem.Mount("umd1:", blockSystem);
306306
pspFileSystem.Mount("disc0:", umd2);
307307
pspFileSystem.Mount("umd:", blockSystem);
308+
} else {
309+
ERROR_LOG(Log::Loader, "mountIso failed: %s", error_string->c_str());
308310
}
309311
}
310312

‎Core/PSPLoaders.h‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,7 @@ bool Load_PSP_ISO(FileLoader *fileLoader, std::string *error_string);
2525
bool Load_PSP_ELF_PBP(FileLoader *fileLoader, std::string *error_string);
2626
bool Load_PSP_GE_Dump(FileLoader *fileLoader, std::string *error_string);
2727

28-
bool MountGameISO(FileLoader *fileLoader);
28+
bool MountGameISO(FileLoader *fileLoader, std::string *errorString);
2929
bool LoadParamSFOFromDisc();
3030
bool LoadParamSFOFromPBP(FileLoader *fileLoader);
3131
void InitMemorySizeForGame();
32-
33-
void PSPLoaders_Shutdown();

‎Core/Reporting.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,11 +135,15 @@ namespace Reporting
135135
AndroidJNIThreadContext jniContext;
136136

137137
FileLoader *fileLoader = ResolveFileLoaderTarget(ConstructFileLoader(crcFilename));
138-
BlockDevice *blockDevice = constructBlockDevice(fileLoader);
138+
139+
std::string errorString;
140+
BlockDevice *blockDevice = ConstructBlockDevice(fileLoader, &errorString);
139141

140142
u32 crc = 0;
141143
if (blockDevice) {
142144
crc = CalculateCRC(blockDevice, &crcCancel);
145+
} else {
146+
ERROR_LOG(Log::Loader, "Failed to read from block device for CRC: %s", errorString.c_str());
143147
}
144148

145149
delete blockDevice;

‎Core/RetroAchievements.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1037,9 +1037,10 @@ void SetGame(const Path &path, IdentifiedFileType fileType, FileLoader *fileLoad
10371037
//
10381038
// TODO: Fish the block device out of the loading process somewhere else. Though, probably easier to just do it here,
10391039
// we need a temporary blockdevice anyway since it gets consumed by ComputePSPISOHash.
1040-
BlockDevice *blockDevice(constructBlockDevice(fileLoader));
1040+
std::string errorString;
1041+
BlockDevice *blockDevice(ConstructBlockDevice(fileLoader, &errorString));
10411042
if (!blockDevice) {
1042-
ERROR_LOG(Log::Achievements, "Failed to construct block device for '%s' - can't identify", path.c_str());
1043+
ERROR_LOG(Log::Achievements, "Failed to construct block device for '%s' - can't identify: %s", path.c_str(), errorString.c_str());
10431044
g_isIdentifying = false;
10441045
return;
10451046
}
@@ -1100,9 +1101,10 @@ void ChangeUMD(const Path &path, FileLoader *fileLoader) {
11001101
return;
11011102
}
11021103

1103-
BlockDevice *blockDevice = constructBlockDevice(fileLoader);
1104+
std::string errorString;
1105+
BlockDevice *blockDevice = ConstructBlockDevice(fileLoader, &errorString);
11041106
if (!blockDevice) {
1105-
ERROR_LOG(Log::Achievements, "Failed to construct block device for '%s' - can't identify", path.c_str());
1107+
ERROR_LOG(Log::Achievements, "Failed to construct block device for '%s' - can't identify: %s", path.c_str(), errorString.c_str());
11061108
return;
11071109
}
11081110

‎Core/System.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -282,8 +282,8 @@ static bool CPU_Init(FileLoader *fileLoader, IdentifiedFileType type, std::strin
282282
case IdentifiedFileType::PSP_ISO_NP:
283283
case IdentifiedFileType::PSP_DISC_DIRECTORY:
284284
// Doesn't seem to take ownership of fileLoader?
285-
if (!MountGameISO(fileLoader)) {
286-
*errorString = "Failed to mount ISO file - invalid format?";
285+
if (!MountGameISO(fileLoader, errorString)) {
286+
*errorString = "Failed to mount ISO file: " + *errorString;
287287
return false;
288288
}
289289
if (LoadParamSFOFromDisc()) {

‎Core/Util/GameManager.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,8 @@ std::string GameManager::GetPBPGameID(FileLoader *loader) const {
421421

422422
std::string GameManager::GetISOGameID(FileLoader *loader) const {
423423
SequentialHandleAllocator handles;
424-
BlockDevice *bd = constructBlockDevice(loader);
424+
std::string errorString;
425+
BlockDevice *bd = ConstructBlockDevice(loader, &errorString);
425426
if (!bd) {
426427
return "";
427428
}

‎UI/GameInfoCache.cpp‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,8 @@ u64 GameInfo::GetSizeUncompressedInBytes() {
198198
return File::ComputeRecursiveDirectorySize(GetFileLoader()->GetPath());
199199
default:
200200
{
201-
BlockDevice *blockDevice = constructBlockDevice(GetFileLoader().get());
201+
std::string errorString;
202+
BlockDevice *blockDevice = ConstructBlockDevice(GetFileLoader().get(), &errorString);
202203
if (blockDevice) {
203204
u64 size = blockDevice->GetUncompressedSize();
204205
delete blockDevice;
@@ -762,9 +763,9 @@ class GameInfoWorkItem : public Task {
762763
info_->MarkReadyNoLock(flags_);
763764
return;
764765
}
765-
BlockDevice *bd = constructBlockDevice(info_->GetFileLoader().get());
766+
BlockDevice *bd = ConstructBlockDevice(info_->GetFileLoader().get(), &errorString);
766767
if (!bd) {
767-
ERROR_LOG(Log::Loader, "Failed constructing block device for ISO %s", info_->GetFilePath().ToVisualString().c_str());
768+
ERROR_LOG(Log::Loader, "Failed constructing block device for ISO %s: %s", info_->GetFilePath().ToVisualString().c_str(), errorString.c_str());
768769
std::unique_lock<std::mutex> lock(info_->lock);
769770
info_->MarkReadyNoLock(flags_);
770771
return;

0 commit comments

Comments
 (0)