Merge pull request #452 from karliss/sanitizers2

Fix memory and other problems found by address and UB sanitizers.

client/CBitmapHandler.cpp

@@ -25,9 +25,9 @@ namespace BitmapHandler
 
 bool isPCX(const ui8 *header)//check whether file can be PCX according to header
 {
-	int fSize  = read_le_u32(header + 0);
-	int width  = read_le_u32(header + 4);
-	int height = read_le_u32(header + 8);
+	ui32 fSize  = read_le_u32(header + 0);
+	ui32 width  = read_le_u32(header + 4);
+	ui32 height = read_le_u32(header + 8);
 	return fSize == width*height || fSize == width*height*3;
 }
 

client/CMT.cpp

@@ -985,7 +985,6 @@ void dispose()
 	}
 
 	// cleanup, mostly to remove false leaks from analyzer
-	CResourceHandler::clear();
 	if(CCS)
 	{
 		CCS->musich->release();

client/CServerHandler.cpp

@@ -108,7 +108,7 @@ void CServerHandler::resetStateForLobby(const StartInfo::EMode mode, const std::
 	th = make_unique<CStopWatch>();
 	packsForLobbyScreen.clear();
 	c.reset();
-	si.reset(new StartInfo());
+	si = std::make_shared<StartInfo>();
 	playerNames.clear();
 	si->difficulty = 1;
 	si->mode = mode;
@@ -475,11 +475,13 @@ void CServerHandler::endGameplay(bool closeConnection)
 	client->endGame();
 	vstd::clear_pointer(client);
 
-	// Game is ending
-	// Tell the network thread to reach a stable state
-	CSH->sendClientDisconnecting();
-	logNetwork->info("Closed connection.");
-
+	if(closeConnection)
+	{
+		// Game is ending
+		// Tell the network thread to reach a stable state
+		CSH->sendClientDisconnecting();
+		logNetwork->info("Closed connection.");
+	}
 	if(CMM)
 	{
 		GH.curInt = CMM;

client/battle/CBattleAnimations.cpp

@@ -1129,7 +1129,6 @@ bool CEffectAnimation::init()
 	else // Effects targeted at a specific creature/hex.
 	{
 		const CStack * destStack = owner->getCurrentPlayerInterface()->cb->battleGetStackByPos(destTile, false);
-		Rect & tilePos = owner->bfield[destTile]->pos;
 		BattleEffect be;
 		be.effectID = ID;
 		be.animation = animation;
@@ -1141,30 +1140,27 @@ bool CEffectAnimation::init()
 //			if(effect == 1)
 //				be.maxFrame = 3;
 
-		if(x == -1)
+		be.x = x;
+		be.y = y;
+		if(destTile.isValid())
 		{
-			be.x = tilePos.x + tilePos.w/2 - first->width()/2;
-		}
-		else
-		{
-			be.x = x;
+			Rect & tilePos = owner->bfield[destTile]->pos;
+			if(x == -1)
+				be.x = tilePos.x + tilePos.w/2 - first->width()/2;
+			if(y == -1)
+			{
+				if(alignToBottom)
+					be.y = tilePos.y + tilePos.h - first->height();
+				else
+					be.y = tilePos.y - first->height()/2;
+			}
+
+			// Correction for 2-hex creatures.
+			if(destStack != nullptr && destStack->doubleWide())
+				be.x += (destStack->side == BattleSide::ATTACKER ? -1 : 1)*tilePos.w/2;
 		}
 
-		if(y == -1)
-		{
-			if(alignToBottom)
-				be.y = tilePos.y + tilePos.h - first->height();
-			else
-				be.y = tilePos.y - first->height()/2;
-		}
-		else
-		{
-			be.y = y;
-		}
-
-		// Correction for 2-hex creatures.
-		if(destStack != nullptr && destStack->doubleWide())
-			be.x += (destStack->side == BattleSide::ATTACKER ? -1 : 1)*tilePos.w/2;
+		assert(be.x != -1 && be.y != -1);
 
 		//Indicate if effect should be drawn on top of everything or just on top of the hex
 		be.position = destTile;

launcher/modManager/cmodlistview_moc.cpp

@@ -24,13 +24,13 @@
 
 void CModListView::setupModModel()
 {
-	modModel = new CModListModel();
-	manager = new CModManager(modModel);
+	modModel = new CModListModel(this);
+	manager = vstd::make_unique<CModManager>(modModel);
 }
 
 void CModListView::setupFilterModel()
 {
-	filterModel = new CModFilterModel(modModel);
+	filterModel = new CModFilterModel(modModel, this);
 
 	filterModel->setFilterKeyColumn(-1); // filter across all columns
 	filterModel->setSortCaseSensitivity(Qt::CaseInsensitive); // to make it more user-friendly

launcher/modManager/cmodlistview_moc.h

@@ -29,7 +29,7 @@ class CModListView : public QWidget
 {
 	Q_OBJECT
 
-	CModManager * manager;
+	std::unique_ptr<CModManager> manager;
 	CModListModel * modModel;
 	CModFilterModel * filterModel;
 	CDownloadManager * dlManager;

lib/CHeroHandler.cpp

@@ -718,8 +718,10 @@ void CHeroHandler::loadExperience()
 	expPerLevel.push_back(34140);
 	while (expPerLevel[expPerLevel.size() - 1] > expPerLevel[expPerLevel.size() - 2])
 	{
-		int i = expPerLevel.size() - 1;
-		expPerLevel.push_back (expPerLevel[i] + (expPerLevel[i] - expPerLevel[i-1]) * 1.2);
+		auto i = expPerLevel.size() - 1;
+		auto diff = expPerLevel[i] - expPerLevel[i-1];
+		diff += diff / 5;
+		expPerLevel.push_back (expPerLevel[i] + diff);
 	}
 	expPerLevel.pop_back();//last value is broken
 }

lib/battle/BattleInfo.cpp

@@ -105,15 +105,15 @@ namespace CGH
 //RNG that works like H3 one
 struct RandGen
 {
-	int seed;
+	ui32 seed;
 
-	void srand(int s)
+	void srand(ui32 s)
 	{
 		seed = s;
 	}
 	void srand(int3 pos)
 	{
-		srand(110291 * pos.x + 167801 * pos.y + 81569);
+		srand(110291 * ui32(pos.x) + 167801 * ui32(pos.y) + 81569);
 	}
 	int rand()
 	{

lib/filesystem/Filesystem.cpp

@@ -22,6 +22,7 @@
 #include "../CStopWatch.h"
 
 std::map<std::string, ISimpleResourceLoader*> CResourceHandler::knownLoaders = std::map<std::string, ISimpleResourceLoader*>();
+CResourceHandler CResourceHandler::globalResourceHandler;
 
 CFilesystemGenerator::CFilesystemGenerator(std::string prefix):
 	filesystem(new CFilesystemList()),
@@ -117,11 +118,6 @@ void CFilesystemGenerator::loadJsonMap(const std::string &mountPoint, const Json
 	}
 }
 
-void CResourceHandler::clear()
-{
-	delete knownLoaders["root"];
-}
-
 ISimpleResourceLoader * CResourceHandler::createInitial()
 {
 	//temporary filesystem that will be used to initialize main one.
@@ -174,7 +170,8 @@ void CResourceHandler::initialize()
 	//    |-saves
 	//    |-config
 
-	knownLoaders["root"] = new CFilesystemList();
+	globalResourceHandler.rootLoader = vstd::make_unique<CFilesystemList>();
+	knownLoaders["root"] = globalResourceHandler.rootLoader.get();
 	knownLoaders["saves"] = new CFilesystemLoader("SAVES/", VCMIDirs::get().userSavePath());
 	knownLoaders["config"] = new CFilesystemLoader("CONFIG/", VCMIDirs::get().userConfigPath());
 

lib/filesystem/Filesystem.h

@@ -75,13 +75,6 @@ public:
 	 */
 	static void initialize();
 
-	/**
-	 * Semi-debug method to track all possible cases of memory leaks
-	 * Used before exiting application
-	 *
-	 */
-	static void clear();
-
 	/**
 	 * Will load all filesystem data from Json data at this path (normally - config/filesystem.json)
 	 * @param fsConfigURI - URI from which data will be loaded
@@ -103,7 +96,12 @@ public:
 	 */
 	static ISimpleResourceLoader * createFileSystem(const std::string &prefix, const JsonNode & fsConfig);
 
+	~CResourceHandler() = default;
 private:
 	/** Instance of resource loader */
 	static std::map<std::string, ISimpleResourceLoader*> knownLoaders;
+	static CResourceHandler globalResourceHandler;
+
+	CResourceHandler() {};
+	std::unique_ptr<ISimpleResourceLoader> rootLoader;
 };

lib/mapObjects/MiscObjects.cpp

@@ -544,17 +544,15 @@ int CGCreature::getNumberOfStacks(const CGHeroInstance *hero) const
 	else
 		split = 2;
 
-	int a = 1550811371;
-	int b = -935900487;
-	int c = 1943276003;
-	int d = -1120346418;
+	ui32 a = 1550811371u;
+	ui32 b = 3359066809u;
+	ui32 c = 1943276003u;
+	ui32 d = 3174620878u;
 
-	int R1 = a * pos.x + b * pos.y + c * pos.z + d;
-	int R2 = R1 / 65536;
-	int R3 = R2 % 32768;
-	if (R3 < 0)
-		R3 += 32767; //is it ever needed if we do modulo calculus?
-	int R4 = R3 % 100 + 1;
+	ui32 R1 = a * ui32(pos.x) + b * ui32(pos.y) + c * ui32(pos.z) + d;
+	ui32 R2 = (R1 >> 16) & 0x7fff;
+
+	int R4 = R2 % 100 + 1;
 
 	if (R4 <= 20)
 		split -= 1;

lib/serializer/Connection.cpp

@@ -58,7 +58,7 @@ void CConnection::init()
 }
 
 CConnection::CConnection(std::string host, ui16 port, std::string Name, std::string UUID)
-	: iser(this), oser(this), io_service(std::make_shared<asio::io_service>()), connectionID(0), name(Name), uuid(UUID)
+	: io_service(std::make_shared<asio::io_service>()), iser(this), oser(this), name(Name), uuid(UUID), connectionID(0)
 {
 	int i;
 	boost::system::error_code error = asio::error::host_not_found;
@@ -111,12 +111,12 @@ connerror1:
 	throw std::runtime_error("Can't establish connection :(");
 }
 CConnection::CConnection(std::shared_ptr<TSocket> Socket, std::string Name, std::string UUID)
-	: iser(this), oser(this), socket(Socket), io_service(&Socket->get_io_service()), connectionID(0), name(Name), uuid(UUID)
+	: iser(this), oser(this), socket(Socket), name(Name), uuid(UUID), connectionID(0)
 {
 	init();
 }
-CConnection::CConnection(std::shared_ptr<TAcceptor> acceptor, std::shared_ptr<boost::asio::io_service> Io_service, std::string Name, std::string UUID)
-	: iser(this), oser(this), connectionID(0), name(Name), uuid(UUID)
+CConnection::CConnection(std::shared_ptr<TAcceptor> acceptor, std::shared_ptr<boost::asio::io_service> io_service, std::string Name, std::string UUID)
+	: io_service(io_service), iser(this), oser(this), name(Name), uuid(UUID), connectionID(0)
 {
 	boost::system::error_code error = asio::error::host_not_found;
 	socket = std::make_shared<tcp::socket>(*io_service);

lib/serializer/Connection.h

@@ -49,13 +49,13 @@ typedef boost::asio::basic_socket_acceptor<boost::asio::ip::tcp, boost::asio::so
 class DLL_LINKAGE CConnection
 	: public IBinaryReader, public IBinaryWriter, public std::enable_shared_from_this<CConnection>
 {
-	CConnection();
-
 	void init();
 	void reportState(vstd::CLoggerBase * out) override;
 
 	int write(const void * data, unsigned size) override;
 	int read(void * data, unsigned size) override;
+
+	std::shared_ptr<boost::asio::io_service> io_service; //can be empty if connection made from socket
 public:
 	BinaryDeserializer iser;
 	BinarySerializer oser;
@@ -66,7 +66,6 @@ public:
 	bool connected;
 	bool myEndianess, contactEndianess; //true if little endian, if endianness is different we'll have to revert received multi-byte vars
 	std::string contactUuid;
-	std::shared_ptr<boost::asio::io_service> io_service;
 	std::string name; //who uses this connection
 	std::string uuid;
 

server/CVCMIServer.cpp

@@ -41,6 +41,7 @@
 #include "../lib/logging/CBasicLogConfigurator.h"
 #include "../lib/CConfigHandler.h"
 #include "../lib/ScopeGuard.h"
+#include "../lib/serializer/CMemorySerializer.h"
 
 #include "../lib/UnlockGuard.h"
 
@@ -145,18 +146,17 @@ CVCMIServer::CVCMIServer(boost::program_options::variables_map & opts)
 
 CVCMIServer::~CVCMIServer()
 {
-
-	for(CPackForLobby * pack : announceQueue)
-		delete pack;
-
 	announceQueue.clear();
+
+	if(announceLobbyThread)
+		announceLobbyThread->join();
 }
 
 void CVCMIServer::run()
 {
 	if(!restartGameplay)
 	{
-		boost::thread(&CVCMIServer::threadAnnounceLobby, this);
+		this->announceLobbyThread = vstd::make_unique<boost::thread>(&CVCMIServer::threadAnnounceLobby, this);
 #ifndef VCMI_ANDROID
 		if(cmdLineOptions.count("enable-shm"))
 		{
@@ -195,7 +195,7 @@ void CVCMIServer::threadAnnounceLobby()
 			boost::unique_lock<boost::recursive_mutex> myLock(mx);
 			while(!announceQueue.empty())
 			{
-				announcePack(announceQueue.front());
+				announcePack(std::move(announceQueue.front()));
 				announceQueue.pop_front();
 			}
 			if(state != EServerState::LOBBY)
@@ -224,7 +224,7 @@ void CVCMIServer::prepareToStartGame()
 		// FIXME: dirry hack to make sure old CGameHandler::run is finished
 		boost::this_thread::sleep(boost::posix_time::milliseconds(1000));
 	}
-
+	state = EServerState::GAMEPLAY_STARTING;
 	gh = std::make_shared<CGameHandler>(this);
 	switch(si->mode)
 	{
@@ -306,7 +306,7 @@ void CVCMIServer::threadHandleClient(std::shared_ptr<CConnection> c)
 			CPack * pack = c->retrievePack();
 			if(auto lobbyPack = dynamic_ptr_cast<CPackForLobby>(pack))
 			{
-				handleReceivedPack(lobbyPack);
+				handleReceivedPack(std::unique_ptr<CPackForLobby>(lobbyPack));
 			}
 			else if(auto serverPack = dynamic_ptr_cast<CPackForServer>(pack))
 			{
@@ -333,56 +333,53 @@ void CVCMIServer::threadHandleClient(std::shared_ptr<CConnection> c)
 
 	boost::unique_lock<boost::recursive_mutex> queueLock(mx);
 //	if(state != ENDING_AND_STARTING_GAME)
+	if(c->connected)
 	{
-		auto lcd = new LobbyClientDisconnected();
+		auto lcd = vstd::make_unique<LobbyClientDisconnected>();
 		lcd->c = c;
 		lcd->clientId = c->connectionID;
-		handleReceivedPack(lcd);
+		handleReceivedPack(std::move(lcd));
 	}
 
 	logNetwork->info("Thread listening for %s ended", c->toString());
 	c->handler.reset();
 }
 
-void CVCMIServer::handleReceivedPack(CPackForLobby * pack)
+void CVCMIServer::handleReceivedPack(std::unique_ptr<CPackForLobby> pack)
 {
-	CBaseForServerApply * apply = applier->getApplier(typeList.getTypeID(pack));
-	if(apply->applyOnServerBefore(this, pack))
-		addToAnnounceQueue(pack);
-	else
-		delete pack;
+	CBaseForServerApply * apply = applier->getApplier(typeList.getTypeID(pack.get()));
+	if(apply->applyOnServerBefore(this, pack.get()))
+		addToAnnounceQueue(std::move(pack));
 }
 
-void CVCMIServer::announcePack(CPackForLobby * pack)
+void CVCMIServer::announcePack(std::unique_ptr<CPackForLobby> pack)
 {
 	for(auto c : connections)
 	{
 		// FIXME: we need to avoid senting something to client that not yet get answer for LobbyClientConnected
 		// Until UUID set we only pass LobbyClientConnected to this client
-		if(c->uuid == uuid && !dynamic_cast<LobbyClientConnected *>(pack))
+		if(c->uuid == uuid && !dynamic_cast<LobbyClientConnected *>(pack.get()))
 			continue;
 
-		c->sendPack(pack);
+		c->sendPack(pack.get());
 	}
 
-	applier->getApplier(typeList.getTypeID(pack))->applyOnServerAfter(this, pack);
-
-	delete pack;
+	applier->getApplier(typeList.getTypeID(pack.get()))->applyOnServerAfter(this, pack.get());
 }
 
 void CVCMIServer::announceTxt(const std::string & txt, const std::string & playerName)
 {
 	logNetwork->info("%s says: %s", playerName, txt);
-	auto cm = new LobbyChatMessage();
+	auto cm = vstd::make_unique<LobbyChatMessage>();
 	cm->playerName = playerName;
 	cm->message = txt;
-	addToAnnounceQueue(cm);
+	addToAnnounceQueue(std::move(cm));
 }
 
-void CVCMIServer::addToAnnounceQueue(CPackForLobby * pack)
+void CVCMIServer::addToAnnounceQueue(std::unique_ptr<CPackForLobby> pack)
 {
 	boost::unique_lock<boost::recursive_mutex> queueLock(mx);
-	announceQueue.push_back(pack);
+	announceQueue.push_back(std::move(pack));
 }
 
 bool CVCMIServer::passHost(int toConnectionId)
@@ -479,7 +476,7 @@ void CVCMIServer::updateStartInfoOnMapChange(std::shared_ptr<CMapInfo> mapInfo,
 	si->playerInfos.clear();
 	if(mi->scenarioOptionsOfSave)
 	{
-		si = std::shared_ptr<StartInfo>(mi->scenarioOptionsOfSave);
+		si = CMemorySerializer::deepCopy(*mi->scenarioOptionsOfSave);
 		si->mode = StartInfo::LOAD_GAME;
 		if(si->campState)
 			campaignMap = si->campState->currentMap.get();
@@ -563,9 +560,9 @@ void CVCMIServer::updateAndPropagateLobbyState()
 		}
 	}
 
-	auto lus = new LobbyUpdateState();
+	auto lus = vstd::make_unique<LobbyUpdateState>();
 	lus->state = *this;
-	addToAnnounceQueue(lus);
+	addToAnnounceQueue(std::move(lus));
 }
 
 void CVCMIServer::setPlayer(PlayerColor clickedColor)
@@ -945,7 +942,6 @@ int main(int argc, char * argv[])
 	envHelper.callStaticVoidMethod(CAndroidVMHelper::NATIVE_METHODS_DEFAULT_CLASS, "killServer");
 #endif
 	vstd::clear_pointer(VLC);
-	CResourceHandler::clear();
 	return 0;
 }
 

server/CVCMIServer.h

@@ -44,9 +44,10 @@ class CVCMIServer : public LobbyInfo
 	std::shared_ptr<boost::asio::io_service> io;
 	std::shared_ptr<TAcceptor> acceptor;
 	std::shared_ptr<TSocket> upcomingConnection;
-	std::list<CPackForLobby *> announceQueue;
+	std::list<std::unique_ptr<CPackForLobby>> announceQueue;
 	boost::recursive_mutex mx;
 	std::shared_ptr<CApplier<CBaseForServerApply>> applier;
+	std::unique_ptr<boost::thread> announceLobbyThread;
 
 public:
 	std::shared_ptr<CGameHandler> gh;
@@ -69,13 +70,13 @@ public:
 	void connectionAccepted(const boost::system::error_code & ec);
 	void threadHandleClient(std::shared_ptr<CConnection> c);
 	void threadAnnounceLobby();
-	void handleReceivedPack(CPackForLobby * pack);
+	void handleReceivedPack(std::unique_ptr<CPackForLobby> pack);
 
-	void announcePack(CPackForLobby * pack);
+	void announcePack(std::unique_ptr<CPackForLobby> pack);
 	bool passHost(int toConnectionId);
 
 	void announceTxt(const std::string & txt, const std::string & playerName = "system");
-	void addToAnnounceQueue(CPackForLobby * pack);
+	void addToAnnounceQueue(std::unique_ptr<CPackForLobby> pack);
 
 	void setPlayerConnectedId(PlayerSettings & pset, ui8 player) const;
 	void updateStartInfoOnMapChange(std::shared_ptr<CMapInfo> mapInfo, std::shared_ptr<CMapGenOptions> mapGenOpt = {});

server/NetPacksLobbyServer.cpp

@@ -75,6 +75,8 @@ bool LobbyClientDisconnected::checkClientPermissions(CVCMIServer * srv) const
 bool LobbyClientDisconnected::applyOnServer(CVCMIServer * srv)
 {
 	srv->clientDisconnected(c);
+	c->close();
+	c->connected = false;
 	return true;
 }
 
@@ -100,10 +102,10 @@ void LobbyClientDisconnected::applyOnServerAfterAnnounce(CVCMIServer * srv)
 	}
 	else if(c == srv->hostClient)
 	{
-		auto ph = new LobbyChangeHost();
+		auto ph = vstd::make_unique<LobbyChangeHost>();
 		auto newHost = *RandomGeneratorUtil::nextItem(srv->connections, CRandomGenerator::getDefault());
 		ph->newHostConnectionId = newHost->connectionID;
-		srv->addToAnnounceQueue(ph);
+		srv->addToAnnounceQueue(std::move(ph));
 	}
 	srv->updateAndPropagateLobbyState();
 }
@@ -175,7 +177,6 @@ bool LobbyStartGame::applyOnServer(CVCMIServer * srv)
 		return false;
 	}
 	// Server will prepare gamestate and we announce StartInfo to clients
-	srv->state = EServerState::GAMEPLAY_STARTING;
 	srv->prepareToStartGame();
 	initializedStartInfo = std::make_shared<StartInfo>(*srv->gh->getStartInfo(true));
 	return true;