fix Quest States Core Crash Item Dupe Bug Fix

[martysama0134 6947]

This vulnerability should affect every server. You can duplicate item rewards, and also crash the server through dangling pointers.

The danger of this bug escalates to how many custom systems, and how many crafting quests (for example, the vitality ore quest, not the cube system) you have in your server.

How to trigger it:

Any quest that uses select & wait, and the item lua module after that is vulnerable.

After the server uses select() or wait(), the player's quest state is suspended. After the player replies using the CG packet, the quest state is recovered.

So what's wrong with it? It doesn't verify if the stored quest item ptr expired.

You basically need to destroy the selected item ptr in order to dupe the rewards of the quest. After some tries, you may get a core crash in the game. (dangling pointers often cause crashes only after that memory sector has been rewritten)

In my files, I've checked (since several years ago) if the quest state was suspended for the the default windows such as exchange, cube, shop.

This bug can work very easily on offline shops or other new systems that don't check that.

After the select() or wait() is called, you send the selected item to the (e.g.) offlineshop system window. It will delete the item ptr in the game. Now, you can press "Ok" on the quest, and the quest will proceed as if the item still existed.

The item still exists in the offlineshop, but not the item ptr anymore. The item won't be deleted by the quest even after item.remove() is called.

This is the fix:

diff --git a/s3ll_server/Srcs/Server/game/src/char.cpp b/s3ll_server/Srcs/Server/game/src/char.cpp
index 0ea307fa..65b1dd65 100644
--- a/s3ll_server/Srcs/Server/game/src/char.cpp
+++ b/s3ll_server/Srcs/Server/game/src/char.cpp
@@ -303,7 +303,10 @@ void CHARACTER::Initialize()

 	m_dwQuestNPCVID = 0;
 	m_dwQuestByVnum = 0;
-	m_pQuestItem = NULL;
+	m_dwQuestItemVID = 0;

 	m_dwUnderGuildWarInfoMessageTime = get_dword_time()-60000;

@@ -6123,33 +6126,37 @@ LPCHARACTER CHARACTER::GetQuestNPC() const

 void CHARACTER::SetQuestItemPtr(LPITEM item)
 {
-	m_pQuestItem = item;
+	m_dwQuestItemVID = (item) ? item->GetVID() : 0;
 }

 void CHARACTER::ClearQuestItemPtr()
 {
-	m_pQuestItem = NULL;
+	m_dwQuestItemVID = 0;
 }

 LPITEM CHARACTER::GetQuestItemPtr() const
 {
-	return m_pQuestItem;
+	if (!m_dwQuestItemVID)
+		return nullptr;
+	return ITEM_MANAGER::Instance().FindByVID(m_dwQuestItemVID);
 }

diff --git a/s3ll_server/Srcs/Server/game/src/char.h b/s3ll_server/Srcs/Server/game/src/char.h
index cc4da2bb..74b3470e 100644
--- a/s3ll_server/Srcs/Server/game/src/char.h
+++ b/s3ll_server/Srcs/Server/game/src/char.h
@@ -1674,9 +1674,9 @@ class CHARACTER : public CEntity, public CFSM, public CHorseRider
 	private:
 		DWORD				m_dwQuestNPCVID;
 		DWORD				m_dwQuestByVnum;
-		LPITEM				m_pQuestItem;
+		DWORD				m_dwQuestItemVID{}; // @fixme304 (LPITEM -> DWORD)

 		// Events

To unlock the client in order to move your windows with a quest open, you edit this from interfacemodule.py:

You can even Hide the TopBar and BottomBar from uiQuest.py for a better result.

 

Important: after this fix, the item ptr may be nullptr after they press enter, so you need to check if the item ptr is still valid by using this function:

	ALUA(item_is_available)
	{
		auto item = CQuestManager::instance().GetCurrentItem();
		lua_pushboolean(L, item != nullptr);
		return 1;
	}

...

			{ "is_available",		item_is_available	},	// [return lua boolean]

By simply doing:

when 169.take begin
    local s1=select("Yes", "No")
    if s1==2 then return end
    if not item.is_available() then return end
end

 

If you want to tryhard, and be sure that the item ptr didn't get swapped, you can do as following via quest:

Edited September 23, 2022 by Metin2 Dev
Core X - External 2 Internal

[Helia01 2055]

ty, maestro!

[Mitachi 1319]

Thanks for your constant contribution

[Braxy 590]

I found this exploit a long time ago, but i never searched why is this happening. My take on it, was to use the pc.remove_item and check if the item is still there.
Basically i "avoided" using item.remove and returned if item was not in the inventory. (trash method now but whatever)

Anyway, the way you did it is the right way to solve it, thanks marty

[Owsap 7722]

I remember I did this bug on the official server with the Marriage ring, all I had to do was make my connection slow, open the trading window with another character, drag the window right beneath the quest button to advance and when the connection was slow it would take time to show the quest dialog but the trading window was open and I could click accept on both characters, this resulting in the quest to not find the item needed and duplicating it.
By the way, thanks for share.

[Mali 39474]

Spoiler

 

 

Edited September 24, 2022 by Metin2 Dev
Core X - External 2 Internal

[MrQuin 6049]

I remember doing this exploit in real life.
Thanks for sharing.

[SukH 856]

thanks for sharing  , you sleeping men

[imWelly 26]

a really great release ,

this problem\bug was around for literally forever . 

i was avoiding it by using check count which ofc is not efficient at all 

[Deso 702]

We could also add 

			if (!item)
				return false;

			LPCHARACTER pkChrCauser = CHARACTER_MANAGER::instance().FindByPID(pc);
			if (pkChrCauser)
			{
				if (!pkChrCauser->CanHandleItem())
				{
					pkChrCauser->ChatPacket(7, "You cannot open this conversation, while other windows are opened.");
					return false;
				}
			}

 

before 

return m_mapNPC[npc].OnTakeItem(*pPC);

 

in questmanager.cpp

 

This would make the quest function smaller. 

Edited October 18, 2022 by Deso

[BadRomani 14]

how can you put objects in offline store with task window open? You are cheating

You do not use any windows while the task window is open. Or I misunderstood.

[martysama0134 6947]

8 hours ago, BadRomani said:

how can you put objects in offline store with task window open? You are cheating

You do not use any windows while the task window is open. Or I misunderstood.

By task you mean "quest"? Anyway, a simple bot, or just lag can trigger this bug.

[Jira 444]

item drop penalty

 source/server/game/char_battle.cpp | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/source/server/game/char_battle.cpp b/source/server/game/char_battle.cpp
index 4b0c013..9f9fab6 100644
--- a/source/server/game/char_battle.cpp
+++ b/source/server/game/char_battle.cpp
@@ -1107,8 +1107,18 @@ void CHARACTER::ItemDropPenalty(LPCHARACTER pkKiller)
 		std::vector<BYTE> vec_bSlots;
 
 		for (i = 0; i < INVENTORY_MAX_NUM; ++i)
-			if (GetInventoryItem(i))
+		{
+			if ((pkItem = GetInventoryItem(i)))
+			{
+				if (pkItem->GetType() == ITEM_QUEST)
+				{
+					if (quest::CQuestManager::instance().GetPCForce(GetPlayerID())->IsRunning() == true)
+						continue;
+				}
+
 				vec_bSlots.push_back(i);
+			}
+		}
 
 		if (!vec_bSlots.empty())
 		{

 

[martysama0134 6947]

4 hours ago, Jira said:

item drop penalty

 source/server/game/char_battle.cpp | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/source/server/game/char_battle.cpp b/source/server/game/char_battle.cpp
index 4b0c013..9f9fab6 100644
--- a/source/server/game/char_battle.cpp
+++ b/source/server/game/char_battle.cpp
@@ -1107,8 +1107,18 @@ void CHARACTER::ItemDropPenalty(LPCHARACTER pkKiller)
 		std::vector<BYTE> vec_bSlots;
 
 		for (i = 0; i < INVENTORY_MAX_NUM; ++i)
-			if (GetInventoryItem(i))
+		{
+			if ((pkItem = GetInventoryItem(i)))
+			{
+				if (pkItem->GetType() == ITEM_QUEST)
+				{
+					if (quest::CQuestManager::instance().GetPCForce(GetPlayerID())->IsRunning() == true)
+						continue;
+				}
+
 				vec_bSlots.push_back(i);
+			}
+		}
 
 		if (!vec_bSlots.empty())
 		{

 

The game is single-thread, so I highly recommend you to add the if (quest::CQuestManager::instance().GetPCForce(GetPlayerID())->IsRunning()) check before the for loop.

It will be either always true, or always false.

Edited August 2, 2023 by martysama0134

[Mitachi 1319]

9 hours ago, martysama0134 said:

The game is single-thread, so I highly recommend you to add the if (quest::CQuestManager::instance().GetPCForce(GetPlayerID())->IsRunning()) check before the for loop.

It will be either always true, or always false.

Something like:

	if (bDropInventory) // Drop Inventory
	{
		const bool bIsQuestRunning = IsQuestRunning();
		std::vector<BYTE> vec_bSlots{};

		for (int j = 0; j < INVENTORY_MAX_NUM; ++j)
		{
			pkItem = GetInventoryItem(j);
			if (pkItem && !(bIsQuestRunning && pkItem->GetType() == ITEM_QUEST))
				vec_bSlots.emplace_back(j);
		}

		if (!vec_bSlots.empty())

People can somehow bug abuse the quest items, and throw them to the ground while using item.remove() or pc.item_remove()

 

::IsQuestRunning it's simply a function that returns the above expression, quest::CQuestManager::instance().GetPCForce(GetPlayerID())->IsRunning()

#ifdef ENABLE_MISCELLANEOUS
bool CHARACTER::IsQuestRunning() const
{
	return quest::CQuestManager::instance().GetPCForce(GetPlayerID())->IsRunning();
}
#endif