diff --git a/src/main/java/net/minecraftforge/client/ClientCommandHandler.java b/src/main/java/net/minecraftforge/client/ClientCommandHandler.java index 0c606ea277..46eb383d33 100644 --- a/src/main/java/net/minecraftforge/client/ClientCommandHandler.java +++ b/src/main/java/net/minecraftforge/client/ClientCommandHandler.java @@ -107,7 +107,11 @@ public class ClientCommandHandler { new CommandSource() { @Override public void sendSystemMessage(Component message) { - mc.gui.hud.getChat().addClientSystemMessage(message); + var chat = mc.gui.hud.getChat(); + if (mc.isSameThread()) + chat.addClientSystemMessage(message); + else + mc.execute(() -> chat.addClientSystemMessage(message)); } @Override diff --git a/src/main/java/net/minecraftforge/common/ForgeConfig.java b/src/main/java/net/minecraftforge/common/ForgeConfig.java index d1f19b544c..ac80a22d3b 100644 --- a/src/main/java/net/minecraftforge/common/ForgeConfig.java +++ b/src/main/java/net/minecraftforge/common/ForgeConfig.java @@ -75,6 +75,7 @@ public class ForgeConfig { } public final ForgeConfigSpec.EnumValue migrationHelperMode; + public final ForgeConfigSpec.LongValue channelListLimit; Common(ForgeConfigSpec.Builder builder) { builder.comment("General configuration settings") @@ -85,6 +86,17 @@ public class ForgeConfig { .translation("forge.configgui.migrationHelperMode") .defineEnum("logLegacyTagWarnings", MigrationHelperMode.OFF); + channelListLimit = builder + .comment( + "Maximum size to use to store the channel list for a network connection.", + "The other end of the connection may send as many channels as it wishes.", + "Some malicious actors may use this to use up all your memory causing OutOfMemoryException", + "A sane default is 1KB (1024), but may need to be expanded for larger modpacks.", + "Forge will auto-adjust this if mods register their channels using our channel manager." + ) + .translation("forge.configui.channellistlimit") + .defineInRange("channelListLimit", 1024, 1, Long.MAX_VALUE); + builder.pop(); } } diff --git a/src/main/java/net/minecraftforge/network/ChannelListManager.java b/src/main/java/net/minecraftforge/network/ChannelListManager.java index c3dd34a05f..06f61f6b9d 100644 --- a/src/main/java/net/minecraftforge/network/ChannelListManager.java +++ b/src/main/java/net/minecraftforge/network/ChannelListManager.java @@ -9,9 +9,11 @@ import com.mojang.logging.LogUtils; import net.minecraft.IdentifierException; import net.minecraft.network.Connection; import net.minecraft.network.FriendlyByteBuf; +import net.minecraft.network.chat.Component; import net.minecraft.network.codec.StreamCodec; import net.minecraft.network.protocol.common.custom.CustomPacketPayload; import net.minecraft.resources.Identifier; +import net.minecraftforge.common.ForgeConfig; import net.minecraftforge.event.ForgeEventFactory; import net.minecraftforge.event.network.ChannelRegistrationChangeEvent; import net.minecraftforge.event.network.CustomPayloadEvent; @@ -26,6 +28,7 @@ import java.util.Collection; import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.stream.Collectors; @ApiStatus.Internal // TODO: Decide what I want to have public, Right now it should just be add/remove channels. public class ChannelListManager { @@ -70,7 +73,7 @@ public class ChannelListManager { } private static void register(Register payload, CustomPayloadEvent.Context ctx) { - updateFrom(ctx, payload.channels(), ChannelRegistrationChangeEvent.Type.REGISTER); + updateFrom(ctx, payload.channels(), ChannelRegistrationChangeEvent.Type.REGISTER, ctx.isClientSide()); ctx.setPacketHandled(true); // Send the client's channels to the server whenever asked. If we're in the login state then our login wrapper will unwrap for us. if (ctx.isClientSide()) @@ -78,7 +81,7 @@ public class ChannelListManager { } private static void unregister(Unregister payload, CustomPayloadEvent.Context ctx) { - updateFrom(ctx, payload.channels(), ChannelRegistrationChangeEvent.Type.UNREGISTER); + updateFrom(ctx, payload.channels(), ChannelRegistrationChangeEvent.Type.UNREGISTER, ctx.isClientSide()); ctx.setPacketHandled(true); } @@ -145,7 +148,7 @@ public class ChannelListManager { return Collections.unmodifiableList(channels); } - private static void updateFrom(CustomPayloadEvent.Context source, List channels, final ChannelRegistrationChangeEvent.Type changeType) { + private static void updateFrom(CustomPayloadEvent.Context source, List channels, final ChannelRegistrationChangeEvent.Type changeType, boolean isClient) { var changed = new HashSet(); for (var channel : channels) { // It also says nothing about the format of channels so ignore bad channels. @@ -162,11 +165,20 @@ public class ChannelListManager { ForgeEventFactory.onChannelRegistrationChange(source.getConnection(), changeType, changed); + // It is a known issue, that I don't really care about, that we don't get the true in-memory size of each channel due to character encoding. + // A sane limit of 1KB will be enough to catch any malicious actors even with non-8-bit strings. var list = NetworkContext.get(source.getConnection()); for (var channel : changed) { - boolean fire = changeType == ChannelRegistrationChangeEvent.Type.UNREGISTER - ? list.remoteChannels.remove(channel) - : list.remoteChannels.add(channel); + boolean fire = false; + if (changeType == ChannelRegistrationChangeEvent.Type.UNREGISTER) { + fire = list.remoteChannels.remove(channel); + if (fire) + list.remoteChannelsSize -= channel.toString().length(); + } else { + fire = list.remoteChannels.add(channel); + if (fire) + list.remoteChannelsSize += channel.toString().length(); + } if (fire) { var target = NetworkRegistry.findTarget(channel); @@ -174,5 +186,21 @@ public class ChannelListManager { target.registrationChange(channel, changeType == ChannelRegistrationChangeEvent.Type.REGISTER); } } + + if (list.remoteChannelsSize >= NetworkRegistry.getChannelMemorySize() && list.remoteChannelsSize >= ForgeConfig.COMMON.channelListLimit.get()) { + LOGGER.warn("Received too many network channels, {} channels using {} bytes, Limit is currently {}, to change edit the Forge {} in config\forge_common.toml", + list.remoteChannels.size(), list.remoteChannelsSize, ForgeConfig.COMMON.channelListLimit.get(), ForgeConfig.COMMON.channelListLimit.getPath().stream().collect(Collectors.joining(".")) + ); + + if (isClient) { + // We don't want to allow the server to screw with us, so just remove the added channels + for (var channel : changed) { + if (list.remoteChannels.remove(channel)) + list.remoteChannelsSize -= channel.toString().length(); + } + } else { + source.getSender().connection.disconnect(Component.translatable("forge.server.channellistlimit.kick")); + } + } } } diff --git a/src/main/java/net/minecraftforge/network/NetworkContext.java b/src/main/java/net/minecraftforge/network/NetworkContext.java index a155b96653..631da0992f 100644 --- a/src/main/java/net/minecraftforge/network/NetworkContext.java +++ b/src/main/java/net/minecraftforge/network/NetworkContext.java @@ -111,6 +111,8 @@ public class NetworkContext { @ApiStatus.Internal Set remoteChannels = new HashSet<>(); + @ApiStatus.Internal + long remoteChannelsSize = 0; private Set remoteChannelsView = Collections.unmodifiableSet(this.remoteChannels); @ApiStatus.Internal diff --git a/src/main/java/net/minecraftforge/network/NetworkRegistry.java b/src/main/java/net/minecraftforge/network/NetworkRegistry.java index e1487cec00..6f3dd5cc99 100644 --- a/src/main/java/net/minecraftforge/network/NetworkRegistry.java +++ b/src/main/java/net/minecraftforge/network/NetworkRegistry.java @@ -43,6 +43,7 @@ public class NetworkRegistry { private static Map instances = Collections.synchronizedMap(new HashMap<>()); private static Map byName = Collections.synchronizedMap(new HashMap<>()); + private static long memorySize = 0; public static boolean acceptsVanillaClientConnections() { return listRejectedVanillaMods(n -> n.clientAcceptedVersions).isEmpty() && DataPackRegistriesHooks.getSyncedCustomRegistries().isEmpty(); @@ -199,9 +200,14 @@ public class NetworkRegistry { if (NetworkRegistry.byName.containsKey(name)) error("Payload name " + name + " already registered."); + memorySize += name.toString().length(); // This can fail if people don't use our api. So this is a best effort. byName.put(name, instance); } + static long getChannelMemorySize() { + return memorySize; + } + static void register(NetworkInstance instance) { checkLock(instance); diff --git a/src/main/resources/assets/forge/lang/en_us.json b/src/main/resources/assets/forge/lang/en_us.json index e6b479363e..71543f6f42 100644 --- a/src/main/resources/assets/forge/lang/en_us.json +++ b/src/main/resources/assets/forge/lang/en_us.json @@ -200,6 +200,10 @@ "forge.configgui.stabilizeDirectionGetNearest": "Stabilize Direction Get Nearest", "forge.configgui.stabilizeDirectionGetNearest.tooltip": "When enabled, a slightly biased Direction#getNearest calculation will be used to prevent normal fighting on 45 degree angle faces.", + "forge.configgui.channellistlimit": "Channel List Size Limit", + "forge.configgui.channellistlimit.tooltip": "Maximum size to use to store the channel list for a network connection. Some malicious actors may use this to use up all your memory. A sane default is 1KB (1024), but may need to be expanded for larger modpacks.", + "forge.server.channellistlimit.kick": "Received too many networking channels", + "forge.controlsgui.shift": "SHIFT + %s", "forge.controlsgui.control": "CTRL + %s", "forge.controlsgui.control.mac": "CMD + %s",