Jump to content

Core crash when cancelling server timers


Sonitex

Recommended Posts

  • Premium

When cancelling server timers a core will crash as it is removing a timer from the map and increasing the iterator twice by calling erase() function and afterwards increasing it once again when entering a new cycle of loop. One way of solving this issue is to first collect the timers which must be removed and clean them up at the end. You can also add a simple counter which is increased at each end of the loop's cycle and remove the timer directly from the first loop by specifying the position with the counter itself.

 

Note that this issue seems to appear after upgrading code to C++11 or higher.

 

Replace the following function inside questmanager.cpp:

	void CQuestManager::CancelServerTimers(DWORD arg)
	{
		vector<pair<string, DWORD>> ServerTimersToDelete;

		for (const auto& kv : m_mapServerTimer) {
			if (kv.first.second == arg) {
				LPEVENT event = kv.second;
				event_cancel(&event);
				ServerTimersToDelete.push_back(kv.first);
			}
		}

		// Delete all the required server timers
		for (const auto &timer : ServerTimersToDelete)
			m_mapServerTimer.erase(timer);

		// Clean up
		ServerTimersToDelete.clear();
	}

 

Edited by Sonitex
  • Metin2 Dev 1
  • Not Good 1
  • Love 8
Link to comment
Share on other sites

  • Honorable Member

thanks for fix :)

For me, I wouldn't prefer this. Because you are copying pair. Just use simple iterator like this:

for ( ; it != container.end(); ) {
	if (condition)
		it = container.erase(it);
	else
		++it;
}

 

  • Angry 1
  • Love 2

 

Link to comment
Share on other sites

  • Premium
17 minutes ago, Mali61 said:

thanks for fix :)

For me, I wouldn't prefer this. Because you are copying pair. Just use simple iterator like this:


for ( ; it != container.end(); ) {
	if (condition)
		it = container.erase(it);
	else
		++it;
}

 

 

That is the other way I was talking about and you are right, this is much more efficient way of doing it as we are not copying elements to another vector and cleaning them up afterwards. Thank you for the code ;)

  • Love 1
Link to comment
Share on other sites

  • 1 year later...

this is the best fix if someone need it

	void CQuestManager::CancelServerTimers(DWORD arg)
	{
		itertype(m_mapServerTimer) it = m_mapServerTimer.begin();
   
		for (; it != m_mapServerTimer.end(); )
		{
			if (it->first.second == arg)
			{
				LPEVENT event = it->second;
				event_cancel(&event);
				m_mapServerTimer.erase(it++);
			}
			else
				++it;
		}
	}

 

  • Love 1
Link to comment
Share on other sites

  • 10 months later...
  • Honorable Member

I know this is an old thread. This should solve it:

On 12/20/2020 at 4:47 PM, Mali said:

thanks for fix :)

For me, I wouldn't prefer this. Because you are copying pair. Just use simple iterator like this:

for ( ; it != container.end(); ) {
	if (condition)
		it = container.erase(it);
	else
		++it;
}

 

The return value of .erase must be stored in the it again.

To be more precise:


	void CQuestManager::CancelServerTimers(DWORD arg)
	{
		auto it = m_mapServerTimer.begin();
		while (it != m_mapServerTimer.end()) {
			if (it->first.second == arg) {
				event_cancel(&it->second);
				it = m_mapServerTimer.erase(it);
				continue;
			}
			++it;
		}
	}

if you're edgy:

	void CQuestManager::CancelServerTimers(DWORD arg)
	{
		for (auto it = m_mapServerTimer.begin(); it != m_mapServerTimer.end();) {
			if (it->first.second == arg) {
				event_cancel(&it->second);
				it = m_mapServerTimer.erase(it);
			} else
				++it;
		}
	}

or in c++20

void CancelServerTimers(uint32_t arg) {
    auto erase_check = [&](auto&& it) {
        if (it.first.second == arg) {
            auto event = it.second; // copy ptr
            event_cancel(&event);
        }
        return it.first.second == arg;
    };
    std::erase_if(m_mapServerTimer, erase_check);
}

https://godbolt.org/z/EEa5Yz7Mn

 

Edited by martysama0134
  • Metin2 Dev 3
  • Good 2
  • Love 4
Link to comment
Share on other sites

  • 3 months later...
  • Premium

It also has a memory leak

The name of the timer is never deallocated when CancelServerTimers or ClearServerTimer is called

Spoiler
diff --git a/game/src/questevent.cpp b/game/src/questevent.cpp
index 21fa0ed5b5a3fa3d0572494137602c2ee5cbc2c4..2a6cf3f73c7732d4d67c0bf044d510f1225ceaa8 100644
--- a/game/src/questevent.cpp
+++ b/game/src/questevent.cpp
@@ -20,6 +20,19 @@ namespace quest
 		event_cancel(ppEvent);
 	}
 
+	void CancelServerTimerEvent(LPEVENT* ppEvent)
+	{
+		quest_server_event_info* info = dynamic_cast<quest_server_event_info*>((*ppEvent)->info);
+
+		if (info)
+		{
+			delete [] (info->name);
+			info->name = NULL;
+		}
+
+		event_cancel(ppEvent);
+	}
+
 	EVENTFUNC(quest_server_timer_event)
 	{
 		quest_server_event_info * info = dynamic_cast<quest_server_event_info *>( event->info );
diff --git a/game/src/questevent.h b/game/src/questevent.h
index 2be83a0106e2f01cc8d1f50e7f2779957842c176..76500b52fd10d8f22965920f1a9d234b91b7ead3 100644
--- a/game/src/questevent.h
+++ b/game/src/questevent.h
@@ -37,4 +37,5 @@ namespace quest
 	extern LPEVENT quest_create_server_timer_event(const char* name, double when, unsigned int timernpc = QUEST_NO_NPC, bool loop = false, unsigned int arg = 0);
 	extern LPEVENT quest_create_timer_event(const char* name, unsigned int player_id, double when, unsigned int npc_id=QUEST_NO_NPC, bool loop = false);
 	extern void CancelTimerEvent(LPEVENT* ppEvent);
-}
\ No newline at end of file
+	extern void CancelServerTimerEvent(LPEVENT* ppEvent);
+}
diff --git a/game/src/questmanager.cpp b/game/src/questmanager.cpp
index 2e58b776c4c3af97d2d0654d227d239ff177bf32..c2ed5eca1352f79f87003f7382b82db8f2682fd3 100644
--- a/game/src/questmanager.cpp
+++ b/game/src/questmanager.cpp
@@ -21,6 +21,7 @@
 #include "dungeon.h"
 #include "regen.h"
+#include "questevent.h"
@@ -1853,7 +1854,7 @@ namespace quest
 		if (it != m_mapServerTimer.end())
 		{
 			LPEVENT event = it->second;
-			event_cancel(&event);
+			CancelServerTimerEvent(&event);
 
 			m_mapServerTimer.erase(it);
 		}
@@ -1864,7 +1865,7 @@ namespace quest
 		auto it = m_mapServerTimer.begin();
 		while (it != m_mapServerTimer.end()) {
 			if (it->first.second == arg) {
-				event_cancel(&it->second);
+				CancelServerTimerEvent(&it->second);
 				it = m_mapServerTimer.erase(it);
 				continue;
 			}

 

 

  • Metin2 Dev 1
Link to comment
Share on other sites

Announcements



×
×
  • Create New...

Important Information

Terms of Use / Privacy Policy / Guidelines / We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.