From 0c481d828dd3eb9b654d8cc50c9e0d24d3220006 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Fri, 3 Aug 2018 02:23:58 -0400 Subject: [PATCH] Remove PluginMessage slicing for now. This was causing leak issues and needs to be reimplemented. If anyone's willing to undertake the work, I will gladly accept the PR! --- .../backend/BackendPlaySessionHandler.java | 26 +++----- .../client/ClientPlaySessionHandler.java | 60 +++++++++---------- .../proxy/protocol/packet/PluginMessage.java | 9 +-- .../protocol/util/PluginMessageUtil.java | 25 ++++---- 4 files changed, 53 insertions(+), 67 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java index a887da94d..bbf407952 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java @@ -45,24 +45,16 @@ public class BackendPlaySessionHandler implements MinecraftSessionHandler { connection.getProxyPlayer().getConnection().write(packet); } else if (packet instanceof PluginMessage) { PluginMessage pm = (PluginMessage) packet; - try { - if (!canForwardPluginMessage(pm)) { - return; - } - - if (PluginMessageUtil.isMCBrand(pm)) { - connection.getProxyPlayer().getConnection().write(PluginMessageUtil.rewriteMCBrand(pm)); - return; - } - - // we'll decrement this twice: once when writing to the server, once just below this block, - // and once in the MinecraftConnection (since this is a slice) - pm.getData().retain(); - - connection.getProxyPlayer().getConnection().write(pm); - } finally { - pm.getData().release(); + if (!canForwardPluginMessage(pm)) { + return; } + + if (PluginMessageUtil.isMCBrand(pm)) { + connection.getProxyPlayer().getConnection().write(PluginMessageUtil.rewriteMCBrand(pm)); + return; + } + + connection.getProxyPlayer().getConnection().write(pm); } else { // Just forward the packet on. We don't have anything to handle at this time. if (packet instanceof ScoreboardTeam || diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java index a3f5904ae..ebf3ae9b4 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/ClientPlaySessionHandler.java @@ -191,46 +191,40 @@ public class ClientPlaySessionHandler implements MinecraftSessionHandler { public void handleClientPluginMessage(PluginMessage packet) { logger.info("Got client plugin message packet {}", packet); - - try { - if (packet.getChannel().equals("REGISTER") || packet.getChannel().equals("minecraft:register")) { - List actuallyRegistered = new ArrayList<>(); - List channels = PluginMessageUtil.getChannels(packet); - for (String channel : channels) { - if (clientPluginMsgChannels.size() >= MAX_PLUGIN_CHANNELS && - !clientPluginMsgChannels.contains(channel)) { - throw new IllegalStateException("Too many plugin message channels registered"); - } - if (clientPluginMsgChannels.add(channel)) { - actuallyRegistered.add(channel); - } + if (packet.getChannel().equals("REGISTER") || packet.getChannel().equals("minecraft:register")) { + List actuallyRegistered = new ArrayList<>(); + List channels = PluginMessageUtil.getChannels(packet); + for (String channel : channels) { + if (clientPluginMsgChannels.size() >= MAX_PLUGIN_CHANNELS && + !clientPluginMsgChannels.contains(channel)) { + throw new IllegalStateException("Too many plugin message channels registered"); } - - if (actuallyRegistered.size() > 0) { - logger.info("Rewritten register packet: {}", actuallyRegistered); - PluginMessage newRegisterPacket = PluginMessageUtil.constructChannelsPacket(packet.getChannel(), actuallyRegistered); - player.getConnectedServer().getMinecraftConnection().write(newRegisterPacket); + if (clientPluginMsgChannels.add(channel)) { + actuallyRegistered.add(channel); } - - return; } - if (packet.getChannel().equals("UNREGISTER") || packet.getChannel().equals("minecraft:unregister")) { - List channels = PluginMessageUtil.getChannels(packet); - clientPluginMsgChannels.removeAll(channels); + if (actuallyRegistered.size() > 0) { + logger.info("Rewritten register packet: {}", actuallyRegistered); + PluginMessage newRegisterPacket = PluginMessageUtil.constructChannelsPacket(packet.getChannel(), actuallyRegistered); + player.getConnectedServer().getMinecraftConnection().write(newRegisterPacket); } - if (PluginMessageUtil.isMCBrand(packet)) { - player.getConnectedServer().getMinecraftConnection().write(PluginMessageUtil.rewriteMCBrand(packet)); - return; - } - - // We're going to forward on the original packet. - packet.getData().retain(); - player.getConnectedServer().getMinecraftConnection().write(packet); - } finally { - ReferenceCountUtil.release(packet.getData()); + return; } + + if (packet.getChannel().equals("UNREGISTER") || packet.getChannel().equals("minecraft:unregister")) { + List channels = PluginMessageUtil.getChannels(packet); + clientPluginMsgChannels.removeAll(channels); + } + + if (PluginMessageUtil.isMCBrand(packet)) { + player.getConnectedServer().getMinecraftConnection().write(PluginMessageUtil.rewriteMCBrand(packet)); + return; + } + + // We're going to forward on the original packet. + player.getConnectedServer().getMinecraftConnection().write(packet); } public void handleServerScoreboardPacket(MinecraftPacket packet) { diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PluginMessage.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PluginMessage.java index 7ff506651..7b8d17f12 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PluginMessage.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/PluginMessage.java @@ -8,7 +8,7 @@ import io.netty.buffer.ByteBufUtil; public class PluginMessage implements MinecraftPacket { private String channel; - private ByteBuf data; + private byte[] data; public String getChannel() { return channel; @@ -18,11 +18,11 @@ public class PluginMessage implements MinecraftPacket { this.channel = channel; } - public ByteBuf getData() { + public byte[] getData() { return data; } - public void setData(ByteBuf data) { + public void setData(byte[] data) { this.data = data; } @@ -37,7 +37,8 @@ public class PluginMessage implements MinecraftPacket { @Override public void decode(ByteBuf buf, ProtocolConstants.Direction direction, int protocolVersion) { this.channel = ProtocolUtils.readString(buf); - this.data = buf.readRetainedSlice(buf.readableBytes()); + this.data = new byte[buf.readableBytes()]; + buf.readBytes(data); } @Override diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/util/PluginMessageUtil.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/util/PluginMessageUtil.java index a71172654..075058486 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/util/PluginMessageUtil.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/util/PluginMessageUtil.java @@ -27,7 +27,7 @@ public enum PluginMessageUtil { message.getChannel().equals("minecraft:register") || message.getChannel().equals("minecraft:unregister"), "Unknown channel type " + message.getChannel()); - String channels = message.getData().toString(StandardCharsets.UTF_8); + String channels = new String(message.getData(), StandardCharsets.UTF_8); return ImmutableList.copyOf(channels.split("\0")); } @@ -37,15 +37,7 @@ public enum PluginMessageUtil { PluginMessage message = new PluginMessage(); message.setChannel(channel); - - ByteBuf data = Unpooled.buffer(); - for (String s : channels) { - ByteBufUtil.writeUtf8(data, s); - data.writeByte(0); - } - data.writerIndex(data.writerIndex() - 1); - - message.setData(data); + message.setData(String.join("\0", channels).getBytes(StandardCharsets.UTF_8)); return message; } @@ -53,13 +45,20 @@ public enum PluginMessageUtil { Preconditions.checkNotNull(message, "message"); Preconditions.checkArgument(isMCBrand(message), "message is not a MC Brand plugin message"); + byte[] rewrittenData; ByteBuf rewrittenBuf = Unpooled.buffer(); - String currentBrand = ProtocolUtils.readString(message.getData()); - ProtocolUtils.writeString(rewrittenBuf, currentBrand + " (Velocity)"); + try { + String currentBrand = ProtocolUtils.readString(Unpooled.wrappedBuffer(message.getData())); + ProtocolUtils.writeString(rewrittenBuf, currentBrand + " (Velocity)"); + rewrittenData = new byte[rewrittenBuf.readableBytes()]; + rewrittenBuf.readBytes(rewrittenData); + } finally { + rewrittenBuf.release(); + } PluginMessage newMsg = new PluginMessage(); newMsg.setChannel(message.getChannel()); - newMsg.setData(rewrittenBuf); + newMsg.setData(rewrittenData); return newMsg; } }