Pryaxis / TShock

☕️⚡️TShock provides Terraria servers with server-side characters, anti-cheat, and community management tools.
GNU General Public License v3.0
2.43k stars 382 forks source link

Multiple hooks use the same, dirty/exhausted MessageBuffer object. #1456

Closed bartico6 closed 7 years ago

bartico6 commented 7 years ago

If more than one plugin hooks NetGetData or some other MessageBuffer-based hook through ServerApi.Hooks.NetGetData.Register(this, function, priority); and your plugin's hook is called after some other plugin, you will likely get an exhausted/dirty MessageBuffer. That is, a buffer read to the end or read halfway through (!!!!!!) by the other plugin which in return guarantees invalid data, or even simply zero (stream of 0s) data (that happens when you're reading from a MemoryStream outside its capacity, which returns 0s).

Current workaround is to manually reinitialize the MessageBuffer you get for safety:

            mb.ResetReader();
            mb.reader.ReadInt16();
            mb.reader.Read();

Above calls will reset it back to the "stock" state, and then set the reader's position to 3 through reading an int16 (packet length) and a byte (packet type) ending up at the beginning of the payload.

However, the above should be done automatically after every single hook returns control to TShock's context.

If nobody fixes this, I'll submit a PR or something.

AxisKriel commented 7 years ago

Plugins are not meant to modify the MessageBuffer directly when reading data. This is why a new MemoryStream object is usually created to avoid dirtying the original buffer, as seen here: https://github.com/QuiCM/EssentialsPlus/blob/master/EssentialsPlus/EssentialsPlus.cs#L363

Naturally, if a single developer does not adhere to these guidelines, every other plugin whose hooks act after said developer's plugin will be reading a corrupted data stream. There may have been a reason to preserve the original object intact in case you really want to make changes (for whatever reason...).

If you still believe we should keep this specific road clean to make developers' lives easier, feel free to make a PR and we shall review it as usual.

bartico6 commented 7 years ago

Just reading (not even writing) directly from the binaryreader (which is, obviously, the first thing that probably comes to mind after seeing Terraria's source - use args.Msg.reader.ReadType calls) is enough to break it for others. And yes, ideally you'd give everyone an object they can do whatever with.

bartico6 commented 7 years ago

Regardless, due to the game's design this issue is unfeasible to fix. Closing