Fix memory stomp due to inconsistent inventory layout

[jwkeating]

Bug repro: Create a multiplayer game with Player A joining Player B's game. Both players enter cathedral. Player A expends inferno staff charges until staff is empty before returning to town. Player A moves staff from hand to inventory (important). Then Player A recharges the staff at the witch. Player A will now send command with invalid inventory slot to Player B, and Player B fails to detect the invalid data which causes data to be written to player.InvGrid[] beyond the length of the array. Because of memory layout in the Player class, this memory stomp corrupts Player A's position.tile on Player B's computer, and Player B drops player A from the game.

I fixed the memory stomp by adding an error check in CheckInvSwap() but this is not sufficient to fix the problem because Player B still sends invalid data due to inconsistent assumptions about "bottom left" versus "top left" corner of items being the slot which defines the item location in the inventory. I made the code consistent by changing everything to be rooted at the bottom left corner because this is what the sprite rendering code assumes. However, I didn't add a conversion for save games so any existing save game will look weird until the inventory items are lifted to hand and replaced in the inventory. Then everything works fine again. The stash assumes items are rooted at the top left corner but this code works reliably as-is, and it's independent from the inventory code so it wasn't necessary to change it.

[StephenCWills]

This looks way more complicated than #8501.

However, I didn't add a conversion for save games so any existing save game will look weird until the inventory items are lifted to hand and replaced in the inventory.

DevilutionX saves are supposed to be compatible with vanilla. Have you tested what affect this PR has on save game compatibility?

[jwkeating]

Ah, I didn't realize you already had a fix in progress for this issue. It looks like you're fixing it from the other side (player who sends slot index should send the upper left instead of the bottom left) instead of my fix (make all of the code use bottom left). Probably, the best solution is to use your fix. However, it doesn't look like your fix actually prevents the memory stomp if the packet has an invalid slot index. I'll simplify my pull request to just the code which prevents the memory stomp, and then it can be merged with your fix and there are no incompatibility issues with vanilla game.

[StephenCWills]

However, it doesn't look like your fix actually prevents the memory stomp if the packet has an invalid slot index.

Actually, it does. That was the reason why I added the validation on the receiving end of the network packet, in the OnChangeInventoryItems() function.

[jwkeating]

You are right again!
In that case, I'll close this pull request since you've already got it covered.
I'm working in a fork of the 1.5.5 branch because I prefer c++ to lua but I just thought I should occasionally try to contribute something back because of all the work you folks have done on the project.
Thanks!