Data not stored / duplication glitch #40

New Issue

2016-03-21T23:25:23-04:00

Kakifrucht commented

2016-03-21 23:25:23 -04:00

(Migrated from github.com)

Finally found it. Was driving me nuts figuring this out.
Once you open a players inventory and thus the player.dat file won't be saved properly. When the player logs out, whatever he does in the meantime, will result in keeping the data on login that was stored before the player logged in. This can be used to duplicate entire inventories. You don't even have to have inventory editing permissions, viewing is enough.

To reproduce:

Spec players inv, close it
Teleport him away, do something with him, log out, move items from your inventory into a chest to dupe
On login, his inventory and location will be restored to the state it had before his last login.

This issue existed a very long time, I just never figured out where it came from. Right now I'm on spigot 1.9. I was able to reproduce this on a empty Spigot with only Openinv installed. I wonder how nobody noticed this already.

Finally found it. Was driving me nuts figuring this out. Once you open a players inventory and thus the player.dat file won't be saved properly. When the player logs out, whatever he does in the meantime, will result in keeping the data on login that was stored before the player logged in. This can be used to duplicate entire inventories. You don't even have to have inventory editing permissions, viewing is enough. To reproduce: 1. Spec players inv, close it 2. Teleport him away, do something with him, log out, move items from your inventory into a chest to dupe 3. On login, his inventory and location will be restored to the state it had before his last login. This issue existed a very long time, I just never figured out where it came from. Right now I'm on spigot 1.9. I was able to reproduce this on a empty Spigot with only Openinv installed. I wonder how nobody noticed this already.

EvilOlaf commented

2016-03-22 02:21:16 -04:00

(Migrated from github.com)

This also means you have to own permissions to view other players inventories, right?

Kakifrucht commented

2016-03-22 09:50:02 -04:00

(Migrated from github.com)

Yes

EvilOlaf commented

2016-03-22 11:25:57 -04:00

(Migrated from github.com)

Okey, so actually only team mates might be able to use this glitch.
Thank you very much for your investigation and sharing your results.

Okey, so actually only team mates might be able to use this glitch. Thank you very much for your investigation and sharing your results.

Kakifrucht commented

2016-03-22 11:34:32 -04:00

(Migrated from github.com)

If you only allow your staff to use it, you are quite safe, however lots of servers give out this permission to players.

EvilOlaf commented

2016-03-22 11:40:18 -04:00

(Migrated from github.com)

Honestly I see no reason why....

Kakifrucht commented

2016-03-22 11:42:09 -04:00

(Migrated from github.com)

Donator perk ;)

EvilOlaf commented

2016-03-22 11:59:16 -04:00

(Migrated from github.com)

This would probably the last thing I would grant for a donation...if I would accept donations or similar at all :P

darkarrow commented

2016-03-30 01:14:10 -04:00

(Migrated from github.com)

I tested this on a test server and put stuff in my inventory then opened it, removed everything then left and when I came back the stuff I removed was back. Then I did the same thing but waiting a minute before leaving with the same result. However, waiting 5 minutes before leaving it actually saved the inventory properly and I didn't have the items that were supposed to be gone. But as said previously if players can use the command it is a pretty bad bug. Is this in the process of getting fixed or is it being investigated or something?

EvilOlaf commented

2016-03-30 01:51:46 -04:00

(Migrated from github.com)

I guess so as @ShadowRanger added a label ;)

Kakifrucht commented

2016-03-30 10:00:54 -04:00

(Migrated from github.com)

This propably requires fundamental internal changes. Maybe two technical invsee's, one for online and the other for offline players? Like Essentials online player /invsee, when the player is online and only use the file fetch invsee when the player is offline.

ShadowRanger commented

2016-03-30 22:52:58 -04:00

(Migrated from github.com)

I've been a bit busy lately so haven't had a chance to look into this. However, I will try and look into it as soon as I can. Thanks for letting us know of this issue.

RoboMWM commented

2016-03-31 20:11:53 -04:00

(Migrated from github.com)

Once you open a players inventory and thus the player.dat file won't be saved properly.

Could you rephrase this, not sure what you're trying to say here.

Teleport him away, do something with him

I'm confused what you mean by this.

I'm just trying to understand what this bug is, since I use OpenInv purely as an admin, informative plugin (I don't edit inventories via it, and only I can use it). Is this scenario affected as well?

> Once you open a players inventory and thus the player.dat file won't be saved properly. Could you rephrase this, not sure what you're trying to say here. > Teleport him away, do something with him I'm confused what you mean by this. I'm just trying to understand what this bug is, since I use OpenInv purely as an admin, informative plugin (I don't edit inventories via it, and only I can use it). Is this scenario affected as well?

Kakifrucht commented

2016-03-31 20:36:31 -04:00

(Migrated from github.com)

Yes, it is. Once you open/view a players inventory while he is online this bug will affect the spectated player. For instance if the player has a full inventory, you view his inventory with /openinv, then he moves all his stuff out of his inventory into a chest and relogs he will still have everything he had before the server last saved chached player.dat's onto hard disk. This may be hier initial login state or the state after a save-all.

RoboMWM commented

2016-03-31 20:50:25 -04:00

(Migrated from github.com)

So does the player move stuff out of inventory into a chest while you're viewing, or after you've viewed the inventory?

Kakifrucht commented

2016-03-31 21:07:32 -04:00

(Migrated from github.com)

Doesn't matter afaik.
But like @darkarrow stated, this only seems to happen until after 5 minutes of closing the players inventory, if he stays on longer than that it will be fine.

Doesn't matter afaik. But like @darkarrow stated, this only seems to happen until after 5 minutes of closing the players inventory, if he stays on longer than that it will be fine.

cbarber commented

2016-04-10 15:59:05 -04:00

(Migrated from github.com)

Added pull request that fixes this issue: #41

Jikoo commented

2016-04-11 19:03:56 -04:00

(Migrated from github.com)

I was unable to reproduce this on my fork using the steps outlined, however, I could replicate pretty much the same issue with the following steps:

Open player inventory, player can be online or offline.
Log player out and back in
Have player make any inventory modifications and log out - dump items, etc. This is where the issue is - when a player comes back online, their inventory is no longer tied to the inventory being viewed.
Close open inventory to set viewed inventory's contents to player's current
Log back in to old inventory

The root of the issue is that SpecialPlayerInventory#playerOnline does not update PlayerInventory.player or SpecialPlayerInventory.owner. The same sort of applies to ender chests, though they don't suffer a duplication or desync bug because of it.

I do not believe #41 addresses the underlying issue yet, it just reduces useless saves.

I was unable to reproduce this on my fork using the steps outlined, however, I could replicate pretty much the same issue with the following steps: 1) Open player inventory, player can be online or offline. 2) Log player out and back in 3) Have player make any inventory modifications and log out - dump items, etc. This is where the issue is - when a player comes back online, their inventory is no longer tied to the inventory being viewed. 4) Close open inventory to set viewed inventory's contents to player's current 5) Log back in to old inventory The root of the issue is that SpecialPlayerInventory#playerOnline does not update PlayerInventory.player or SpecialPlayerInventory.owner. The same sort of applies to ender chests, though they don't suffer a duplication or desync bug because of it. I do not believe #41 addresses the underlying issue yet, it just reduces useless saves.

Kakifrucht commented

2016-04-11 19:36:16 -04:00

(Migrated from github.com)

@Jikoo I tried reproducing with your given steps but couldn't.

Does your fork fix this issue already?

@Jikoo I tried reproducing with your given steps but couldn't. Does your fork fix this issue already?

Jikoo commented

2016-04-11 19:43:44 -04:00

(Migrated from github.com)

Yeah, I fixed my fork today. I diverged back when 1.8 came out because lookups were horrible, so I'm not too surprised there are differences in how it manifests, but the issue itself extended back all the way to pre-versioned-packaging. Kind of amazing that it had never been noticed or fixed before, nice spot.

cbarber commented

2016-04-11 21:23:32 -04:00

(Migrated from github.com)

@Jikoo, #41 resets the open inventory to the player's inventory on login, reverting any changes but keeping inventories consistent in their saves. I tested all paths of login/logout of the players interacting in the inventory. I did not test a third (it does not look like that is intended to work in the command method).

You're right about the proper fix, however, SpecialPlayerInventory passes player/owner handles to its super constructor. You would need to close the current instance and create a new instance to make sure all of the internals are set correctly and hopefully (more) future proof. That would be a much larger overhaul.

@Jikoo, #41 resets the open inventory to the player's inventory on login, reverting any changes but keeping inventories consistent in their saves. I tested all paths of login/logout of the players interacting in the inventory. I did not test a third (it does not look like that is intended to work in the command method). You're right about the proper fix, however, SpecialPlayerInventory passes player/owner handles to its super constructor. You would need to close the current instance and create a new instance to make sure all of the internals are set correctly and hopefully (more) future proof. That would be a much larger overhaul.

ShadowRanger commented

2016-04-12 00:42:33 -04:00

(Migrated from github.com)

I've just merged a pull request of my fork which contains cbarber's fix for this issue as well as a few other changes which are listed in the pull request. I will release the updated version of this plugin on BukkitDev within the next couple of days. If anyone has any other issues or changes they would like to put forward before I do so, please let me know. Thanks.

cbarber commented

2016-04-14 08:07:19 -04:00

(Migrated from github.com)

I was thinking about this more, and the bug will still exist between player/player interactions.

Player A opens Player B's inventory while player B is offline.
Player A moves items out of the inventory to another storage.
Player B signs on. The open inventory is reverted to the current .dat file.

I think the correct way to handle a player signing on is to clone the current contents to their inventory, close the open inventory, and open a new inventory using the current player's handle (passing it down to super).

[Edit] - "player B is offline" not A.

I was thinking about this more, and the bug will still exist between player/player interactions. - Player A opens Player B's inventory while player B is offline. - Player A moves items out of the inventory to another storage. - Player B signs on. The open inventory is reverted to the current .dat file. I think the correct way to handle a player signing on is to clone the current contents to their inventory, close the open inventory, and open a new inventory using the current player's handle (passing it down to super). [Edit] - "player B is offline" not A.

Kakifrucht commented

2016-04-14 09:39:01 -04:00

(Migrated from github.com)

Yeah, noticed that behaviour with your fix too, @Jikoo's implementation is fine however.

Jikoo commented

2016-04-14 11:18:21 -04:00

(Migrated from github.com)

I know @cbarber's argument against my fix was that we don't know that the internals are all set properly, however, since we already need to read a little NMS for every update, I don't think it's too outlandish to decompile PlayerInventory and see that the constructor populates the fields player, g, the inventory arrays, and a couple of other minor things we don't need to mess with. I did have to correct my method for setting item arrays with reflection to include the field g, but that would have been a bug from the release of 1.9 anyway. Beyond that, setting player to the new EntityPlayer and setting the player's item arrays works like a charm.
For your consideration, here's how I set item arrays and set the owner online to correct the existing inventory.

Edit: I guess this fix is becoming more an issue of "do you continue to use a lot of NMS or do you slowly work towards a more API-based solution." It's up to the repo owners I suppose, it's certainly possible to achieve either way.

I know @cbarber's argument against my fix was that we don't know that the internals are all set properly, however, since we already need to read a little NMS for every update, I don't think it's too outlandish to decompile PlayerInventory and see that the constructor populates the fields player, g, the inventory arrays, and a couple of other minor things we don't need to mess with. I did have to correct my method for setting item arrays with reflection to include the field g, but that would have been a bug from the release of 1.9 anyway. Beyond that, setting player to the new EntityPlayer and setting the player's item arrays works like a charm. For your consideration, here's how I [set item arrays](https://github.com/Jikoo/OpenInv/blob/1fbaa5b2a91938e987ff1085c54c1260f22fc173/src/main/java/com/lishid/openinv/internal/v1_9_R1/SpecialPlayerInventory.java#L49) and [set the owner online](https://github.com/Jikoo/OpenInv/blob/1fbaa5b2a91938e987ff1085c54c1260f22fc173/src/main/java/com/lishid/openinv/internal/v1_9_R1/SpecialPlayerInventory.java#L101) to correct the existing inventory. Edit: I guess this fix is becoming more an issue of "do you continue to use a lot of NMS or do you slowly work towards a more API-based solution." It's up to the repo owners I suppose, it's certainly possible to achieve either way.

ShadowRanger commented

2016-04-14 23:07:49 -04:00

(Migrated from github.com)

I don't mind how the fix is achieved, just as long as it resolves the issue properly. I can't seem to tell what the argument is against Jikoo's way of doing it if it works properly and fixes the issue though?

cbarber commented

2016-04-14 23:29:45 -04:00

(Migrated from github.com)

The argument is tighter coupling between PlayerInventory's implementation and SpecialPlayerInventory. The consequence of this is there is a higher chance future changes to PlayerInventory will break SpecialPlayerInventory. The importance of that risk is up to the maintainers as they'll be the ones to decompile and test after updates. If you don't mind, run with it.

👍 1

JohOply commented

2016-04-17 17:35:36 -04:00

(Migrated from github.com)

Same bug : in case where admin see his self enderchest

Check enderchest (/oe)
Change something in your inventory
Logout, then login
Enderchest is empty, inventory rollbacked, location not saved

in case where admin see his self inventory

Check inventory (/oi)
Change something in your inventory, change world...
Logout, then login
Inventory save, location not saved

Same bug : in case where admin see his self enderchest 1. Check enderchest (/oe) 2. Change something in your inventory 3. Logout, then login 4. Enderchest is empty, inventory rollbacked, location not saved in case where admin see his self inventory 1. Check inventory (/oi) 2. Change something in your inventory, change world... 3. Logout, then login 4. Inventory save, location not saved

PseudoKnight commented

2016-04-19 20:31:00 -04:00

(Migrated from github.com)

Could this be related to how 1.9 doesn't save inventories on player logout anymore? It used to but doesn't anymore. This is why after 5 minutes the problem resolves itself, because that's probably when the save interval occurs.

👎 1

Kakifrucht commented

2016-04-19 21:33:08 -04:00

(Migrated from github.com)

@PseudoKnight no, you can reproduce this on any version. It happened well before 1.9 aswell.

👍 1

Johandrex commented

2016-05-03 17:14:43 -04:00

(Migrated from github.com)

Jesus christ how haven't this been fixed yet?

Kakifrucht commented

2016-05-03 17:24:00 -04:00

(Migrated from github.com)

Use @Jikoo's fork.

👍 1

Johandrex commented

2016-05-03 17:41:20 -04:00

(Migrated from github.com)

@Kakifrucht ah thanks, I should've read the earlier posts lol.

ShadowRanger commented

2016-05-04 00:44:54 -04:00

(Migrated from github.com)

Alright so I've finally gotten around to being able to implement Jikoo's fix for this issue. Sorry for taking so long. @Jikoo if you could please check over my pull request containing your fixes before I merge it, it would be much appreciated. You can view it here: ba9396ad5c Please let me know if I have missed anything or made any mistakes. Thanks.

Alright so I've finally gotten around to being able to implement Jikoo's fix for this issue. Sorry for taking so long. @Jikoo if you could please check over my pull request containing your fixes before I merge it, it would be much appreciated. You can view it here: https://github.com/ShadowRanger/OpenInv/commit/ba9396ad5ca39b27d5e5b883f1d7ffa144394198 Please let me know if I have missed anything or made any mistakes. Thanks.

👍 1

Jikoo commented

2016-05-04 01:03:29 -04:00

(Migrated from github.com)

I'll take a look tomorrow, unfortunately right now it's 1am for me and I have work at 7:30. Great life choices were made today.

❤️ 1

Jikoo commented

2016-05-04 08:52:40 -04:00

(Migrated from github.com)

Looks good to me, can't see anything wrong. I see you also opted to include the feature to reduce duplicate saves 👍

Looks good to me, can't see anything wrong. I see you also opted to include the feature to reduce duplicate saves :+1:

❤️ 1