Make fs::extractZipFile thread-safe

Author: sfan5

src/filesys.cpp

@@ -28,11 +28,18 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include "log.h"
 #include "config.h"
 #include "porting.h"
+#ifndef SERVER
+#include "irr_ptr.h"
+#endif
 
 namespace fs
 {
 
-#ifdef _WIN32 // WINDOWS
+#ifdef _WIN32
+
+/***********
+ * Windows *
+ ***********/
 
 #define _WIN32_WINNT 0x0501
 #include <windows.h>
@@ -201,7 +208,11 @@ std::string CreateTempFile()
 	return path;
 }
 
-#else // POSIX
+#else
+
+/*********
+ * POSIX *
+ *********/
 
 #include <sys/types.h>
 #include <dirent.h>
@@ -392,6 +403,10 @@ std::string CreateTempFile()
 
 #endif
 
+/****************************
+ * portable implementations *
+ ****************************/
+
 void GetRecursiveDirs(std::vector<std::string> &dirs, const std::string &dir)
 {
 	static const std::set<char> chars_to_ignore = { '_', '.' };
@@ -753,69 +768,66 @@ bool safeWriteToFile(const std::string &path, const std::string &content)
 	return true;
 }
 
+#ifndef SERVER
 bool extractZipFile(io::IFileSystem *fs, const char *filename, const std::string &destination)
 {
-	if (!fs->addFileArchive(filename, false, false, io::EFAT_ZIP)) {
+	// Be careful here not to touch the global file hierarchy in Irrlicht
+	// since this function needs to be thread-safe!
+
+	io::IArchiveLoader *zip_loader = nullptr;
+	for (u32 i = 0; i < fs->getArchiveLoaderCount(); i++) {
+		if (fs->getArchiveLoader(i)->isALoadableFileFormat(io::EFAT_ZIP)) {
+			zip_loader = fs->getArchiveLoader(i);
+			break;
+		}
+	}
+	if (!zip_loader) {
+		warningstream << "fs::extractZipFile(): Irrlicht said it doesn't support ZIPs." << std::endl;
 		return false;
 	}
 
-	sanity_check(fs->getFileArchiveCount() > 0);
-
-	/**********************************************************************/
-	/* WARNING this is not threadsafe!!                                   */
-	/**********************************************************************/
-	io::IFileArchive* opened_zip = fs->getFileArchive(fs->getFileArchiveCount() - 1);
-
+	irr_ptr<io::IFileArchive> opened_zip(zip_loader->createArchive(filename, false, false));
 	const io::IFileList* files_in_zip = opened_zip->getFileList();
 
-	unsigned int number_of_files = files_in_zip->getFileCount();
-
-	for (unsigned int i=0; i < number_of_files; i++) {
-		std::string fullpath = destination;
-		fullpath += DIR_DELIM;
+	for (u32 i = 0; i < files_in_zip->getFileCount(); i++) {
+		std::string fullpath = destination + DIR_DELIM;
 		fullpath += files_in_zip->getFullFileName(i).c_str();
 		std::string fullpath_dir = fs::RemoveLastPathComponent(fullpath);
 
-		if (!files_in_zip->isDirectory(i)) {
-			if (!fs::PathExists(fullpath_dir) && !fs::CreateAllDirs(fullpath_dir)) {
-				fs->removeFileArchive(fs->getFileArchiveCount()-1);
-				return false;
-			}
-
-			io::IReadFile* toread = opened_zip->createAndOpenFile(i);
+		if (files_in_zip->isDirectory(i))
+			continue; // ignore, we create dirs as necessary
 
-			FILE *targetfile = fopen(fullpath.c_str(),"wb");
+		if (!fs::PathExists(fullpath_dir) && !fs::CreateAllDirs(fullpath_dir))
+			return false;
 
-			if (targetfile == NULL) {
-				fs->removeFileArchive(fs->getFileArchiveCount()-1);
-				return false;
-			}
+		irr_ptr<io::IReadFile> toread(opened_zip->createAndOpenFile(i));
 
-			char read_buffer[1024];
-			long total_read = 0;
+		std::ofstream os(fullpath.c_str(), std::ios::binary);
+		if (!os.good())
+			return false;
 
-			while (total_read < toread->getSize()) {
+		char buffer[4096];
+		long total_read = 0;
 
-				unsigned int bytes_read =
-						toread->read(read_buffer,sizeof(read_buffer));
-				if ((bytes_read == 0 ) ||
-					(fwrite(read_buffer, 1, bytes_read, targetfile) != bytes_read))
-				{
-					fclose(targetfile);
-					fs->removeFileArchive(fs->getFileArchiveCount() - 1);
-					return false;
-				}
-				total_read += bytes_read;
+		while (total_read < toread->getSize()) {
+			long bytes_read = toread->read(buffer, sizeof(buffer));
+			bool error = true;
+			if (bytes_read != 0) {
+				os.write(buffer, bytes_read);
+				error = os.fail();
 			}
-
-			fclose(targetfile);
+			if (error) {
+				os.close();
+				remove(fullpath.c_str());
+				return false;
+			}
+			total_read += bytes_read;
 		}
-
 	}
 
-	fs->removeFileArchive(fs->getFileArchiveCount() - 1);
 	return true;
 }
+#endif
 
 bool ReadFile(const std::string &path, std::string &out)
 {
@@ -829,7 +841,7 @@ bool ReadFile(const std::string &path, std::string &out)
 	is.seekg(0);
 	is.read(&out[0], size);
 
-	return true;
+	return !is.fail();
 }
 
 bool Rename(const std::string &from, const std::string &to)

src/filesys.h

@@ -24,12 +24,12 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include <vector>
 #include "exceptions.h"
 
-#ifdef _WIN32 // WINDOWS
+#ifdef _WIN32
 #define DIR_DELIM "\\"
 #define DIR_DELIM_CHAR '\\'
 #define FILESYS_CASE_INSENSITIVE true
 #define PATH_DELIM ";"
-#else // POSIX
+#else
 #define DIR_DELIM "/"
 #define DIR_DELIM_CHAR '/'
 #define FILESYS_CASE_INSENSITIVE false
@@ -133,7 +133,9 @@ const char *GetFilenameFromPath(const char *path);
 
 bool safeWriteToFile(const std::string &path, const std::string &content);
 
+#ifndef SERVER
 bool extractZipFile(irr::io::IFileSystem *fs, const char *filename, const std::string &destination);
+#endif
 
 bool ReadFile(const std::string &path, std::string &out);
 

src/script/lua_api/l_mainmenu.cpp

@@ -644,9 +644,9 @@ int ModApiMainMenu::l_extract_zip(lua_State *L)
 	std::string absolute_destination = fs::RemoveRelativePathComponents(destination);
 
 	if (ModApiMainMenu::mayModifyPath(absolute_destination)) {
-		auto rendering_engine = getGuiEngine(L)->m_rendering_engine;
-		fs::CreateAllDirs(absolute_destination);
-		lua_pushboolean(L, fs::extractZipFile(rendering_engine->get_filesystem(), zipfile, destination));
+		auto fs = RenderingEngine::get_raw_device()->getFileSystem();
+		bool ok = fs::extractZipFile(fs, zipfile, destination);
+		lua_pushboolean(L, ok);
 		return 1;
 	}
 
@@ -916,7 +916,7 @@ void ModApiMainMenu::InitializeAsync(lua_State *L, int top)
 	API_FCT(delete_dir);
 	API_FCT(copy_dir);
 	API_FCT(is_dir);
-	//API_FCT(extract_zip); //TODO remove dependency to GuiEngine
+	API_FCT(extract_zip);
 	API_FCT(may_modify_path);
 	API_FCT(download_file);
 	API_FCT(get_min_supp_proto);