Firmware installation bugfixes part 1

* Disallow concurrent (multiple files at once) PUP installations because they can cause race conditions with each other. * Fix race condition in PUP installation abortion. * Fix freezes of emulator in case the PUP installation failed due to filesystem errors. * Use fs::create_path as opposed to fs::create_dir as it is can create upper directories in case they are missing and is better in error handling. * Report TAR errors on failure to create directories. * Fix pup_object constructor to not crash on invalid PUP file header. (report an error) * Fix pup_object::validate_hashes to not crash on invalid PUP file entries. (report an error) * Do not call Qt functions inside a named_thread because it is wrong.
RPCS3 · Feb 27, 2021 · c05ccc5 · c05ccc5
1 parent d2de8a8
commit c05ccc5
Show file tree

Hide file tree

Showing 5 changed files with 85 additions and 35 deletions.
diff --git a/rpcs3/Loader/PUP.cpp b/rpcs3/Loader/PUP.cpp
@@ -9,23 +9,37 @@ pup_object::pup_object(const fs::file& file): m_file(file)
 {
 	if (!file)
 	{
-		isValid = false;
 		return;
 	}
 
 	m_file.seek(0);
-	PUPHeader m_header;
-	m_file.read(m_header);
-	if (m_header.magic != "SCEUF\0\0\0"_u64)
+	PUPHeader m_header{};
+
+	if (!m_file.read(m_header) || m_header.magic != "SCEUF\0\0\0"_u64)
+	{
+		// Either file is not large enough to contain header or magic is invalid
+		return;
+	}
+
+	constexpr usz entry_size = sizeof(PUPFileEntry) + sizeof(PUPHashEntry);
+
+	if (!m_header.file_count || (m_file.size() - sizeof(PUPHeader)) / entry_size < m_header.file_count)
 	{
-		isValid = false;
+		// These checks before read() are to avoid some std::bad_alloc exceptions when file_count is too large
+		// So we cannot rely on read() for error checking in such cases
 		return;
 	}
 
 	m_file_tbl.resize(m_header.file_count);
-	m_file.read(m_file_tbl);
 	m_hash_tbl.resize(m_header.file_count);
-	m_file.read(m_hash_tbl);
+
+	if (!m_file.read(m_file_tbl) || !m_file.read(m_hash_tbl))
+	{
+		// If these fail it is an unexpected filesystem error, because file size must suffice as we checked in previous checks
+		return;
+	}
+
+	isValid = true;
 }
 
 fs::file pup_object::get_file(u64 entry_id)
@@ -47,11 +61,18 @@ fs::file pup_object::get_file(u64 entry_id)
 
 bool pup_object::validate_hashes()
 {
+	if (!isValid) return false;
+
 	for (usz i = 0; i < m_file_tbl.size(); i++)
 	{
 		u8 *hash = m_hash_tbl[i].hash;
 		PUPFileEntry file = m_file_tbl[i];
 
+		if (usz size = m_file.size(); size < file.data_offset || size - file.data_offset < file.data_length)
+		{
+			return false;
+		}
+
 		std::vector<u8> buffer(file.data_length);
 		m_file.seek(file.data_offset);
 		m_file.read(buffer.data(), file.data_length);

diff --git a/rpcs3/Loader/PUP.h b/rpcs3/Loader/PUP.h
@@ -33,7 +33,7 @@ struct PUPHashEntry
 class pup_object
 {
 	const fs::file& m_file;
-	bool isValid = true;
+	bool isValid = false;
 
 	std::vector<PUPFileEntry> m_file_tbl;
 	std::vector<PUPHashEntry> m_hash_tbl;

diff --git a/rpcs3/Loader/TAR.cpp b/rpcs3/Loader/TAR.cpp
@@ -131,7 +131,12 @@ bool tar_object::extract(std::string path, std::string ignore)
 
 		case '5':
 		{
-			fs::create_dir(result);
+			if (!fs::create_path(result))
+			{
+				tar_log.error("TAR Loader: failed to create directory %s (%s)", header.name, fs::g_tls_error);
+				return false;
+			}
+
 			break;
 		}
 

diff --git a/rpcs3/rpcs3qt/main_window.cpp b/rpcs3/rpcs3qt/main_window.cpp
@@ -789,21 +789,39 @@ void main_window::HandlePupInstallation(QString file_path)
 	Emu.SetForceBoot(true);
 	Emu.Stop();
 
-	m_gui_settings->SetValue(gui::fd_install_pup, QFileInfo(file_path).path());
 	const std::string path = sstr(file_path);
 
+	auto critical = [mw = this](QString str)
+	{
+		Emu.CallAfter([mw, str = std::move(str)]()
+		{
+			QMessageBox::critical(mw, tr("Firmware Installation Failed"), str);
+		});
+	};
+
+	std::unique_lock lock(m_pup_sema, std::try_to_lock);
+
+	if (!lock)
+	{
+		gui_log.error("Concurrent PUP installation!");
+		critical(tr("Trying to install multiple PUP files at once!"));
+		return;
+	}
+
+	m_gui_settings->SetValue(gui::fd_install_pup, QFileInfo(file_path).path());
+
 	fs::file pup_f(path);
 	if (!pup_f)
 	{
 		gui_log.error("Error opening PUP file %s", path);
-		QMessageBox::critical(this, tr("Firmware Installation Failed"), tr("Firmware installation failed: The selected firmware file couldn't be opened."));
+		critical(tr("Firmware installation failed: The selected firmware file couldn't be opened."));
 		return;
 	}
 
 	if (pup_f.size() < sizeof(PUPHeader))
 	{
 		gui_log.error("Too small PUP file: %llu", pup_f.size());
-		QMessageBox::critical(this, tr("Firmware Installation Failed"), tr("Firmware installation failed: The provided file is empty."));
+		critical(tr("Firmware installation failed: The provided file is empty."));
 		return;
 	}
 
@@ -814,22 +832,22 @@ void main_window::HandlePupInstallation(QString file_path)
 	if (header.header_length + header.data_length != pup_f.size())
 	{
 		gui_log.error("Firmware size mismatch, expected: %llu + %llu, actual: %llu", header.header_length, header.data_length, pup_f.size());
-		QMessageBox::critical(this, tr("Firmware Installation Failed"), tr("Firmware installation failed: The provided file is incomplete. Try redownloading it."));
+		critical(tr("Firmware installation failed: The provided file is incomplete. Try redownloading it."));
 		return;
 	}
 
 	pup_object pup(pup_f);
 	if (!pup)
 	{
 		gui_log.error("Error while installing firmware: PUP file is invalid.");
-		QMessageBox::critical(this, tr("Firmware Installation Failed"), tr("Firmware installation failed: The provided file is corrupted."));
+		critical(tr("Firmware installation failed: The provided file is corrupted."));
 		return;
 	}
 
 	if (!pup.validate_hashes())
 	{
 		gui_log.error("Error while installing firmware: Hash check failed.");
-		QMessageBox::critical(this, tr("Firmware Installation Failed"), tr("Firmware installation failed: The provided file's contents are corrupted."));
+		critical(tr("Firmware installation failed: The provided file's contents are corrupted."));
 		return;
 	}
 
@@ -864,15 +882,13 @@ void main_window::HandlePupInstallation(QString file_path)
 	pdlg.show();
 
 	// Synchronization variable
-	atomic_t<int> progress(0);
+	atomic_t<uint> progress(0);
 	{
 		// Run asynchronously
 		named_thread worker("Firmware Installer", [&]
 		{
 			for (const auto& update_filename : update_filenames)
 			{
-				if (progress == -1) break;
-
 				fs::file update_file = update_files.get_file(update_filename);
 
 				SCEDecrypter self_dec(update_file);
@@ -884,51 +900,55 @@ void main_window::HandlePupInstallation(QString file_path)
 				if (dev_flash_tar_f.size() < 3)
 				{
 					gui_log.error("Error while installing firmware: PUP contents are invalid.");
-					QMessageBox::critical(this, tr("Firmware Installation Failed"), tr("Firmware installation failed: Firmware could not be decompressed"));
+					critical(tr("Firmware installation failed: Firmware could not be decompressed"));
 					progress = -1;
+					return;
 				}
 
 				tar_object dev_flash_tar(dev_flash_tar_f[2]);
 				if (!dev_flash_tar.extract(g_cfg.vfs.get_dev_flash(), "dev_flash/"))
 				{
 					gui_log.error("Error while installing firmware: TAR contents are invalid.");
-					QMessageBox::critical(this, tr("Firmware Installation Failed"), tr("Firmware installation failed: Firmware contents could not be extracted."));
+					critical(tr("Firmware installation failed: Firmware contents could not be extracted."));
 					progress = -1;
+					return;
 				}
 
-				if (progress >= 0)
-					progress += 1;
+				if (!progress.try_inc(::narrow<uint>(update_filenames.size())))
+				{
+					return;
+				}
 			}
 		});
 
 		// Wait for the completion
-		while (std::this_thread::sleep_for(5ms), std::abs(progress) < pdlg.maximum())
+		for (uint value = progress.load(); value < update_filenames.size(); std::this_thread::sleep_for(5ms), value = progress)
 		{
+			// Update progress window
+			pdlg.SetValue(value);
+			QCoreApplication::processEvents();
+
 			if (pdlg.wasCanceled())
 			{
 				progress = -1;
 				break;
 			}
-			// Update progress window
-			pdlg.SetValue(static_cast<int>(progress));
-			QCoreApplication::processEvents();
 		}
 
-		update_files_f.close();
-		pup_f.close();
-
-		if (progress > 0)
-		{
-			pdlg.SetValue(pdlg.maximum());
-			std::this_thread::sleep_for(100ms);
-		}
+		worker();
 	}
 
+	update_files_f.close();
+	pup_f.close();
+
 	// Update with newly installed PS3 fonts
 	Q_EMIT RequestGlobalStylesheetChange();
 
-	if (progress > 0)
+	if (progress == update_filenames.size())
 	{
+		pdlg.SetValue(pdlg.maximum());
+		std::this_thread::sleep_for(100ms);
+
 		gui_log.success("Successfully installed PS3 firmware version %s.", version_string);
 		m_gui_settings->ShowInfoBox(tr("Success!"), tr("Successfully installed PS3 firmware and LLE Modules!"), gui::ib_pup_success, this);
 

diff --git a/rpcs3/rpcs3qt/main_window.h b/rpcs3/rpcs3qt/main_window.h
@@ -13,6 +13,8 @@
 #include "update_manager.h"
 #include "settings.h"
 
+#include "Utilities/sema.h"
+
 #include <memory>
 
 class log_frame;
@@ -48,6 +50,8 @@ class main_window : public QMainWindow
 	bool m_save_slider_pos = false;
 	int m_other_slider_pos = 0;
 
+	::semaphore<> m_pup_sema;
+
 	QIcon m_app_icon;
 	QIcon m_icon_play;
 	QIcon m_icon_pause;