Merge pull request #22260 from hrydgard/iso-trailing-partial-sector

Don't drop an ISO's trailing partial sector

Core/ELF/ElfReader.cpp

@@ -544,8 +544,18 @@ int ElfReader::LoadInto(u32 loadAddress, bool fromTop) {
 	if (header->e_ident[EI_DATA] != ELFDATA2LSB)
 		return SCE_KERNEL_ERROR_MEMBLOCK_ALLOC_FAILED;
 
-	if (size_ < header->e_phoff + sizeof(Elf32_Phdr) * GetNumSegments() || size_ < header->e_shoff + sizeof(Elf32_Shdr) * GetNumSections()) {
-		ERROR_LOG(Log::Loader, "Truncated ELF, %d bytes with %d sections and %d segments", (int)size_, GetNumSections(), GetNumSegments());
+	const size_t phdrEnd = header->e_phoff + sizeof(Elf32_Phdr) * GetNumSegments();
+	const size_t shdrEnd = header->e_shoff + sizeof(Elf32_Shdr) * GetNumSections();
+	if (size_ < phdrEnd || size_ < shdrEnd) {
+		// Say which table runs off the end and by how much. "Truncated ELF" on its own sends you
+		// looking at the executable, when the usual cause is that we were handed fewer bytes than
+		// the file really has - a short read, or a size clamped somewhere upstream.
+		const char *which = size_ < phdrEnd ? "program header table" : "section header table";
+		const size_t needed = size_ < phdrEnd ? phdrEnd : shdrEnd;
+		loadError_ = StringFromFormat(
+			"Truncated ELF: %s ends at %d but only %d bytes are available (%d short), %d sections, %d segments",
+			which, (int)needed, (int)size_, (int)(needed - size_), GetNumSections(), GetNumSegments());
+		ERROR_LOG(Log::Loader, "%s", loadError_.c_str());
 		// Probably not the right error code.
 		return SCE_KERNEL_ERROR_MEMBLOCK_ALLOC_FAILED;
 	}

Core/ELF/ElfReader.h

@@ -174,6 +174,13 @@ public:
 	std::vector<SectionID> GetCodeSections() const;
 
 	int LoadInto(u32 vaddr, bool fromTop);
+
+	// Set when LoadInto() rejects the image, so the caller can put the reason in the error it
+	// shows instead of only an error code. Untranslated - this is for developers and bug reports.
+	const std::string &LoadError() const {
+		return loadError_;
+	}
+
 	bool LoadSymbols();
 	bool LoadRelocations(const Elf32_Rel *rels, int numRelocs);
 	void LoadRelocations2(int rel_seg);
@@ -196,6 +203,7 @@ private:
 	std::vector<u32> segmentVAddr;
 	size_t size_ = 0;
 	u32 firstSegAlign = 0;
+	std::string loadError_;
 };
 
 // Homebrew usually ships the unstripped ELF it was built from next to the EBOOT (app.elf beside

Core/FileSystems/BlockDevices.cpp

@@ -306,8 +306,18 @@ FileBlockDevice::~FileBlockDevice() {}
 
 bool FileBlockDevice::ReadBlock(int blockNumber, u8 *outPtr, bool uncached) {
 	FileLoader::Flags flags = uncached ? FileLoader::Flags::HINT_UNCACHED : FileLoader::Flags::NONE;
-	size_t retval = fileLoader_->ReadAt((u64)blockNumber * (u64)GetBlockSize(), 1, 2048, outPtr, flags);
+	const u64 offset = (u64)blockNumber * (u64)GetBlockSize();
+	size_t retval = fileLoader_->ReadAt(offset, 1, 2048, outPtr, flags);
 	if (retval != 2048) {
+		// Not every image is a whole number of sectors. Tools that build pre-patched ISOs do write
+		// images that stop in the middle of their last sector, with a file legitimately ending
+		// there. The bytes that are present are real, so zero the rest of the sector and report
+		// success. Failing instead loses them: callers substitute an all-zero sector, which
+		// quietly corrupts whatever was in that tail.
+		if (retval > 0 && offset < filesize_) {
+			memset(outPtr + retval, 0, 2048 - retval);
+			return true;
+		}
 		DEBUG_LOG(Log::FileSystem, "Could not read 2048 byte block, at block offset %d. Only got %d bytes", blockNumber, (int)retval);
 		return false;
 	}

Core/FileSystems/ISOFileSystem.cpp

@@ -307,10 +307,15 @@ void ISOFileSystem::ReadDirectory(TreeEntry *root) const {
 			// drop the entry - truncated ISOs are common and used to work with just the warning
 			// above, and dropping EBOOT.BIN would turn that into an unbootable game. For a sane
 			// file this is a no-op, since the extent always fits in its sectors.
+			// Measured in bytes, not whole sectors: an image whose length isn't a multiple of the
+			// sector size still contains its final partial sector, and a file is allowed to end
+			// there. Counting blocks discards that tail, which clamped real files short - a dump
+			// with EBOOT.BIN running to the last byte of the image lost the end of it, and the ELF
+			// section headers that live there went with it.
 			if (isFile) {
-				const u64 numBlocks = blockDevice->GetNumBlocks();
-				const u64 firstSector = dir.firstDataSector;
-				const s64 availableBytes = firstSector >= numBlocks ? 0 : (s64)((numBlocks - firstSector) * (u64)sectorSize);
+				const u64 imageBytes = blockDevice->GetUncompressedSize();
+				const u64 firstByte = (u64)dir.firstDataSector * (u64)sectorSize;
+				const s64 availableBytes = firstByte >= imageBytes ? 0 : (s64)(imageBytes - firstByte);
 				if (entry->size > availableBytes) {
 					entry->size = availableBytes;
 				}

Core/HLE/sceKernelModule.cpp

@@ -1468,7 +1468,13 @@ static PSPModule *__KernelLoadELFFromPtr(const u8 *ptr, size_t elfSize, u32 load
 
 	int result = reader.LoadInto(loadAddress, fromTop);
 	if (result != SCE_KERNEL_ERROR_OK) {
-		ERROR_LOG(Log::sceModule, "LoadInto failed with error %08x",result);
+		// Carry the reader's reason up, so what the user is shown says more than an error code.
+		if (!reader.LoadError().empty()) {
+			*error_string = reader.LoadError();
+		} else {
+			*error_string = StringFromFormat("ELF load failed (%08x)", result);
+		}
+		ERROR_LOG(Log::sceModule, "LoadInto failed with error %08x: %s", result, error_string->c_str());
 		delete [] newptr;
 		module->Cleanup();
 		kernelObjects.Destroy<PSPModule>(module->GetUID());
@@ -2092,7 +2098,8 @@ static bool __KernelLoadExecFromPtr(MIPSState * mips, const u8 *data, size_t siz
 			module->Cleanup();
 			kernelObjects.Destroy<PSPModule>(module->GetUID());
 		}
-		ERROR_LOG(Log::Loader, "Failed to load module %s", filename);
+		ERROR_LOG(Log::Loader, "Failed to load module %s (%d bytes): %s", filename, (int)size,
+			error_string->empty() ? "no reason given" : error_string->c_str());
 		*error_string = "Failed to load executable: " + *error_string;
 		delete[] param_argp;
 		delete[] param_key;