ProtocolLib

3M Downloads

Player kicked while using BungeeCord and TrMenu

diogotcorreia opened this issue ยท 7 comments

commented

Describe the bug
While using BungeeCord and TrMenu, when clicking an item on the menu (e.g. /trmenu open Example),
the player is kicked with the following message.

DecoderException : java.lang.IndexOutOfBoundsException: readerIndex(0) + length(1) exceeds writerIndex(0): UnpooledSlicedByteBuf(ridx: 0, widx: 0, cap: 0/0, unwrapped: PooledUnsafeDirectByteBuf(ridx: 2, widx: 2, cap: 416)) @ io.netty.handler.codec.MessageToMessageDecoder:98

This ONLY happens if there is a plugin that contains SET_SLOT in the ListeningWhitelist. E.g.

ListeningWhitelist.newBuilder().types(PacketType.Play.Server.SET_SLOT).build();

Example of ProtocolListener implementation: https://pastebin.com/xDdFVLiD

To Reproduce
Steps to reproduce the behavior:

  1. Download the latest version of BungeeCord (git:BungeeCord-Bootstrap:1.16-R0.4-SNAPSHOT:9c078b7:1540)
  2. Clone the TrMenu repository and build the plugin (tested on v2.08).
  3. Start a BungeeCord server (no plugins)
  4. Start a 1.16.5 Spigot server (git-Spigot-5bde311-0bad58f) with TrMenu, PlaceholderAPI (dependency of TrMenu), ProtocolLib and a plugin that listens for the SET_SLOT packet using ProtocolLib.
  5. Open the default menu with /trmenu open Example.
  6. Click anywhere on the GUI.
  7. You've been kicked from the server.

Expected behavior
The client should not be kicked. For technical information, keep reading.

Version Info
https://pastebin.com/SxEZApqL

Additional context
I've been messing with ProtocolLib's source code to try and fix the issue (might submit a PR if I find the cause).

While doing it, I found out that Spigot's native packet encoder outputs a different result than ProtocolLib's encoder on a specific packet (the last one sent before the player is kicked).

The situation described below happens after a net.minecraft.server.v1_16_R3.PacketPlayInWindowClick is received by the server.
The packet in question is net.minecraft.server.v1_16_R3.PacketPlayOutSetSlot.
When encoding it, Spigot's native packet encoder outputs

bytes = [21, -1, -1, -1, 0] // packetID = 0x15; windowId = -1; slotId = -1; item = null

while ProtocolLib's outputs an empty array

bytes = []

The way I've tested it was by editing https://github.com/dmulloy2/ProtocolLib/blob/master/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java#L269

System.out.println("packet = " + packet);
//encodeBuffer.invoke(vanillaEncoder, ctx, packet, output);
ChannelInjector.this.encode(ctx, packet, output);
byte[] bytes = new byte[output.readableBytes()];
int readerIndex = output.readerIndex();
output.getBytes(readerIndex, bytes);
System.out.println("bytes = " + Arrays.toString(bytes));

(I commented out the ChannelInjector line and uncommented the encodeBuffer line to test Spigot's native encoder)

I don't think there's anything else in the other packets, but I'll leave the full logs here. I've clicked the top-left most slot in both logs (slot 0 I'd guess).

Spigot's Native Encoder: https://pastebin.com/7yfLiNXi
ProtocolLib's Encoder: https://pastebin.com/X1KZ9tQN

I also placed some System.out.println in ChannelInjector.encode, as you might've seen in the logs.
Not sure if it's related, but currentEvent (or the event local variable) was null on the "empty" packet, while it wasn't on the others

commented

Some more testing:
I've tried 1.15.2 and 1.8.8 Spigot as well, the same behaviour can be observed.
It's probably safe to assume it also happens in the versions in between.

commented

Just found something.
If you set this if to always run the else statement (by changing it to if(false), the crash doesn't happen anymore.
https://github.com/dmulloy2/ProtocolLib/blob/master/src/main/java/com/comphenix/protocol/injector/netty/ChannelInjector.java#L484

There's probably a reason for that to be there, so I'm gonna wait on feedback from the author.

commented

In that case, the plugin likely needs to run their listener async. You can set ListenerOptions.ASYNC in the listener

commented

@diogotcorreia It means your listener runs on the netty thread instead of the server thread. Anything that needs to be run on the server thread should be wrapped in a task of some kind (i.e. BukkitRunnable)

commented

@dmulloy2 That seems to have fixed the issue indeed. What else does it affect? Just want to make sure it won't break other parts of my plugin.

commented

@dmulloy2 Alright. It seems to not have broken anything.
I'm unsure if I should leave this issue open or not because just adding the packet to the listener causes the crash, it shouldn't require you to make your listener async.
The issue is fixed on my side.

commented

Some packets just have to be handled async because that's how they're handled in NMS. Not much we can do