Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition when handling unknown packets. #3508

Merged
merged 1 commit into from
Jan 5, 2024
Merged

Fix race condition when handling unknown packets. #3508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions ...ent/java/net/fabricmc/fabric/mixin/networking/client/ClientCommonNetworkHandlerMixin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@

package net.fabricmc.fabric.mixin.networking.client;

import org.slf4j.Logger;
import org.spongepowered.asm.mixin.Final;
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;
Expand All @@ -32,6 +35,10 @@

@Mixin(ClientCommonNetworkHandler.class)
public abstract class ClientCommonNetworkHandlerMixin implements NetworkHandlerExtensions {
@Shadow
@Final
private static Logger LOGGER;

@Inject(method = "onCustomPayload(Lnet/minecraft/network/packet/s2c/common/CustomPayloadS2CPacket;)V", at = @At("HEAD"), cancellable = true)
public void onCustomPayload(CustomPayloadS2CPacket packet, CallbackInfo ci) {
if (packet.payload() instanceof ResolvablePayload payload) {
Expand All @@ -45,14 +52,15 @@ public void onCustomPayload(CustomPayloadS2CPacket packet, CallbackInfo ci) {
throw new IllegalStateException("Unknown network addon");
}

if (handled) {
ci.cancel();
} else if (payload instanceof RetainedPayload retained && retained.buf().refCnt() > 0) {
// Vanilla forces to use the render thread for its payloads,
// that means this method can get called multiple times.
if (!handled && payload instanceof RetainedPayload retained && retained.buf().refCnt() > 0) {
// Duplicate the vanilla log message, as we cancel further processing.
LOGGER.warn("Unknown custom packet payload: {}", payload.id());

retained.buf().skipBytes(retained.buf().readableBytes());
retained.buf().release();
}

ci.cancel();
}
}
}
29 changes: 28 additions & 1 deletion ...1/src/testmod/java/net/fabricmc/fabric/test/networking/play/NetworkingPlayPacketTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static net.minecraft.server.command.CommandManager.literal;

import java.util.List;
import java.util.UUID;

import com.mojang.brigadier.Command;
import com.mojang.brigadier.CommandDispatcher;
Expand All @@ -38,6 +39,7 @@

import net.fabricmc.api.ModInitializer;
import net.fabricmc.fabric.api.command.v2.CommandRegistrationCallback;
import net.fabricmc.fabric.api.event.lifecycle.v1.ServerTickEvents;
import net.fabricmc.fabric.api.networking.v1.FabricPacket;
import net.fabricmc.fabric.api.networking.v1.PacketByteBufs;
import net.fabricmc.fabric.api.networking.v1.PacketType;
Expand All @@ -47,6 +49,7 @@
public final class NetworkingPlayPacketTest implements ModInitializer {
public static final Identifier TEST_CHANNEL = NetworkingTestmods.id("test_channel");
private static final Identifier UNKNOWN_TEST_CHANNEL = NetworkingTestmods.id("unknown_test_channel");
private static boolean spamUnknownPackets = false;

public static void sendToTestChannel(ServerPlayerEntity player, String stuff) {
ServerPlayNetworking.getSender(player).sendPacket(new OverlayPacket(Text.literal(stuff)), future -> {
Expand All @@ -55,7 +58,13 @@ public static void sendToTestChannel(ServerPlayerEntity player, String stuff) {
}

private static void sendToUnknownChannel(ServerPlayerEntity player) {
ServerPlayNetworking.getSender(player).sendPacket(UNKNOWN_TEST_CHANNEL, PacketByteBufs.create());
PacketByteBuf buf = PacketByteBufs.create();

for (int i = 0; i < 20; i++) {
buf.writeUuid(UUID.randomUUID());
}

ServerPlayNetworking.getSender(player).sendPacket(UNKNOWN_TEST_CHANNEL, buf);
}

public static void registerCommand(CommandDispatcher<ServerCommandSource> dispatcher) {
Expand All @@ -71,6 +80,11 @@ public static void registerCommand(CommandDispatcher<ServerCommandSource> dispat
sendToUnknownChannel(ctx.getSource().getPlayer());
return Command.SINGLE_SUCCESS;
}))
.then(literal("spamUnknown").executes(ctx -> {
spamUnknownPackets = true;
ctx.getSource().sendMessage(Text.literal("Spamming unknown packets state:" + spamUnknownPackets));
return Command.SINGLE_SUCCESS;
}))
.then(literal("bufctor").executes(ctx -> {
PacketByteBuf buf = PacketByteBufs.create();
buf.writeIdentifier(TEST_CHANNEL);
Expand Down Expand Up @@ -106,6 +120,19 @@ public void onInitialize() {
CommandRegistrationCallback.EVENT.register((dispatcher, registryAccess, environment) -> {
NetworkingPlayPacketTest.registerCommand(dispatcher);
});

ServerTickEvents.START_SERVER_TICK.register(server -> {
if (!spamUnknownPackets) {
return;
}

// Send many unknown packets, used to debug https://github.com/FabricMC/fabric/issues/3505
for (ServerPlayerEntity player : server.getPlayerManager().getPlayerList()) {
for (int i = 0; i < 50; i++) {
sendToUnknownChannel(player);
}
}
});
}

public record OverlayPacket(Text message) implements FabricPacket {
Expand Down
Loading