fix: retain reference-counted packets forwarded via handleGeneric (#1856)

During configuration, a ServerboundCustomClickActionPacket arriving
when connectionInFlight is null falls through to handleGeneric, which
writes it to the connected backend without retaining. The encoder
releases the packet, then MinecraftConnection.channelRead's finally
block releases again - double-free.

Two fixes:
- handle() now uses getConnectionInFlightOrConnectedServer() so the
  packet is properly retained before being written
- handleGeneric() retains any ByteBufHolder packet before write, not
  just PluginMessagePacket

Closes #1841
This commit is contained in:
Radmir Noirusov
2026-08-12 18:29:30 +01:00
committed by GitHub
parent 71c50a75eb
commit 14a69904f9
2 changed files with 125 additions and 4 deletions
@@ -47,6 +47,7 @@ import com.velocitypowered.proxy.protocol.packet.config.FinishedUpdatePacket;
import com.velocitypowered.proxy.protocol.packet.config.KnownPacksPacket; import com.velocitypowered.proxy.protocol.packet.config.KnownPacksPacket;
import com.velocitypowered.proxy.protocol.util.PluginMessageUtil; import com.velocitypowered.proxy.protocol.util.PluginMessageUtil;
import io.netty.buffer.ByteBuf; import io.netty.buffer.ByteBuf;
import io.netty.buffer.ByteBufHolder;
import io.netty.buffer.ByteBufUtil; import io.netty.buffer.ByteBufUtil;
import io.netty.buffer.Unpooled; import io.netty.buffer.Unpooled;
import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletableFuture;
@@ -212,8 +213,9 @@ public class ClientConfigSessionHandler implements MinecraftSessionHandler {
@Override @Override
public boolean handle(ServerboundCustomClickActionPacket packet) { public boolean handle(ServerboundCustomClickActionPacket packet) {
if (player.getConnectionInFlight() != null) { VelocityServerConnection serverConnection = player.getConnectionInFlightOrConnectedServer();
player.getConnectionInFlight().ensureConnected().write(packet.retain()); if (serverConnection != null) {
serverConnection.ensureConnected().write(packet.retain());
return true; return true;
} }
@@ -240,8 +242,8 @@ public class ClientConfigSessionHandler implements MinecraftSessionHandler {
MinecraftConnection smc = serverConnection.getConnection(); MinecraftConnection smc = serverConnection.getConnection();
if (smc != null && serverConnection.getPhase().consideredComplete()) { if (smc != null && serverConnection.getPhase().consideredComplete()) {
if (packet instanceof PluginMessagePacket) { if (packet instanceof ByteBufHolder bufHolder) {
((PluginMessagePacket) packet).retain(); bufHolder.retain();
} }
smc.write(packet); smc.write(packet);
} }
@@ -0,0 +1,119 @@
/*
* Copyright (C) 2018-2026 Velocity Contributors
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*/
package com.velocitypowered.proxy.connection.client;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import com.velocitypowered.proxy.VelocityServer;
import com.velocitypowered.proxy.connection.MinecraftConnection;
import com.velocitypowered.proxy.connection.backend.BackendConnectionPhase;
import com.velocitypowered.proxy.connection.backend.VelocityServerConnection;
import com.velocitypowered.proxy.protocol.packet.ServerboundCustomClickActionPacket;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.netty.util.ReferenceCountUtil;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
class ClientConfigSessionHandlerTest {
private VelocityServer server;
private ConnectedPlayer player;
private ClientConfigSessionHandler handler;
@BeforeEach
void setUp() {
server = mock(VelocityServer.class);
player = mock(ConnectedPlayer.class);
handler = new ClientConfigSessionHandler(server, player);
}
@AfterEach
void tearDown() {
// nothing to clean up; each test manages its own ByteBufs
}
private ServerboundCustomClickActionPacket makePacket() {
ByteBuf frame = Unpooled.buffer().writeByte(0);
ServerboundCustomClickActionPacket pkt = new ServerboundCustomClickActionPacket();
pkt.replace(frame.readRetainedSlice(frame.readableBytes()));
return pkt;
}
@Test
void handleForwardsToInFlightServer() {
VelocityServerConnection inFlight = mock(VelocityServerConnection.class);
MinecraftConnection backend = mock(MinecraftConnection.class);
when(player.getConnectionInFlightOrConnectedServer()).thenReturn(inFlight);
when(inFlight.ensureConnected()).thenReturn(backend);
ServerboundCustomClickActionPacket pkt = makePacket();
assertTrue(handler.handle(pkt));
verify(backend).write(pkt);
ReferenceCountUtil.release(pkt);
}
@Test
void handleForwardsToConnectedServerWhenInFlightIsNull() {
VelocityServerConnection connected = mock(VelocityServerConnection.class);
MinecraftConnection backend = mock(MinecraftConnection.class);
when(player.getConnectionInFlightOrConnectedServer()).thenReturn(connected);
when(connected.ensureConnected()).thenReturn(backend);
ServerboundCustomClickActionPacket pkt = makePacket();
assertTrue(handler.handle(pkt));
verify(backend).write(pkt);
ReferenceCountUtil.release(pkt);
}
@Test
void handleReturnsFalseWhenNoServer() {
when(player.getConnectionInFlightOrConnectedServer()).thenReturn(null);
ServerboundCustomClickActionPacket pkt = makePacket();
assertFalse(handler.handle(pkt));
ReferenceCountUtil.release(pkt);
}
@Test
void handleGenericRetainsAndForwards() {
VelocityServerConnection connected = mock(VelocityServerConnection.class);
MinecraftConnection backend = mock(MinecraftConnection.class);
BackendConnectionPhase phase = mock(BackendConnectionPhase.class);
when(player.getConnectedServer()).thenReturn(connected);
when(connected.getConnection()).thenReturn(backend);
when(connected.getPhase()).thenReturn(phase);
when(phase.consideredComplete()).thenReturn(true);
ServerboundCustomClickActionPacket pkt = makePacket();
int refBefore = pkt.refCnt();
handler.handleGeneric(pkt);
// retain() was called (+1) before write
assertEquals(refBefore + 1, pkt.refCnt());
verify(backend).write(pkt);
ReferenceCountUtil.release(pkt);
}
}