hilburn / StevesAddons

Additional functionality for SFM
Other
17 stars 10 forks source link

Critical bug #64

Open zed-hex opened 9 years ago

zed-hex commented 9 years ago

There is a duplication bug with the energistics connecter when it pulls items from a subnet that has a security terminal. Here is a print screen of how this can be replicated: http://prntscr.com/7alo3n (the manager has just a simple input output code) this bug was found when I was using build: 0.10.9 of Steve’s Addons (I looked at the change log of the newer builds and did not see any thing about this being fixed)

Other relevant mods to bug: Steve’s factory A93 AE2 rv2 stable 3

hilburn commented 9 years ago

I'm not entirely sure that this is my bug - the methods I use to pull from AE systems are API ones that should take into account any security settings before giving me a stack to use.

Might be worth raising it over there as well but I'll look at it

yueh commented 9 years ago

The implementation looks mostly ok.

It is probably debatable to change AEHelper.java#L207 and add a SIMULATE step. These are usually backed by a cache as opposed to MODULATE, which needs to try and extract the items from the actual inventory. So this can be a bit more taxing on the network when it often fails to extract items. It can also potentially return null, calling .getItemStack() could therefore throw an NPE.

But AEItemBufferElement.java#L56 looks like it could be an issue. No idea SFM actually works, but it just looks wrong.

If the extraction fails it will just ignore it and silently fail. Should the extract just depend on getSizeLeft() then it will always report items available to dupe.

I have no idea how these are created, but if just the reported inventory list is used, then this will enable dupes. Not just with a security station but also anything reporting itemstacks as visible but not extractable (like storage buses can)

hilburn commented 9 years ago

Yeah there's a lot of code I've had to implement in very strange ways because of the incredibly closed down nature of SFM AEItemBufferElements are created here: https://github.com/hilburn/StevesAddons/blob/master/src/main/java/stevesaddons/tileentities/TileEntityAENode.java#L262-L273 , https://github.com/hilburn/StevesAddons/blob/master/src/main/java/stevesaddons/tileentities/TileEntityAENode.java#L311-L330 so only valid items should be made available, but I may try simulating an extract step before they are added to the buffer as that should avoid those issues. Honestly though... this is somewhat low on my list of priorities right now. Vswe has apparently woken up so if he wants to fix it he's welcome to

yueh commented 9 years ago

Not sure what you mean with "valid items". But if you mean only extractables items, then this is not possible it will break many things like storage monitors. They always need to be visible to anyone and only fail when actually trying to interact with it.

Simulating an extraction is probably very bad idea as this can get quite taxing on networks with a large amount of items. Especially if this needs to be done every tick/operation. At least they look like being created when needed and not cached and modified based on the network events.

Once there is an integration with LP or the BC robots and having requestable items. In these cases the system will also report them, but these will not be instantaneously available. On our side this should be managed through autocrafting. But if this is not considered, there might be some timed exploits. Like requesting something through SFM and immediatly use it also to block the path items would take through the LP network to reach the AE network.

AEItemBufferElement.reduceAmount() would really need the actual removed items as return value. If there is no additional check immediately against getSizeLeft(). If it does that, it might be an idea to use the return value of AEHelper.extract() to invalid the buffer. But if it just works like 1. Copy the item reported by the buffer; 2. Reduce stack, ignoring any failures; 3. Use the duped stack from 1., then there is probably no way around it with current design.

It might be an idea to implement the AEItemBufferElement as a proxy to AE2 and not cache anything. Basically chance getSizeLeft() to use a simulated extract from the network and return this value and hope no race conditions show up between the simulate to fetch the available items and actually extract them. Usually you cannot expect to obtain the correct value of items until using a MODULATE.

hilburn commented 9 years ago

As I said, I know it is not perfect - it is a) the best I could do without having to rewrite thousand line classes in SFM via asm and b) the most sense I could make of the completely nonsensical AE2 API. I will fix it to the best of my ability as and when I have time to, but if you have a burning desire to submit a PR then go ahead