Rewritten CLoadFile::openNextFile to be cleaner and not leak. Desperate messing around for #989.

Global.h

@@ -349,6 +349,9 @@ namespace range = boost::range;
 template<typename T, size_t N> char (&_ArrayCountObj(const T (&)[N]))[N];
 #define ARRAY_COUNT(arr)    (sizeof(_ArrayCountObj(arr)))
 
+
+#define THROW_FORMAT(message, formatting_elems)  throw std::runtime_error(boost::str(boost::format(message) % formatting_elems))
+
 //XXX pls dont - 'debug macros' are usually more trouble than it's worth
 #define HANDLE_EXCEPTION  \
 	catch (const std::exception& e) {	\

client/CPreGame.cpp

@@ -1063,27 +1063,30 @@ void SelectionTab::parseGames(std::vector<FileInfo> &files, bool multi)
 {
 	for(int i=0; i<files.size(); i++)
 	{
-		CLoadFile lf(files[i].name);
+		try
-		if(!lf.sfile)
-			continue;
-
-		ui8 sign[8];
-		lf >> sign;
-		if(std::memcmp(sign,"VCMISVG",7))
 		{
-			tlog1 << files[i].name << " is not a correct savefile!" << std::endl;
+			CLoadFile lf(files[i].name);
-			continue;
+
+			ui8 sign[8];
+			lf >> sign;
+			if(std::memcmp(sign,"VCMISVG",7))
+				throw std::runtime_error("not a correct savefile!");
+
+			allItems[i].mapHeader = new CMapHeader();
+			lf >> *(allItems[i].mapHeader) >> allItems[i].scenarioOpts;
+			allItems[i].filename = files[i].name;
+			allItems[i].countPlayers();
+			allItems[i].date = std::asctime(std::localtime(&files[i].date));
+
+			if((allItems[i].actualHumanPlayers > 1) != multi) //if multi mode then only multi games, otherwise single
+			{
+				vstd::clear_pointer(allItems[i].mapHeader);
+			}
 		}
-		allItems[i].mapHeader = new CMapHeader();
+		catch(std::exception &e)
-		lf >> *(allItems[i].mapHeader) >> allItems[i].scenarioOpts;
-		allItems[i].filename = files[i].name;
-		allItems[i].countPlayers();
-		allItems[i].date = std::asctime(std::localtime(&files[i].date));
-
-		if((allItems[i].actualHumanPlayers > 1) != multi) //if multi mode then only multi games, otherwise single
 		{
-			delete allItems[i].mapHeader;
+			vstd::clear_pointer(allItems[i].mapHeader);
-			allItems[i].mapHeader = NULL;
+			tlog3 << "Failed to process " << files[i].name <<": " << e.what() << std::endl;
 		}
 	}
 }

lib/Connection.cpp

@@ -302,54 +302,56 @@ CLoadFile::~CLoadFile()
 
 int CLoadFile::read( const void * data, unsigned size )
 {
-	char *bytePtr = (char *)data;
+	sfile->read((char *)data,size);
-	sfile->read(bytePtr, size);
 	return size;
 }
 
 void CLoadFile::openNextFile(const std::string &fname, int minimalVersion)
 {
-	fName = fname;
+	assert(!reverseEndianess);
-	sfile = make_unique<std::ifstream>(fname.c_str(),std::ios::binary);
+	assert(minimalVersion <= version);
-	if(!(*sfile))
+
-	{
+	try
-		tlog1 << "Error: cannot open to read " << fname << std::endl;
-		sfile.release();
-	}
-	else
 	{
+		fName = fname;
+		sfile = make_unique<std::ifstream>(fname, std::ios::binary);
+		sfile->exceptions(std::ifstream::failbit | std::ifstream::badbit); //we throw a lot anyway
+
+		if(!(*sfile))
+			THROW_FORMAT("Error: cannot open to read %s!", fname);
+
+		//we can read
 		char buffer[4];
 		sfile->read(buffer, 4);
-
 		if(std::memcmp(buffer,"VCMI",4))
-		{
+			THROW_FORMAT("Error: not a VCMI file(%s)!", fname);
-			tlog1 << "Error: not a VCMI file! ( " << fname << " )\n";
-			sfile.release();
-			return;
-		}
 
-		*this >> myVersion;	
+		*this >> fileVersion;	
-		if(myVersion < minimalVersion)
+		if(fileVersion < minimalVersion)
+			THROW_FORMAT("Error: too old file format (%s)!", fname);
+
+		if(fileVersion > version)
 		{
-			tlog1 << "Error: Too old file format! (file " << fname << " )\n";
+			tlog3 << boost::format("Warning format version mismatch: found %d when current is %d! (file %s)\n") % fileVersion % version % fname;
-			sfile.release();
+
-		}
+			auto versionptr = (char*)&fileVersion;
-		if(myVersion > version)
-		{
-			auto versionptr = (char*)&myVersion;
 			std::reverse(versionptr, versionptr + 4);
-			if(myVersion == version)
+			tlog3 << "Version number reversed is " << fileVersion << ", checking...\n";
+
+			if(fileVersion == version)
 			{
+				tlog3 << fname << " seems to have different endianess! Entering reversing mode.\n";
 				reverseEndianess = true;
-				tlog3 << fname << " seems to have different endianess!\n";
 			}
 			else
-			{
+				THROW_FORMAT("Error: too new file format (%s)!", fname);
-				tlog1 << "Error: Too new file format! (file " << fname << " )\n";
-				sfile.release();
-			}
 		}
 	}
+	catch(...)
+	{
+		clear(); //if anything went wrong, we delete file and rethrow
+		throw;
+	}
 }
 
 void CLoadFile::reportState(CLogger &out)
@@ -361,6 +363,13 @@ void CLoadFile::reportState(CLogger &out)
 	}
 }
 
+void CLoadFile::clear()
+{
+	sfile = nullptr;
+	fName.clear();
+	fileVersion = 0;
+}
+
 CTypeList::CTypeList()
 {
 	registerTypes(*this);

lib/Connection.h

@@ -686,7 +686,7 @@ template <typename Serializer> class DLL_LINKAGE CISer : public CLoaderBase
 public:
 	bool saving;
 	std::map<ui16,CBasicPointerLoader*> loaders; // typeID => CPointerSaver<serializer,type>
-	ui32 myVersion;
+	ui32 fileVersion;
 	bool reverseEndianess; //if source has different endianess than us, we reverse bytes
 
 	std::map<ui32, void*> loadedPointers;
@@ -695,7 +695,7 @@ public:
 	CISer()
 	{
 		saving = false;
-		myVersion = version;
+		fileVersion = 0;
 		smartPointerSerialization = true;
 		reverseEndianess = false;
 	}
@@ -760,12 +760,18 @@ public:
 	template <typename T>
 	void loadPrimitive(T &data)
 	{
-		char * dataPtr = (char*)&data;
+		if(0) //for testing #989
-		unsigned length = sizeof(data);
+		{
-
+ 			this->This()->read(&data,sizeof(data));
-		this->This()->read(dataPtr,length);
+		}
-		if(reverseEndianess)
+		else
-			std::reverse(dataPtr, dataPtr + length);
+		{
+			unsigned length = sizeof(data);
+			char* dataPtr = (char*)&data;
+			this->This()->read(dataPtr,length);
+			if(reverseEndianess)
+				std::reverse(dataPtr, dataPtr + length);
+		}
 	}
 
 	template <typename T>
@@ -774,7 +780,7 @@ public:
 		////that const cast is evil because it allows to implicitly overwrite const objects when deserializing
 		typedef typename boost::remove_const<T>::type nonConstT;
 		nonConstT &hlp = const_cast<nonConstT&>(data);
-		hlp.serialize(*this,myVersion);
+		hlp.serialize(*this,fileVersion);
 		//data.serialize(*this,myVersion);
 	}	
 
@@ -1018,11 +1024,13 @@ public:
 	std::string fName;
 	unique_ptr<std::ifstream> sfile;
 
-	CLoadFile(const std::string &fname, int minimalVersion = version);
+	CLoadFile(const std::string &fname, int minimalVersion = version); //throws!
 	~CLoadFile();
 	int read(const void * data, unsigned size);
 
-	void openNextFile(const std::string &fname, int minimalVersion);
+	void openNextFile(const std::string &fname, int minimalVersion); //throws!
+
+	void clear();
 	void reportState(CLogger &out);
 };