A bug that can be used to duplicate any items! #2655

Closed
azbh111 opened this Issue Nov 23, 2016 · 44 comments

Projects

None yet

4 participants

@mindforger
Collaborator
mindforger commented Nov 23, 2016 edited

and which AE2 Version did you use ? which minecraft version is that? i mean nice video but how should we guess what you are on :)

@azbh111
azbh111 commented Nov 23, 2016

appliedenergistics2-rv3-beta-6

@azbh111
azbh111 commented Nov 23, 2016

forge:1.7.10-10.13.4.1614-1.7.10

@mindforger
Collaborator

oookay, i am gonna try this on evening XD

@azbh111
azbh111 commented Nov 23, 2016

now I have to ban the ME Controller
QAQ

@mindforger
Collaborator
mindforger commented Nov 23, 2016 edited

do you have channels disabled on your video ?
oops stupid question XD i see them in your video, is it because of the controller ?
banning the controller is a bad idea, you cripple everyone to 8 channels for their ME system!

@azbh111
azbh111 commented Nov 23, 2016 edited

what does "channels disabled on your video" mean?

@azbh111
azbh111 commented Nov 23, 2016

I test with only AE2mod,the bug is still exist.
and rv2-stable-10 has the same bug

@azbh111
azbh111 commented Nov 23, 2016

I have other ideas but ban the controller

@azbh111
azbh111 commented Nov 23, 2016

Over the past few months, some players duplicated a lot of items.
I know it,but I know the method today.
I think it related to the ME system which contains ME controller

@mindforger
Collaborator
mindforger commented Nov 23, 2016 edited

i will test this in a few hours if not somebody else is faster, stay a bit patient, i am sure it is only a special case and if not it will work with similar setups without controller also

in the meantime just some more infos for me please, is this video demo in multiplayer or single player? if multiplayer, are there any things like sponge, bukkit, cauldron present?

@azbh111
azbh111 commented Nov 23, 2016 edited

multiplayer server: Thermos-1.7.10-1614-58-server
https://tcpr.ca/downloads/thermos
https://github.com/CyberdyneCC/Thermos
single player:forge:1.7.10-10.13.4.1614-1.7.10
I tested on both single and multiplayer and both were successful

@mindforger
Collaborator
mindforger commented Nov 23, 2016 edited

yeah i can confirm this @yueh and i also have a strong guess

removed explanation for the sake of running servers to not be exploited

@yueh
Member
yueh commented Nov 23, 2016

Even if it should happen, as long as it does not affect rv4 we will not fix it.

@mindforger
Collaborator
mindforger commented Nov 23, 2016 edited

dupe works fine on 1.10.2 rv4 a6

@yueh
Member
yueh commented Nov 23, 2016

I think I have fixed it, will do a PR in a few minutes to test (for rv4)

@yueh yueh added this to the rv4 beta - 1.10 milestone Nov 23, 2016
@yueh yueh added the type-bug label Nov 23, 2016
@yueh
Member
yueh commented Nov 23, 2016

@mindforger Could you please fetch the build for #2657 from jenkins and verify it?

@mindforger
Collaborator

in about 10hours or so i can test :)

@mindforger
Collaborator

is there a slight chance this will turn into an rv3 a7 eventually ? :) if by chance this video/info spreads it will ruin a lot of 1.7.10 servers i guess

@azbh111
azbh111 commented Nov 24, 2016

I have deleted this video. :)

@mindforger
Collaborator

removed my explanation for the sake of running servers :)

@yueh
Member
yueh commented Nov 24, 2016

It is actually a duplicate of #2276, so there are already enough documented ways to reproduce it.

And no we will not release a new rv3 build. Potentially a stable to preserve the last version as moving rv4 to beta will overwrite them.

@mindforger
Collaborator
mindforger commented Nov 24, 2016 edited

that one O_o i thought this one was solved already ... well okay then :)
wait, hope and ban everybody who misused it

EDIT: ahhhh it was that one that escalated so bad because of somebody thinking you get payed from mojang because he payed mojang for the game XD ... actually scratch that smiley it was a sad topic and more sad is that it seemingly prevented the fix of that bug earlier

EDIT2: @yueh quote of yourself

There is a reason why the remove is done in the inverse order compared to add. Not doing it this way can cause data loss as gridnodes are no longer able to store their data at network level

dunno if that is related to your change but it almost looks like

@azbh111
azbh111 commented Nov 24, 2016

I saw #2276 ,then I found that two separate ME systems can be confusing.
Wait for my video

@azbh111
azbh111 commented Nov 24, 2016

Even if fixed #2657, I can still duplicate items with #2276.

@mindforger
Collaborator
mindforger commented Nov 24, 2016 edited

not if it is related, it's most likely the same bug and the fix would help against both probably

if you are able, insall youself an 1.10 version and try the jenkins build that yueh asked me to test, i still have at least 3 hours of work to do before i can do it :)

@azbh111
azbh111 commented Nov 24, 2016

After my test,it do not fix both.

@azbh111
azbh111 commented Nov 24, 2016

//fix
this.inactiveCellProviders.remove(cc); //++
this.getGrid().postEvent(new MENetworkCellArrayUpdate());
this.removeCellProvider(cc, new CellChangeTracker()).applyChanges();
//this.inactiveCellProviders.remove(cc); //--

I build GridStorageCache.class myself.
#2655 fixed,but #2276 not
I'm recording a video for #2276

@mindforger
Collaborator

ah okay sorry i misunderstood your post the first time, you already tested it, thank you :)

@azbh111
azbh111 commented Nov 24, 2016 edited

https://youtu.be/aqFz9YLdHgA
appliedenergistics2-rv3-beta-6 (#2655 has been fiexd)
forge:1.7.10-10.13.4.1614-1.7.10
single player
I use another ME system to simulate a player who destroy the ME Cable.
I am not sure this video is detailed enough for you.
If not,tell me.

@mindforger
Collaborator

so it seems like the cache is not updated properly while somebody is looking at the interface

@azbh111
azbh111 commented Nov 24, 2016

I am waiting for the fixing code
:)

@mindforger
Collaborator
mindforger commented Nov 24, 2016 edited

i would almost bet that
https://github.com/AppliedEnergistics/Applied-Energistics-2/blob/fc834036a0a76daec3cf7d39bad383d97a11ac0d/src/main/java/appeng/me/NetworkEventBus.java#L126
is firing and goes unnoticed

@azbh11 you can try to patch this with a log output of the actual fired exception trace and see if my guess was right XD

EDIT: whoops wrong line

EDIT2: try-catch without even a configurable log output is kindof bad programming practice in my opinion when the thrown exception is not 100% by design

@mindforger
Collaborator
mindforger commented Nov 24, 2016 edited

i changed the line in my comment, i was mistaken for the call, it is line 126 not 152 and no it is not fixed

but i pointed out that this empty catch may be dropping an unhandled exception, you can try to insert a log output there and see if it fires when you do that exploit

@yueh i top my guess with an additional bet, when it fires i bet it is an concurrent modification exception

@azbh111
azbh111 commented Nov 24, 2016

there is no exception be thrown.

@mindforger
Collaborator
mindforger commented Nov 24, 2016 edited

darn it, okay i was looking for the connection between an open terminal view and the broken cable

but i only know about the remove() call for a node and this was the only connection between them

@yueh
Member
yueh commented Nov 25, 2016

It is funny how this takes the exact same route as the last one..

  1. I certainly will not watch a video longer than maybe 20-30 seconds without any explanation. There is simply too much stuff going on to track, consider important and try to remember it without having a chance to go back and reread a certain section. Compared to a written report with additional screenshots or an actual world download.

  2. Stop guessing that completely irrelevant things are responsible. Yes the try {} catch {} to handle cancled events might not be ideal compared to a if (event.isCanceled) return;. But it works and nothing in AE2 ever cancels these events. I can either spent my time fixing stuff or writing down why the proposed solution is useless.

Nevertheless, should be fixed now once the new build for the PR is done.

@mindforger
Collaborator
mindforger commented Nov 25, 2016 edited

whoops forgot to write last evening
i can confirm that this issue with placebreaking a cable is FIXED
but the referenced #2276 is still NOT FIXED and NOT YET REPRODUCED with rv4, only rv3 has been reproduced!

how to reproduce:

build a similar setup like before, open the terminal
let now a seconds player, turtle, blockbreaker or annihilation plane remove the connection to the drive
the terminal will not update and you can still take out the items from the drive without modifying the contents of the yet disconnected drive!

and sorry for the wild guessing there but after i got behind the update route i was so sure that the update throws an exception which is silently discarded and the update aborted, resulting in a missing terminal update
(the reason for canceling could also be an error, thus dropping the whole event still is very bad in my opinion and should result at least in a clear error log)

PS i did not test yet the changes you did 24minutes before to the fix

@yueh yueh added a commit that closed this issue Nov 26, 2016
@yueh yueh Fixes #2655, #2276: Two dupe bugs related to network storage handling
* Fixes #2655: Actually remove an ICellContainer before updating the list.
* Fixes #2276: Apply tracker changes in the correct order.
d11d6e7
@yueh yueh closed this in d11d6e7 Nov 26, 2016
@bookerthegeek
Collaborator

@yeah did you want me to test this and the other linked bug on the latest build for you? If so, I'll need steps to reproduce this bug, but the other one I still actually have that save. Lol.

@mindforger
Collaborator
mindforger commented Nov 30, 2016 edited

@bookerthegeek make a simple setup:

[drive] [cable 1] [cable 2 + terminal (add a cable and a storage monitor for the dupe item above)] [controller] [powercell]

now place a storage cell in the drive and drop an item into the terminal

break cable 1, observer storage monitor -> should update immediately to 0

now place the cable 1-> update should take a while since the network is rebuilt

now break the cabel 1 again to prepare the initial glitch

to execute glitch, place and immediately break the cable again -> the monitor would falsely update on breaking the cable with the items stored in the drive without beeing attached

you could then remove the cell from the drive and take out the items and you have duplicated them

second setup is similar but you need someone or something to break cable 1 WHILE you are in the terminal!

OP did it with a delayline of repeaters that activate an annihilation pane destroying the cable
he flicked the lever, opened the terminal and the delayed destruction did the trick then, you could also take out all items stored on the cell without modifying it's content because it is not conected, effectively duping everything

@bookerthegeek
Collaborator

Thanks. I'll get some testing setups tonight and report back.

@azbh111
azbh111 commented Dec 1, 2016

both were fiexd

@bookerthegeek
Collaborator

Nice, I'll test the other bugs then. 😎

@azbh111 azbh111 referenced this issue Dec 7, 2016
Closed

duplicate items #2688

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment