Ensure access transforms of core registry components fail reliably, implement security manager to enforce

[cpw]

We need to protect the registry from rogue mods that are using reflective and access transforming attacks on the registries, causing world corruption, and other serious end user issues..

Found attacks by cofh, hunger overhaul, and other mods in the wild..

[LexManos]

Ya this is gunna be a bitch, and cause issues. Why can't modders you know.. actually help the project and expand it like a open source community project should be. Instead of just hacking things because obviously they know best?

[Parker8283]

You guys have been preaching "ALIASING" for a long time, but since it isn't implemented, we have to come up with our own ways to make it work. I would be completely willing to help make a system that doesn't get us chewed out by you guys; Lex basically laid out his plan for this system in my issue on FG, so I'll see what I can do.

[wgaylord]

I will start working on a system for this as well. But the biggest problem comes when more then one mod replaces a block how should it be handled.

[Parker8283]

I've got a good one going right now; there are a couple of minor things I need to fix, but other than that, it works. Currently, mine is first come, first serve, so after the first alias is registered, no others can. That should be expanded upon.

[Parker8283]

Also, considering that I know that it's intended to not REMOVE the vanilla blocks and items, but just to make them unused, the aliased blocks and items will have different data values, but I've modified the FMLControlledRegistryNamespaced so that even if someone did /give Parker8283 1 or /give Parker8283 minecraft:stone, they get the aliased stone with the different data value.

[wgaylord]

I would say keep the normal vanilla blocks in place but make a secondary list that has the replaced ones in it but the other blocks still exist with there correct "ID" (the minecraft:stone) but the new block is used instead. Basically a dictionary with minecraft name (stone) and then the mods name like mod:newstone that is used to look up the right blocks on first load of a world.

Sorry about the rambling I am working on something else also.. (Hint: my own class mapper to work with snapshots.)

[Parker8283]

Yea, that's what I just described. Lex did say that he wants the Blocks.* and Items.* to get aliased blocks (if they exist). In theory, since I added the alias check into the method that populates those fields, it should work, but they are final, so it may still contain the original unaliased thing.

Also, why? There's no point. Forge won't come out with snapshot releases, and pure coremodding is hard and should have no place.

[wgaylord]

What if the class that registers them to minecraft was patched to go though your method so when a mod registers it can "remap" the old "pointer" (old reference) to point at the mod block replacing it instead of pointing to the Minecraft one. And when a minecraft block is called for it uses that list and the real blocks are still registered just not usable.

(I am working on finding bugs in minecraft and finding were in the code that bug is so Mojang can fix it. like the current TNT bug in the snapshots. And also I want to work on making the mapper map from 1.7 and up so i can only test it on snapshots and 1.7. Plus when it is finished as soon as a new minecraft comes out I can run it and get a bunch of mappings for MCP to work with so that they can update faster.)

[Parker8283]

I'm not quite sure that I understand what you are saying, but it appears that Blocks.* and Items.* are successfully referencing their respective aliases. To an extent. Testing with block placement (world.setBlock(x, y, z, Blocks.*)), and it appears that the block is transparent until one of them are broke in whatever structure is placed, then all textures reappear. It even appears that the transparent blocks are the aliased blocks.

[wgaylord]

Okay! I am hoping for the best but we also I think need to stop "rogue mods" from changing the real list. That is if i am reading what cpw put right.

[Parker8283]

I AM half of one of the rogue mods, and I was the one who started the whole debate. That's why I'm trying to fix it.

[cpw]

Lol chibill. You're never gonna get 1.7 to work @snapshots. Sorry.

On the main point of this issue. Yes that's exactly the idea. References to 'Minecraft:stone' become the replaced block. Trivially, there can be only one replacement. For first pass it should just allow substitution of any one block or item with an alternative. That should be stored, since it is part of persistent world state (these are my main concerns with the hackarounds that exist today- they take no account of others, and they don't persist, both of which could result is extreme surprises).

I truly thought player had implemented a complete aliasing system, not just missing block replacement.

Anyway, the only main issue with this is item stacks that hold references to the 'replacement'. Blocks.* and items.* can be dynamically replaced at id load time. Perhaps mods should do their replacements as part of that cycle? A lot more mods should move things like recipe creation to the server creation events so they always get a consistent view. Imposing this rule might require work and a hard version change..

[Parker8283]

GameRegistry.addAlias had NOTHING in the method, and in the FMLControlledRegistryNamespaced had a method called addAlias, but it appears to not be what we need it for.

Would you like me to commit to my fork so you can see what I've done and give me feedback? It would be greatly appreciated.

[sfPlayer1]

GameRegistry.addAlias is legacy, FMLControlledRegistryNamespaced.addAlias is just for remapping from the missing item event. I haven't added replacement support so far as it just isn't feasible to do in a solid reliable way.

There's various issues to be solved with replacements in general:

• conflicts: 2 mods want to replace the same item
• compatibility: replacement doesn't inherit from the original block or incompatible api in general, compatibility with failed/double replacements due to conflicts
• consistency: what happens if the replacement providing mod is removed, replaced by another mod, added after creating the world, drops the replacement or acts differently between client+server or between saves
• references: the whole system is built on references, the to-be-replaced item may already be referenced directly by something else, e.g. ItemBlocks, recipes, config type data structures, caches
• possibly more, the longer you think about this, the worse it gets ;)

Of course one can work around some of those by restrictive replacement api, but esp. the reference issue is unfixable.

The only clean fix is adding the required extra functionality directly to the item in question or using an event. Both are Forge territory or use ASM to manipulate the block/item class for short term workarounds.

[wgaylord]

But Lex said it is not a Forge problem and to come here. Also i am not trying to make FML or Forge work on snapshots. I am making a minecraft automatic jar notch to srg name mapper.

[wgaylord]

We could handle it much like we do the Ore dictionary Let the blocks have the same name and register them with flags like Onplace(Which also gets on destroy) ,Onworldgen ,and then maybe some other flags. that way you can in theory let more then one replace the normal ones.

[Parker8283]

Conflicts: My system gives the alias to the first that tries to register it. It's not the best, but it's a good start. Compatibility: I'm not quite sure I fully understand, but you would expect the modder to not be dumb and if they're replacing Stone, they would replace it with Stone and not something else. Consistency: I haven't tested it yet, but I see your point. I should look into that; I have some good ideas. References: Again, I don't fully understand it, but it seems that you're referring to Blocks.* or Items.* being used in stuff internally, how would that work with aliasing?

Let me be clear; I'm not trying to say that I've got the best system, nor am I saying that everyone should use mine. I'm helping out an open source community. In fact, I'm going to go out and say that I DON'T have the best system. But it's a start, and a non-hacky way (hopefully). I started this recent madness with my issue on FG, so I'm doing my part in trying to rectify it.

[cpw]

@sfPlayer1 OK. Here's my thoughts.

• conflicts: we enforce one and only one replacement.
• compatibility: runtime type checking is very feasible. Substitute needs a relationship with original.
• consistency: we persist the aliasing for a world. Treat it as a missing element if it is not present-> it has risks, but can be handled as missing blocks are.
• references: Yes, this is a significant issue. Ultimately, mods shouldn't reference stuff outside the scope of the server. This might become much more enforceable in 1.8.

On references, mods can create itemstacks, which will encode a reference to the original item. That itemstack will not be updated with the replacement. Ultimately, the best behaviour is to only create things like recipes at server initialization time, not in the traditional pre/init/post events.

Note, all of these issues are problems with the hackaround. @skyboy cannot address any of them in a meaningful way in his "clever code". Each of them will be a source of bugs, and some are possibly world corrupting. He can pretend they won't be, but they will.

Unfortunately, that is why it needs to be fixed in FML. There is no other place to fix it. Hackarounds in mods are never going to be able to capture the "full state" of the system. Also note, this is a fundamental restriction with the registry approach, irrespective of if FML synchronizes it.

Long term, the problem becomes something vanilla has to solve as well, so whatever we adopt will likely influence their solution as well.

In the meantime, you're gonna have to suck it up that either:

1. You fuck up worlds, mods and other things and replace items and blocks with a hackaround.
2. You wait for or help develop the solution.

[cpw]

Note, as @chibill says, it's possible a partial forge based solution is to add events for a lot of the common vanilla things - so you can change the behaviour of vanilla blocks without having to replace them.

[LexManos]

I'm looking into this when I get time and sitting down and testing things. There is a good solution that I like {it shouldn't be official as it includes asm black magic} but I need to actually get it working. And then do some profiling to see if it's to costly. As for Player's concerns:

• First Come First Serve like we have for many other things.But chaining may be possible AE: minecraft:stone -> thaumcraft:stone -> terafirmacraft:stone Haven't decided yet
• That is a mod issue and if a modder fucks that up they can die in a fire for all I care.
• The client would sync all overrides from the server, If a mod overrides another block they are assumed to keep compatibility if removed {see point 1} or we would most likely throw a missing block exception similiar to how we do now when removing mods.
• This can in theory be done, See my first paragraph about testing things.
• You sound like you think we don't know this stuff or think it through, I do, quite a bit.

There wil be a solution, and a good fully tested one, eventually...

[cpw]

@LexManos yes, I agree. I think these are solvable problems. The reference one is the hardest, but can probably be addressed by either changing itemstack to store the name, or some ASM fun.

The rest is as you say. I would suggest not supporting chained replacement. That could get weird. I think if an alias already exists, throw a HARD exception. That forces modders to think about what they're gonna do if they can't get their alias ;)

[LexManos]

Ya, it worked out well in the FluidRegistry I think... Unless people have reports of mods bypassing this: https://github.com/MinecraftForge/MinecraftForge/blob/master/src/main/java/net/minecraftforge/fluids/Fluid.java#L113

[wgaylord]

Actually if we have events for this why are people not using the events to tell miencraft like when i break grass to not give seeds AKA cancel event and set block to air. Or when planting seeds make it place a custom seed. With a cancel and set block to your block then decrees the stack size. Then we just need to protect the default block and item list some how. But we need to set up a block placed event to make this as usable as i see it.

[skyboy]

The code in CoFH hasn't caused any end-user issues (because it's not even been released), nor is it an "attack." If you're having issues with registry corruption right now, it's either your code or mojang's code that's faulty.

[wgaylord]

its you people going out of the way to go against what forge need to do to not corrupt worlds.

[skyboy]

I'm very much working with forge/FML's system with my code, and only replacing the object instance associated with the ID/string mapping. Saves remain stable and compatible regardless of the presence of the mod, or it being removed. I repeat, if you're having issues with registry corruption right now, it's either your code or mojang's code that's faulty.

[LexManos]

Calm your tits or i'll start using my moderation powers. The point has been made and it needs to be addressed. You should know better then to hack around on Forge/FML internals. We are open source, and highly accessible to people. Please from now on go down the proper channels instead of setting bad examples for everyone.

[Parker8283]

https://github.com/Parker8283/FML/tree/patch-aliasing

So here's what I've gotten so far for ideas (check most recent commit on that branch). It looks like I fucked up the formatting on the Block.java.patch. I'll fix it (if I did indeed fuck it up). Maybe what I've done will give you guys some ideas on how to make this work. One minor bug is that the Blocks.* and Items.* reference the aliased thing only sometimes. On world generation, it appears to reference the aliased, but for things like recipes, it doesn't. For world.setBlock when using Blocks.*, it makes them transparent until a block update happens, then they look like the aliased. That can probably be fixed. So, what do you think?

Again, stating that I don't care who's system is used, as long as an alias system is added at some point.