Porting over custom packet code from ED about dewrecode

[Shockfire]

This is a bit of an interesting issue, because it points out an inherent flaw with having a plugin system like this. While a system like this is definitely nice to have, we have to make sure we pay a lot of attention to how we design it so that it doesn't cause users tons of issues with having missing or incompatible plugins.

First of all, I think that the "required" and "optional" stuff should be done at the plugin level instead of at the packet level. Plugins can indicate whether they require to also be installed on clients, and then when a client connects it can send a list of all of the plugins it has installed (name and possibly even a version number) and the server can boot it if it's missing a required plugin. I also think we should be pretty strict about this - if a plugin ever needs to send a packet to a client for any reason, even if it might not necessarily be bad for the client to ignore it, it should be marked as required. For example, I don't really think it's fair for a server to say it's VoIP-enabled and then allow people to not even have the plugin installed. (If someone doesn't want VoIP, they can just mute everyone and disable their microphone through the plugin without actually fully disabling it. This would also let the plugin actually know that the user has done this and inform everyone else that the player has voice chat muted.) The nice thing about handling this at the plugin level, too, is that it would allow people to make server plugins which require things like client-side in-memory tag edits to be done.

As far as handling discrepancies between packet IDs, I have a couple of thoughts on how we can do this:

• The simplest solution would just be to have plugins explicitly specify the packet ID and then just maintain a list somewhere of which packet IDs are already in use. This would be the easiest thing for us to do, but it might cause a lot of issues with incompatibility between plugins if developers are stupid and don't pay attention to it.
• I think the idea of having the server send a list of registered custom packets back might work pretty well. We have to be a bit careful with how we do this though, because if a plugin gets a "peer connected" event and then tries to fire off a packet before the client's packet table has been updated, there will be a huge problem. I was thinking a good approach might be to directly integrate the handshake into the packets that the game sends when it connects, so that we can ensure that clients get the necessary info right away. (This might also prevent people from hacking around it and skipping the handshake in case we implement something like identity verification into it.) Either way, we have to be careful about how we handle network events and make sure that plugins don't receive any events until a peer has been fully set up.

Finally, I think we also need to be careful with how we use the plugin system in general. I've been thinking that we should really only be using the plugin system for community-made plugins or optional server-side plugins. Anything which we make as part of the "core gameplay experience" (so this would include VoIP, IMO) should just be built directly into ED. We have to remember that every additional plugin we make and ship with the game is just giving users one more way to break their installation (and also giving us one more way to screw up pushing out an update...). While I agree that modularity is a good thing, and that having plugins allows us to accomplish it more easily, we should also be enforcing stricter coding/design standards. Most hooks should always just be nothing more than event sources which other parts of the code hook into, for example, and we should be very careful with how we use things like global objects and singletons (since old ED is just singleton hell right now with everything coupled together and it's honestly terrible to work with). This is a completely different issue on its own though which we need to talk about separately.

Anyway, enough ranting, there's my two cents.

[kiwidoggie]

Handling collisions for packet ID's should be straight forward, if unused, we should use the high end of the spectrum (0xFE for single byte, 0xFFFE for word). Plugins should be very tightly integrated into ED, I tend to follow the KISS principle when it comes to different things like this. Plugins should never be able to be unloaded after they have been loaded once, there should be a proper management system in place, so depending on what gametype, if you want voip disabled etc, will all be handled via the management system. Community made plugins should be separated entirely from the internals for maximum security, (sandboxed, whitelisted lua environment if you choose to go that route). But the implementation of this would take some time, also include many extra dependencies (yuck).

[emoose]

Collisions probably wouldn't be that much of a problem, main issue is making sure that the server and client have the same IDs for the same packets. The server sending it's custom packets + IDs should solve that though, if we can get it into the handshake packet it should work pretty well too.

Not sure if there's any way to deregister the packet IDs after the client leaves though, if not maybe making our own sort of custom packet protocol would be easier (as in, all custom packets use the same game-packet ID, but have headers that describe the actual packet ID etc, then we'd have some sort of packet manager that takes these packets and sends them to the registered handler).

If we used a system like this maybe we could scrap the packet IDs and use a different form of identifier (GUIDs or strings maybe? If packets used the form [modulename].[packetname] as an ID there should be no chance of collision, and we can easily look up if they have the proper plugin/packet registered), it'd also give us a lot more flexibility with things, although I'm not really sure how possible it is.

I mostly only split up things into plugins as a test for the plugin system, and the ChatPlugin was split up because of the IRC/VoIP code inside which is pretty huge. We could probably refactor the VoIP stuff to be a lot smaller, and scrap the IRC stuff in favor of the chat packets (although maybe keep global chat around as a plugin.. I know some don't see the need in it, and there's a lot of idiocy there, but I've also seen people helping out when people don't know how to do things, it could be helpful for people) If people don't want VoIP enabled we could just add a VoIP.Enabled var to control whether the client connects or not.

The ServerPlugin stuff should probably be integrated back into the main DLL though, but for now it serves as a good test to make sure nobody has messed up our ABI.

Plugins should be very tightly integrated into ED, I tend to follow the KISS principle when it comes to different things like this. Plugins should never be able to be unloaded after they have been loaded once
Well with DR plugins have access to pretty much all the same funcs the main DLL uses, the modules/patchmodules in the main DLL all use the same base class/helper funcs/etc that plugins would use, we could easily move a module back and forth from the main DLL and plugins if we wanted, so they're pretty well integrated.

I've also been following that way, plugins might patch something or do something else that unloading wouldn't revert (although 99% of patches in DR are revertable), loading in plugins later on should be fine though.

Community made plugins should be separated entirely from the internals for maximum security, (sandboxed, whitelisted lua environment if you choose to go that route).
I was thinking of something like that too. Most plugins would want full access to the game though, but maybe later on we could include a scripting language for smaller plugins like that (eg things like server plugins that just act like chat bots and add chat commands)

I don't really see much of a need in them being secure though, I doubt that any malware would try using our plugin system as an attack vector, and plugins are executable code all the same so they wouldn't really need to use any of the ED funcs to attack the users system, besides our plugin loader.

If they managed to get their malware into our plugins folder it'd be surprising they didn't just replace our main DLL instead. Only real way I could see that happening is if someone uploaded a fake plugin somewhere and said to put it in the plugins folder, but I was planning on having community plugins go through something like HaloShare so they could be updated automatically etc, so users shouldn't really need to download zips from random websites.

If you meant secure as in game hacks though I'm not sure about the best way to handle it. We should probably ensure that clients only have the same plugins as the host, but then we'd need to unload any unused plugins (or force the user to restart without a plugin), and would probably have to make a way for servers to send plugins the user is missing. But then what about plugins that just do something like control an MP3 player? or a server plugin that just adds some chat commands, would the user need to download that chat command plugin too?

Maybe we shouldn't worry about plugins or any extra DLLs being hacks, there'll always be things adding DLLs to our process (overlays, AVs, etc..), for anticheat we probably shouldn't care about DLLs being added and only care about what they do to the game, though this isn't really the place for anticheat discussion :P

[kiwidoggie]

Well, it really depends on how you want to process the custom packets. If we only had access to a few unused packetID's then having everything that would need to be consistant, in 1 location and 1 location only. (Enums in a common library). Also with that, we can set 1 packetID for Halo, catch it before it finishes and use a messaging system (not like chat messaging) to handle all other custom packets. I have an example of this, and causes very little overhead (category/type checks)

[Shockfire]

So to keep this moving forward, I agree that the best system would be to only ever allocate one packet ID for all custom packets and then implement an additional identification system on top of it. It shouldn't add any significant overhead as far as I know. I have a few ideas how this can be done:

• Whenever you register a packet, manually specify a GUID value to associate with it. This has the advantage of the network protocol changing less frequently if we rename or move packet classes around.
• Hash the packet's type name at runtime and just use that as a GUID. Easier to register custom packets, but means the network protocol changes if we refactor.
• Maybe instead of hashing the packet's type name, require manually specifying a (unique) packet name when you register a packet and then hash that instead. Easier than having to manually generate a GUID value while also providing all of the benefits that the first option does.

In any case, this approach has the advantage of not needing to send a packet mapping table on connect because we can deterministically locate the handler to send a packet to.

Thoughts?

[emoose]

Maybe instead of hashing the packet's type name, require manually specifying a (unique) packet name when you register a packet and then hash that instead. Easier than having to manually generate a GUID value while also providing all of the benefits that the first option does.

Would probably be the best way, hashing the type name could lead to collisions (eg two plugins both having a "MessagePacket" packet), and as you said it'd be easier than generating a GUID.

Maybe we should send something between the client and server to make sure they both support the same packets though. I still think we should have something that specifies if the packet is required or not too, eg. if a server had some RadioPlugin which would send some internet radio data via a RadioPacket over to other players who have the RadioPlugin installed, it shouldn't be required that all users have that plugin in order to connect. (maybe a bad example, but you can get what I mean, some optional plugin that adds extra features for clients who have that plugin, but isn't required for gameplay)

In any case the required packets check should be done server-side so that people don't use some modded DLL that skips the packet check or something (eg to bypass an AnticheatPacket). Maybe the server shouldn't send any packet list, instead the client would send theirs and the server would check it and make sure they have all the required packets registered. Of course this could be easily bypassed too, but any major plugins should probably have their own handshake etc to make sure it works, the server check is just for smaller required plugins/packets.

[Shockfire]

I still think that optional packets should be done at the plugin level instead of making the packet system deal with it. Like I said, plugins can indicate whether they require to be installed on clients, and then we can provide an interface for optional plugins to query whether a peer has it installed. Each optional plugin would simply only send data to peers which have it installed. The purpose of the packet system is to send and receive packets, not deal with client configurations.

[Shockfire]

All right, I went ahead and implemented a basic GUID system. Currently it generates GUIDs by SHA-1 hashing the packet name and then taking the first 4 bytes of the digest. 4 bytes should hopefully be sufficient enough to prevent collisions, but if we end up running into issues then it can be expanded pretty easily (best to keep things simple though and reduce the amount of data we send across the network).

The public interface for the system didn't change much, except we have to specify a name when registering packets and packets have to be created by using a New() method on the sender object (this is so the GUID field in the packet struct gets initialized correctly). I'm proposing that we always prefix names with the plugin name (or "eldewrito" for official packets) to reduce the chances of a GUID conflict. For example, I called the chat message packets "eldewrito-text-chat".

[kiwidoggie]

Why not just use a actual guid? (Just askin')

[Shockfire]

I considered that, but it just overcomplicates things IMO especially when it's nice to have a packet name anyway.

[kiwidoggie]

What happens if packet's have the same name?

If we fired 2 packets with the same name, it would be a race condition for which data returned properly.

[Shockfire]

Packet names are required to be unique. Not only because of the hashing system but because it makes debugging easier if we can just print a packet name and immediately know where it came from. I've been working on a packet logging/debugging system.

[emoose]

Maybe packet names could follow the same syntax as the modules/commands stuff, something like [plugin name].[packet name] eg. "ChatPlugin.ChatMessage".

Or maybe [module name].[packet name]. We could add something to ModuleBase so modules can add packets without needing to worry about adding the plugin name to it, something like ModuleBase::AddPacket(packetName, handlerCallback), which would be called like AddPacket("ChatMessage", messageHandler), with the AddPacket func prefixing the module name to it like the AddCommand funcs do (so it'd be registered as "ModuleChat.ChatMessage").

There's a chance of two plugins using the same module names/packet names though, but maybe we can work something out to add the plugin DLL filename to it too, that way there'd be no chance of conflicts since two plugins wouldn't be able to have the same filename.