Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Critical bug #64

Open
zed-hex opened this issue May 29, 2015 · 5 comments
Open

Critical bug #64

zed-hex opened this issue May 29, 2015 · 5 comments

Comments

@zed-hex
Copy link

zed-hex commented May 29, 2015

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
Copy link
Owner

hilburn commented May 29, 2015

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
Copy link

yueh commented May 29, 2015

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
Copy link
Owner

hilburn commented May 29, 2015

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
Copy link

yueh commented May 29, 2015

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
Copy link
Owner

hilburn commented May 29, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants