Client-Server Item Desyncronization Bugs

[Uleat]

I looked through the ts and foraging (fish/forage) code and didn't see anything out of the ordinary. I'm still sorting through the 'inventory' calls.
There's several areas that I don't trust based on what I see, but I still need to find all of the ways to test it in-game.

(The two foraging actions call different methods, PushItemOnCursor vs. PutItemInInventory(SLOT_CURSOR), but the end result is the same..just a
little more overhead with one of them.)


As far as cursor items being out of order, unless I find something specific somewhere, I'm thinking the client may be receiving UDP packets out of order.

There's no unique identifier in a series of cursor items. If the server sends three packets (a, b & c) to the client with the summonitem argument, the
client won't know that b->c->a is the wrong order unless it has the ability to identify and re-order based on timestamps. (I just don't know...)

My server is setup with drop_packets->false and is on a 100Mbs LAN..single player as well. I probably won't be able to reproduce issues that arise with
public servers.

[Uleat]

I'm not sure what's going on with this code snippet, but I can't seem to get it to work properly when I add it to another function.

This code is copied directly from inventory.cpp::Client:eleteItemInInventory. It handles client updates when deleting partial quantity from current stack charges.

// Stackable, arrows, etc ? Delete one from the stack
	outapp = new EQApplicationPacket(OP_DeleteItem, sizeof(MoveItem_Struct));

	DeleteItem_Struct* delitem	= (DeleteItem_Struct*)outapp->pBuffer;
	delitem->from_slot		= slot_id;
	delitem->to_slot		= 0xFFFFFFFF;
	delitem->number_in_stack	= 0xFFFFFFFF;
	for(int loop=0;loop<quantity;loop++)
		QueuePacket(outapp);
	safe_delete(outapp);

I've adapted it to my needs, but what is happening is the client deletes the entire stack from the cursor, then each iteration
after the first creates the 'Move Item Failed In Client Application' error message.

Changing 'number_in_stack' to 'quantity' and disabling the iteration produces no results. I've tried changing from delete_struct
to moveitem_struct, but can't seem to get the client to take charge change commands unless the client is the originator.


I think this is another area where CSDs could be creeping in..server tells client to delete 2 charges in a 5 charge stack,
but all are deleted..and the client generates a 'Move Item Failed' error.

I don't see where normal 'actions' are causing this, but it could creep up in scripted/quest calls.

[Harakiri23]

The client has 2 different opcodes for this - one is MSG_MoveCharges to move charges from a stackable item anywhere (or delete with 0xFFFF..) and one is MSG_MoveItem - which moves non-stackable items (or delete them), can also be used to "delete" a stack of items.

The opcode names for you may differ, but that is your issue to figure out the naming in eqemu.

[image]

have you tried OP_DeleteCharge instead?

[Uleat]

It's been a while since I messed with this portion of the code.

I can't remember what I did and did not try. But, when I get back to a point where I can, I will try both of those.

Thanks!

[Uleat]

I'm going to try and work on the CSD issues a little more. I think that I can manage keeping my home source code updated by manual file transfer.
Posting-to-reply times will be delayed by at least a day, however...

I've not heard any negative feedback on this CSD patch series, as yet. Just remember..if you are using the bandolier and become de-sync'd from another
unfixed bug, you can still lose items just as any 'normal' item swap would cause. So, keep an eye out for those de-sync messages and take corrective action
immediately. (I can't guarantee that you will receive a message on a de-sync that originates in code outside of this patch, so be careful out there with
bandolier use and any other action that creates/moves/deletes items without doing a zone change/re-log first.)


My next area to work on is code using inventory.cpp::Client::TryStacking(). I've noticed that the stack search order does not match
Client::MoveItemToInventory() or the client behavior that I noticed in the work with the Bandolier function. I need to find all of the ways that it is used in-game,
test them out, and verify client behavior while using them.

The two functions that call this procedure are Client::AutoPutItemInInventory() and Client::Handle_OP_ShopPlayerBuy(). ShopPlayerBuy() is a client event
driven call. The other function is called by Client::AdventureMerchantPurchase(), Client::AltCurrencyPurchase(), Corpse::LootItem() and Client::TryStacking()
(and any possible script calls.)

I will be looking through the forums in search of postings about these in-game actions. But, if you know definitively about de-sync's and item loss while using
these actions, please re-post them here. This can include mysterious item movements (especially involving the cursor) since players are likely to change
zones/log and restore syncronization before switching equipment around.


Thanks!

[Uleat]

Just a little note on item loss with overloaded inventory...


I do have a working theory on this one, but I HAVE to ensure that I can test every call to the affected code. (I'm assuming that I won't get any script calls
unless there is something defined to handle it. Otherwise, another function calls it.)

The problem likely lies in multiple areas, but the code in item.cpp::Inventory::FindFreeSlot() is most culprit. (Kinda the reverse issue that was in the Bandolier code.)

In the case of a corpse [Loot All]... When you have a full inventory (with an empty cursor), but still have occupied bags on your corpse:

- The next bag is pushed to SLOT_CURSOR and the contents are pushed to 331 - 340
- ALL subsequent bags are also pushed to SLOT_CURSOR, via the queue, and each bag's contents overwrite any previous bag's data in 331 - 340
- The bags themselves will end up in the cursor queue
- The contents of the first cursored bag will be:
-- The contents of the last corpse bag if it was full -or-
-- A mix of contents of both previous and last corpse bag items if it was not full
-- (I'll need to double-check, but I believe _putitem is used instead of _swapitem..which means existing items go bye-bye..changing it would create more problems)
- Any queued cursored bags will be empty

I'm not sure exactly what behavior would occur if a non-container item was first on the cursor since bag slots aren't moved in that case. A bag 'popping' into the
cursor slot 'may' avail any lost and un-deleted items in the 331 to 340 range. These items would now be assigned to that bag and any remaining bags would
still be empty.


This problem will probably arise with overloaded trades as well. I've seen posts regarding both loot all and trading with this issue.


Anyways... I can't seem to find an OP_Code that handles the 'Loot All' action. Does the client actually send a loot request op for each item in the corpse
window?

If it does, and the client DOES NOT move the items, we could cancel the transaction and leave the bags on the corpse. If the client DOES move the items
regardless, all we can do is minimize the damage..maybe by 'unpacking' the bags to the cursor queue, but we might have to bump up the active queue array
size. Dropping them to the ground would most likely lead to them being lost if the player can't clear their inventory to make room. (I guess we could unpack
the bags when the corpse is created as well... Wouldn't that be a mess...)

(What I've seen with the cursor queue size on the server and a SoF client: {client: 37; profile: 102; database: 1000})


(This conversation sounds familiar..sorry if I duped it from somewhere else...)

[Uleat]

Currently, I am working on a fix for item deletion during a de-sync condition.

It may, or may not, work depending on my success with manipulating the client...


Does anyone know of way to send a 'NULL' item instance to the client? I'm trying to avoid using 'OP_DeleteItem' because of the issues surrounding its use and
already empty slots. (I'd like to avoid ANY messages to the client with the exception of the one informing them of the 'Inventory Resync' itself.)

client->SendItemPacket(slot_id, item_inst, ItemPacketTrade);

If I can use this to 'empty' a slot, it would save a ton of overhead.

[Uleat]

I've bugged at least one of the dev's enough with pm's about this resync thing, so I'm taking it public.

I'm just getting back into this after a hiatus and wanted to get some feedback, if anyone has some good suggestions.


What I'm looking at doing is creating two structs for one opcode. First for incoming, and the second for outgoing.

MoveItem requires a 32-bit incoming because of the timestamp sent by the client. However, the client seems to crash when you send a 32-bit data value
(size-wise) to certain slots. Inventory appears to be ok, but bank and shared bank slots appear to be the cause of the crash.

BulkSendInventory uses a sint16 data value when sending initial equipment and I want to convert the outgoing MoveItem_Struct to sint16 as well... Obviously,
there is a lot that will require mod'ing, so I wanted to get some input from you guys before I devote that much time and effort into something that may not work.

Thanks!

[lerxst2112]

You can't just assume that because one opcode uses a particular sized variable that another would do the same.

You would need packet captures of all the cases you are trying to figure out and then you would need to reverse engineer the packets to determine the difference when moving to/from the problem slots.

[Uleat]

Yeah, I agree that would be the proper way to do it. I just don't have the means to actually do that and I don't want to involve anyone into my chaotic thought
processes beyond the Q & A at this point...

It could be that the client is not coded to handle server-originating move/delete OP's in those slots and is crashing instead of giving the normal 'failure' message.

Whether it's that or the struct size (or something else entirely,) I'd still like to pin-point the cause. The resync function works with the exception of those slots. If I
can figure this out, that would eliminate the de-sync'd item deletion completely.

The main problem with this concept is that I don't believe live servers or clients are coded to do what I'm trying to do..which is why I'm running into issues...
I believe that I was told there is a known issue with certain op code actions and slots, but they are rare in their occurrences.

I don't mind fruitless ventures as long as I can use it as a training exercise, so I'll see what becomes of it. If this does pan out, I'll see if I can get someone to
verify my results with a live packet collection.

[Uleat]

I'm not quite ready to play with adding a special out-going struct for move/delete item, so..two quick questions: (don't hit me Lerxst! )


1) The tooltip info for int32 says it's a typedef for an unsigned int32. However, MSDN .net (and/or silverlight) defines it as a signed int32...

Question: Does MSVC++ not define it the same way, is it re-defined somewhere in the code, or is this an issue between compilers (and languages?)


2) I had been playing around and discovered that a 16-bit value for 'to_slot' and/or 'number_in_stack' didn't crash the client (though it may not work...)

Question: Would a bad conversion in the client where an (unsigned) 32-bit (~0) is being forced to an (unsigned) 16-bit register cause the crash? (I'm thinking
that might be causing an overload, or at least data loss, if it's not that the client isn't coded to handle the request.)


Thanks!

[lerxst2112]

There is no such thing as an int32 in the C++ standard. In this project, as you've seen, it is a typedef, or alias, for unsigned int. It just happens to be 32 bits on the platforms the code is intended to compile on. A compiler could generate code using 64 bit integers and still be 100% standard compliant.

The packets sent between server and client are the result of packet capturing and reverse engineering. It is possible that some of the types used are not exactly the same as what the client expects, but the sizes of the packets are almost certainly correct which means changing values to a smaller type would result in an incorrectly sized packet and also offset anything that came after that value unless you add the appropriate amount of padding to match the size. If you believe a packet is being sent incorrectly the right thing to do is capture traces of those specific packets and check them against the existing code. Perhaps the packet you're looking at does expect 0xffff and not 0xffffffff in that position, but the only way to know for sure is to verify it against packets from live.

[Uleat]

Ok..duh on the int32. I didn't have my code def window up when I was looking or I would have found <types.h>

I am really leaning away from even testing a modified out-going structure at this point. My question about conversions was more about the possibility of
corrupted data when narrowing. I seem to get what I expect when using integers, so I'm guessing data loss is more prevelent with floats.


(Sorry for the re-iterations..I just want to clarify the actual issue.)

As far as capturing from live... I don't believe what I'm trying to do is ever done on live.


For instance:

When a player deletes a bank item, they first move it to their cursor. Then a client-originating moveitem deletes it (from->30: to->-1: num_in_stk->0)

When an item is on the cursor, we can issue a server-originating moveitem to delete, regardless of client action. (from->30: to->-1: num_in_stk->-1 seems to
work and is what is currently used in existing code.)

But there is no existing action (that I'm aware of) that will generate a server-originating direct bank slot deletion on live. All quest updates, tradeskill combines,
etc... come from the client's cursor or personal inventory (and bags), which I have no trouble deleting from.


That's why I don't think there's a way to actually capture this particular value for a direct bank slot delete action.

(I can overwrite the existing item with no problem. But, then we're left with a client that could have 'phantom' items in empty slots and we're back at starting
point for this whole resync business... I do have a method that will clear empty bank slots on the client, but the client will spam itself with 'deletion' messages
for each empty slot, which I'm trying to avoid.)

[lerxst2112]

If there's no way to legally generate a deletion from a bank slot then why are you worried about it? If it can't happen on live then it shouldn't happen on the emu. If it does, then that's the bug.