Player kicked while using BungeeCord and TrMenu
diogotcorreia opened this issue ยท 7 comments
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:
- Download the latest version of BungeeCord (
git:BungeeCord-Bootstrap:1.16-R0.4-SNAPSHOT:9c078b7:1540
) - Clone the TrMenu repository and build the plugin (tested on v2.08).
- Start a BungeeCord server (no plugins)
- 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. - Open the default menu with
/trmenu open Example
. - Click anywhere on the GUI.
- 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
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.
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.
In that case, the plugin likely needs to run their listener async. You can set ListenerOptions.ASYNC
in the listener
@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)
@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.
@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.