COMMITTED: Augmentation Inventory Container - Fix lag lore item deletion defect.

[l0stmancd]

There is an issue with the augmentation inventory container where a small amount of lag between when the items are pushed onto the cursor and the old-items are deleted from the inventory could cause the client to delete the inventory item causing lore item collision. This will cause the new item that should have been pushed onto your cursor to disappear until you zone or camp.

The code change is minor - it will keep track of the items that you were pushing onto the cursor and instead hold these until the item deletion has occurred.

The only interesting part of the code is in the ->Clone() call that had to be added. If we do not add this then we would be trying to push down an item to the client that was already deleted in the above section - causes obvious issues.

Testing the following with SoD and Titanium:
Augments can be removed from an item and both augment/item are pushed onto cursor.
Augments can be put onto an item and the new item is pushed onto the cursor.
No older augments are modified / removed as part of this unless they were chosen to be removed.

Tested with the following item ids:
82363
69312
60332
47013

Index: tradeskills.cpp
===================================================================
--- tradeskills.cpp	(revision 1806)
+++ tradeskills.cpp	(working copy)
@@ -108,11 +108,13 @@
 
 	bool deleteItems = false;
 
+	ItemInst *itemOneToPush = NULL, *itemTwoToPush = NULL;
+
 	// Adding augment
 	if (in_augment->augment_slot == -1) {
 		if (((slot=tobe_auged->AvailableAugmentSlot(auged_with->GetAugmentType()))!=-1) && (tobe_auged->AvailableWearSlot(auged_with->GetItem()->Slots))) {
 			tobe_auged->PutAugment(slot,*auged_with);
-			user->PushItemOnCursor(*tobe_auged,true);
+			itemOneToPush = tobe_auged->Clone();
 			deleteItems = true;
 		} else {
 			user->Message(13, "Error: No available slot for augment");
@@ -125,9 +127,9 @@
 		else
 			old_aug=tobe_auged->RemoveAugment(in_augment->augment_slot);
 
-		user->PushItemOnCursor(*tobe_auged,true);
+		itemOneToPush = tobe_auged->Clone();
 		if (old_aug)
-			user->PushItemOnCursor(*old_aug,true);
+			itemTwoToPush = old_aug->Clone();
 
 		deleteItems = true;
 	}
@@ -154,6 +156,14 @@
 			container->Clear();
 		}
 	}
+
+	// Must push items after the items in inventory are deleted - necessary due to lore items...
+	if (itemOneToPush) {
+		user->PushItemOnCursor(*itemOneToPush,true);
+	}
+	if (itemTwoToPush) {
+		user->PushItemOnCursor(*itemTwoToPush,true);
+	}
 }
 
 // Perform tradeskill combine

[l0stmancd]

Problem Definition:

What we see happening is that a user would attempt to, for example, deaugment a lore item.

Scenario:
User has SwordA that is Lore with Augment A on it.

User puts SwordA (with AugmentA) into the augmentation sealer.
- At this time, user has a single lore sword in inventory.
---
User puts the augment remover into the augmentation sealer.
- At this time, the user has a single lore sword in inventory.
---
User clicks the remove augment int eh window.
- At this time, the user has a single lore sword in inventory.
--
User confirms that the user wishes to remove the augment
- At this time, the user has a single lore sword in inventory.
--
At this point the server removes the augment and pushes the new sword and the new augment onto the cursor.
- At this time, the user has two lore swords in inventory (one in aug sealer and one on hand).
* Problem Happens Here*
--
For most users, the server would then send a delete item request for the items in the augmentation sealer.
- At this time, the user has a single lore sword in inventory (zero in aug sealer, one in hand).

The issue is that occasionally (uncommon but not rare) the client would get "DUPLICATE LORE ITEM DELETED!" message and then it would look like they only have a single augment on their cursor. The items in the augmentation sealer would, of course, be deleted. It would -appear- at this time that the user has lost the LoreSwordA.

The problem is that the server believes the user still has the lore sword in the top slot of their cursor inventory slots. At this point the client and server have a sync issue. The client can move around what they believe is the augment and the server believes they are moving around the loreSwordA. If the user zones / camps, then they are resynced and the user sees both the loreSwordA as well as the augment.

Reproduction Steps:
On a server augment / deaugment using the inventory augmentation container until you receive the "Duplicate Lore Item Deleted" message. The longest it took for me to receive this was ~20-25 times. I am not able to reproduce this easily on my local server without massively lagging out my virtual machine to try to introduce latency.

Original thought as to the cause was the two items being on the client at a given time causing the server (or client) to believe that lore items should be deleted. Original solution (as posted above) was to bring us into line with tradeskill combines and remove the items first and then add in the new item.

In chatting with Trevius, there seems to be some questions still regarding how this is happening so bringing into the thread to discuss.

[l0stmancd]

Here is the really weird thing that makes me think this is client and not server controlled.

I was debugging into zoneperl.exe and had a trap on CheckLoreConflict call. I made sure it worked when I picked up a batwing off the ground. All good. Then I reproduced this defect.

When I reproduced this defect, the CheckLoreConflict function was not called at all.

Recompiling with this hard coded to "return false;" - my bet is that the client will still delete it because it believes there is a lore conflict.

-- Update --

Yup, this is a pure client issue. Appears to be occurring in SoD for sure. Will test this out in a Titanium Client.

How to reproduce easily.

Hard code CheckLoreConflict to return false.

#summonitem 5407 multiple times (Executioners axe - lore).

Note you can have multiple on you.

Drop one on the ground.

Pick it up and note you will have "Duplicate lore items deleted!" and the items will disappear. Log and come back in and they will still be present.

Testing this out on Titanium.

-- Update --

Titanium has same issue.

This may have been already noted in source via the comment:
" // if there is a lore conflict, delete the offending item from the server inventory
// the client updates itself and takes care of sending "duplicate lore item" messages
"

Seems to imply that the client will remove it from its own inventory and the server just needs to remove it from ours.

Would explain why it is fine when the user zones / camps (as server still has the "duplicate lore items" but client has them removed).

I consider that this is still the proper fix for this issue from the tests above...

[l0stmancd]

Final update from me for now.

On the original code that is in src right now, I was able to reproduce the defect by making Kreljnok's Sword of Eternal Power lore (sql: UPDATE items SET loregroup = -1 WHERE id = 60332 and continuously aug/deaug. Happened on clicks between 0 and 20 times. My guess is the client only checks every so many milliseconds and if the server "Fixes" it fast enough between these points then everything is good.

After the patch (first post) I was unable to reproduce this issue any more. Tested 60 combines without an issue.

[trevius]

Thanks for the extra testing. Sounds like we should be all set to get this added in.

If the client is deleting lore items, it would be nice to know which one it will delete (the existing/old one(s) or the newly acquired one). Either way, it sounds like we just need to make sure that the scenario of duplicate lore items never happens such as this case. Makes me wonder if there may be other cases of this out there as well. I haven't seen any zone crashes in relation to augmenting items in quite a while, but I have definitely had reports of item loss while augmenting. This fix sounds like it should resolve the issue with the new inventory based aug sealer, but we may still have more aug code changes needed to really remove the possibility of item loss during augmentation.

I noticed a few things in the augmenting code that could probably use some rewriting. Some of it stood out as potential issues, but it will take another closer look to make sure changes are required. Maybe I will do a rewrite of some of it when I have some time.

[trevius]

Ok, here is my rewrite of the function, including the changes from l0stmancd. I highlighted the section I changed in green (can't make a normal diff atm).

tradeskills.cpp

void Object::HandleAugmentation(Client* user, const AugmentItem_Struct* in_augment, Object *worldo)
{
	if (!user || !in_augment)
	{
		LogFile->write(EQEMuLog::Error, "Client or AugmentItem_Struct not set in Object::HandleAugmentation");
		return;
	}

	ItemInst* container = NULL;

	if (worldo)
	{
		container = worldo->m_inst;
	} 
	else
	{
		// Check to see if they have an inventory container type 53 that is used for this.
		Inventory& user_inv = user->GetInv();
		ItemInst* inst = NULL;
		
		inst = user_inv.GetItem(in_augment->container_slot);
		if (inst)
		{
			const Item_Struct* item = inst->GetItem();
			if (item && inst->IsType(ItemClassContainer) && item->BagType == 53)
			{
				// We have found an appropriate inventory augmentation sealer
				container = inst;

				// Verify that no more than two items are in container to guarantee no inadvertant wipes.
				uint8 itemsFound = 0;
				for (uint8 i=0; i<10; i++)
				{
					const ItemInst* inst = container->GetItem(i);
					if (inst)
					{
						itemsFound++;
					}
				}

				if (itemsFound != 2)
				{
					user->Message(13, "Error:  Too many/few items in augmentation container.");
					return;
				}
			}
		}
	}

	if(!container)
	{
		LogFile->write(EQEMuLog::Error, "Player tried to augment an item without a container set.");
		user->Message(13, "Error: This item is not a container!");
		return;
	}
	
	ItemInst *tobe_auged, *auged_with = NULL;
	sint8 slot=-1;

	// Verify 2 items in the augmentation device
	if (container->GetItem(0) && container->GetItem(1))
	{
		// Verify 1 item is augmentable and the other is not
		if (container->GetItem(0)->IsAugmentable() && !container->GetItem(1)->IsAugmentable())
		{
			tobe_auged = container->GetItem(0);
			auged_with = container->GetItem(1);
		}
		else if (!container->GetItem(0)->IsAugmentable() && container->GetItem(1)->IsAugmentable())
		{
			tobe_auged = container->GetItem(1);
			auged_with = container->GetItem(0);
		}
		else
		{
			// Either 2 augmentable items found or none found
			// This should never occur due to client restrictions, but prevent in case of a hack
			user->Message(13, "Error: Must be 1 augmentable item in the sealer");
			// May want to user->Kick() them to prevent potential inventory sync issues if it happened legitimately
			return;
		}
	}
	else
	{
		// Less than 2 items in the augmentation device
		// This should never occur due to client restrictions, but prevent in case of a hack
		if (!container->GetItem(0))
		{
			user->Message(13, "Error: No item in slot 0 of sealer");
		}
		if (!container->GetItem(1))
		{
			user->Message(13, "Error: No item in slot 1 of sealer");
		}
		// May want to user->Kick() them to prevent potential inventory sync issues if it happened legitimately
		return;
	}

	bool deleteItems = false;

	ItemInst *itemOneToPush = NULL, *itemTwoToPush = NULL;

	// Adding augment
	if (in_augment->augment_slot == -1)
	{
		if (((slot=tobe_auged->AvailableAugmentSlot(auged_with->GetAugmentType()))!=-1) && (tobe_auged->AvailableWearSlot(auged_with->GetItem()->Slots)))
		{
			tobe_auged->PutAugment(slot,*auged_with);
			itemOneToPush = tobe_auged->Clone();
			deleteItems = true;
		}
		else
		{
			user->Message(13, "Error: No available slot for augment");
		}
	}
	else
	{
		ItemInst *old_aug=NULL;
		const uint32 id=auged_with->GetID();
		if (id==40408 || id==40409 || id==40410)
			tobe_auged->DeleteAugment(in_augment->augment_slot);
		else
			old_aug=tobe_auged->RemoveAugment(in_augment->augment_slot);

		itemOneToPush = tobe_auged->Clone();
		if (old_aug)
			itemTwoToPush = old_aug->Clone();

		deleteItems = true;
	}

	if (deleteItems)
	{
		if (worldo)
		{
			container->Clear();
			EQApplicationPacket* outapp = new EQApplicationPacket(OP_ClearObject, sizeof(ClearObject_Struct));
			ClearObject_Struct *cos = (ClearObject_Struct *)outapp->pBuffer;
			cos->Clear = 1;
			user->QueuePacket(outapp);
			safe_delete(outapp);
			database.DeleteWorldContainer(worldo->m_id, zone->GetZoneID());
		}
		else
		{
			// Delete items in our inventory container... 
			for (uint8 i=0; i<10; i++)
			{
				const ItemInst* inst = container->GetItem(i);
				if (inst)
				{
					user->DeleteItemInInventory(Inventory::CalcSlotId(in_augment->container_slot,i),0,true);
				}
			}
			// Explicitly mark container as cleared.
			container->Clear();
		}
	}

	// Must push items after the items in inventory are deleted - necessary due to lore items...
	if (itemOneToPush)
	{
		user->PushItemOnCursor(*itemOneToPush,true);
	}
	if (itemTwoToPush)
	{
		user->PushItemOnCursor(*itemTwoToPush,true);
	}

}

Hopefully I have some time to actually test these changes myself. I dunno if that will be soon or when. If l0stmancd or anyone else wants to run it through some decent testing, feel free. I will probably run it on my server for a day or so to make sure there are no major issues with it before committing it. If anyone sees any potential issues with the code, lemme know as well.

[l0stmancd]

Reviewed change - off hand I do not see an issue.

Will do a full test on the augmentation sealer and the bird bath later today and post findings.

[l0stmancd]

Hi Trevius,

Spent about 30 minutes testing the change today. Everything seems good in the code as it is except maybe fixing the indent on the "itemOneToPush = tobe_auged->Clone()" unless this was introduced just on the forums when you posted.

Testing conducted in both aug pool + aug inventory container:
Augment / Deaugmenting with lore sword / augment in different slots each time.
Introduced server lag and was able to still augment / deaugment without an issue.

NOTE: there is still the chance to cause client oddities if a user calls the augment procedure, the server lags out, and then a client picks up one of the items in the augmentation pool / inventory. This has always been here and is present for tradeskills also so I dont think its something to worry about. Just mentioning it to be complete. Very rare that this would happen. Wish there was a way to stop the user from taking items out of a pack / container after they send the client command for combine or aug. Not to stop hackers but to stop legit people who may make a mistake when the server is lagging.

NOTE: Was able to get into one of the areas that you have marked "// Less than 2 items in the augmentation device ... in case of a hack ..." It is actually pretty easy to get in here if the server lags out long enough for you to press the augment/deaugment button twice. You have a note saying you may wish to kick a user due to sync issues - would not necessarily suggest kicking a user in that case as the server could be (in another thread) about to pass down the users newly augmented items. You don't have a user->Kick() call (this was just a note you made) but wanted to mention it.

Your other area above it ("Either 2 augmentable items found or none found") - yeah, I cannot think of a single way that this could be done legitimately. Would not be a bad thing to either kick() or log to the hackers table but, frankly, I dont see a harm in leaving it the way you have it.

Everything seems good.

[trevius]

Thanks for reviewing and testing that

I will get it on my server and then put it on the SVN if no issues. I will just remove the comments about kicking users, as it doesn't seem to be needed. If there are any other item loss issues, they are probably related to something else like SwapItem, or something else that the client handles a bit differently than the server.