Fix Fabric-caused networking ByteBuf leaks ()

A vanilla-caused leak remains, which appears to be a new take on an old Mojang bug: https://bugs.mojang.com/browse/MC-121884?focusedCommentId=548927&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-548927
This commit is contained in:
Adrian Siekierka 2019-05-19 21:16:45 +02:00 committed by GitHub
parent 7861737634
commit 200eb5c2ac
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 84 additions and 37 deletions

View file

@ -1,5 +1,5 @@
archivesBaseName = "fabric-networking-v0"
version = getSubprojectVersion(project, "0.1.1")
version = getSubprojectVersion(project, "0.1.2")
dependencies {
compile project(path: ':fabric-api-base', configuration: 'dev')

View file

@ -25,13 +25,17 @@ import net.minecraft.util.PacketByteBuf;
public interface PacketConsumer {
/**
* Receive a CustomPayload-based packet.
* <p>
*
* The PacketByteBuf received will be released as soon as the method exits,
* meaning that you have to call .retain()/.release() on it if you want to
* keep it around after that.
*
* Please keep in mind that this CAN be called OUTSIDE of the main thread!
* Most game operations are not thread-safe, so you should look into using
* the thread task queue ({@link PacketContext#getTaskQueue()}) to split
* the "reading" (which should, but does not have to, happen off-thread)
* the "reading" (which should happen within this method's execution)
* and "applying" (which, unless you know what you're doing, should happen
* on the main thread).
* on the main thread, after this method exits).
*
* @param context The context (receiving player, side, etc.)
* @param buffer The byte buffer containing the received packet data.

View file

@ -95,8 +95,4 @@ public class ClientSidePacketRegistryImpl extends PacketRegistryImpl implements
protected void onReceivedUnregisterPacket(PacketContext context, Collection<Identifier> ids) {
S2CPacketTypeCallback.UNREGISTERED.invoker().accept(ids);
}
public final boolean accept(CustomPayloadS2CPacket packet, PacketContext context) {
return accept(packet.getChannel(), context, packet.getData());
}
}

View file

@ -0,0 +1,25 @@
/*
* Copyright (c) 2016, 2017, 2018, 2019 FabricMC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package net.fabricmc.fabric.impl.network;
public final class PacketDebugOptions {
public static final boolean DISABLE_BUFFER_RELEASES = System.getProperty("fabric.networking.broken.disableBufferReleases", "false").equalsIgnoreCase("true");
private PacketDebugOptions() {
}
}

View file

@ -29,6 +29,7 @@ import org.apache.logging.log4j.Logger;
import java.nio.charset.StandardCharsets;
import java.util.*;
import java.util.function.Supplier;
public abstract class PacketRegistryImpl implements PacketRegistry {
protected static final Logger LOGGER = LogManager.getLogger();
@ -96,32 +97,35 @@ public abstract class PacketRegistryImpl implements PacketRegistry {
return Optional.of(toPacket(id, buf));
}
private boolean acceptRegisterType(Identifier id, PacketContext context, PacketByteBuf buf) {
private boolean acceptRegisterType(Identifier id, PacketContext context, Supplier<PacketByteBuf> bufSupplier) {
Collection<Identifier> ids = new HashSet<>();
{
StringBuilder sb = new StringBuilder();
char c;
PacketByteBuf buf = bufSupplier.get();
int oldIndex = buf.readerIndex();
while (buf.readerIndex() < buf.writerIndex()) {
c = (char) buf.readByte();
if (c == 0) {
String s = sb.toString();
if (!s.isEmpty()) {
try {
ids.add(new Identifier(s));
} catch (InvalidIdentifierException e) {
LOGGER.warn("Received invalid identifier in " + id + ": " + s + " (" + e.getLocalizedMessage() + ")");
LOGGER.trace(e);
try {
while (buf.readerIndex() < buf.writerIndex()) {
c = (char) buf.readByte();
if (c == 0) {
String s = sb.toString();
if (!s.isEmpty()) {
try {
ids.add(new Identifier(s));
} catch (InvalidIdentifierException e) {
LOGGER.warn("Received invalid identifier in " + id + ": " + s + " (" + e.getLocalizedMessage() + ")");
LOGGER.trace(e);
}
}
sb = new StringBuilder();
} else {
sb.append(c);
}
sb = new StringBuilder();
} else {
sb.append(c);
}
} finally {
buf.release();
}
buf.readerIndex(oldIndex);
String s = sb.toString();
if (!s.isEmpty()) {
@ -148,22 +152,31 @@ public abstract class PacketRegistryImpl implements PacketRegistry {
/**
* Hook for accepting packets used in Fabric mixins.
*
* As PacketByteBuf getters in vanilla create a copy (to allow releasing the original packet buffer without
* breaking other, potentially delayed accesses), we use a Supplier to generate those copies and release them
* when needed.
*
* @param id The packet Identifier received.
* @param context The packet context provided.
* @param buf The packet data buffer received.
* @param bufSupplier A supplier creating a new PacketByteBuf.
* @return Whether or not the packet was handled by this packet registry.
*/
public boolean accept(Identifier id, PacketContext context, PacketByteBuf buf) {
public boolean accept(Identifier id, PacketContext context, Supplier<PacketByteBuf> bufSupplier) {
if (id.equals(PacketTypes.REGISTER) || id.equals(PacketTypes.UNREGISTER)) {
return acceptRegisterType(id, context, buf);
return acceptRegisterType(id, context, bufSupplier);
}
PacketConsumer consumer = consumerMap.get(id);
if (consumer != null) {
PacketByteBuf buf = bufSupplier.get();
try {
consumer.accept(context, buf);
} catch (Throwable t) {
LOGGER.warn("Failed to handle packet " + id + "!", t);
} finally {
if (buf.refCnt() > 0 && !PacketDebugOptions.DISABLE_BUFFER_RELEASES) {
buf.release();
}
}
return true;
} else {

View file

@ -109,9 +109,4 @@ public class ServerSidePacketRegistryImpl extends PacketRegistryImpl implements
protected void onReceivedUnregisterPacket(PacketContext context, Collection<Identifier> ids) {
C2SPacketTypeCallback.UNREGISTERED.invoker().accept(context.getPlayer(), ids);
}
public final boolean accept(CustomPayloadC2SPacket packet, PacketContext context) {
CustomPayloadC2SPacketAccessor accessor = ((CustomPayloadC2SPacketAccessor) packet);
return accept(accessor.getChannel(), context, accessor.getData());
}
}

View file

@ -20,6 +20,7 @@ import net.fabricmc.api.EnvType;
import net.fabricmc.fabric.api.network.ClientSidePacketRegistry;
import net.fabricmc.fabric.api.network.PacketContext;
import net.fabricmc.fabric.impl.network.ClientSidePacketRegistryImpl;
import net.fabricmc.fabric.impl.network.PacketDebugOptions;
import net.fabricmc.fabric.impl.network.PacketRegistryImpl;
import net.fabricmc.fabric.impl.network.PacketTypes;
import net.minecraft.client.MinecraftClient;
@ -28,12 +29,15 @@ import net.minecraft.client.network.packet.CustomPayloadS2CPacket;
import net.minecraft.client.network.packet.GameJoinS2CPacket;
import net.minecraft.entity.player.PlayerEntity;
import net.minecraft.network.Packet;
import net.minecraft.util.Identifier;
import net.minecraft.util.PacketByteBuf;
import net.minecraft.util.ThreadExecutor;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Shadow;
import org.spongepowered.asm.mixin.injection.At;
import org.spongepowered.asm.mixin.injection.Inject;
import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;
import org.spongepowered.asm.mixin.injection.callback.LocalCapture;
import java.util.Optional;
@ -55,16 +59,20 @@ public abstract class MixinClientPlayNetworkHandler implements PacketContext {
}
// Optional hook: it only removes a warning message.
@Inject(method = "onCustomPayload", at = @At(value = "CONSTANT", args = "stringValue=Unknown custom packed identifier: {}"), cancellable = true, require = 0)
public void onCustomPayloadNotFound(CustomPayloadS2CPacket packet, CallbackInfo info) {
@Inject(method = "onCustomPayload", at = @At(value = "CONSTANT", args = "stringValue=Unknown custom packed identifier: {}"), cancellable = true, locals = LocalCapture.CAPTURE_FAILSOFT, require = 0)
public void onCustomPayloadNotFound(CustomPayloadS2CPacket packet, CallbackInfo info, Identifier id, PacketByteBuf buf) {
if (packet.getChannel().equals(PacketTypes.REGISTER) || packet.getChannel().equals(PacketTypes.UNREGISTER)) {
if (buf.refCnt() > 0) {
buf.release();
}
info.cancel();
}
}
@Inject(method = "onCustomPayload", at = @At("HEAD"), cancellable = true)
public void onCustomPayload(CustomPayloadS2CPacket packet, CallbackInfo info) {
if (((ClientSidePacketRegistryImpl) ClientSidePacketRegistry.INSTANCE).accept(packet, this)) {
if (((ClientSidePacketRegistryImpl) ClientSidePacketRegistry.INSTANCE).accept(packet.getChannel(), this, packet::getData)) {
info.cancel();
}
}

View file

@ -37,6 +37,6 @@ public class MixinCustomPayloadC2SPacket implements CustomPayloadC2SPacketAccess
@Override
public PacketByteBuf getData() {
return data;
return new PacketByteBuf(this.data.copy());
}
}

View file

@ -19,12 +19,16 @@ package net.fabricmc.fabric.mixin.network;
import net.fabricmc.api.EnvType;
import net.fabricmc.fabric.api.network.PacketContext;
import net.fabricmc.fabric.api.network.ServerSidePacketRegistry;
import net.fabricmc.fabric.impl.network.CustomPayloadC2SPacketAccessor;
import net.fabricmc.fabric.impl.network.PacketDebugOptions;
import net.fabricmc.fabric.impl.network.ServerSidePacketRegistryImpl;
import net.minecraft.entity.player.PlayerEntity;
import net.minecraft.server.MinecraftServer;
import net.minecraft.server.network.ServerPlayNetworkHandler;
import net.minecraft.server.network.ServerPlayerEntity;
import net.minecraft.server.network.packet.CustomPayloadC2SPacket;
import net.minecraft.util.Identifier;
import net.minecraft.util.PacketByteBuf;
import net.minecraft.util.ThreadExecutor;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Shadow;
@ -41,7 +45,9 @@ public class MixinServerPlayNetworkHandler implements PacketContext {
@Inject(method = "onCustomPayload", at = @At("HEAD"), cancellable = true)
public void onCustomPayload(CustomPayloadC2SPacket packet, CallbackInfo info) {
if (((ServerSidePacketRegistryImpl) ServerSidePacketRegistry.INSTANCE).accept(packet, this)) {
Identifier channel = ((CustomPayloadC2SPacketAccessor) packet).getChannel();
if (((ServerSidePacketRegistryImpl) ServerSidePacketRegistry.INSTANCE).accept(channel, this, ((CustomPayloadC2SPacketAccessor) packet)::getData)) {
info.cancel();
}
}