Skip to content

fix async packet type logic, buffer slicing, and null/out-of-bounds guards #3655

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

Open
Spagles wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix async packet type logic, buffer slicing, and null/out-of-bounds guards #3655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d2d502b
fix inverted sending and receiving types in async filter manager
Spagles Aug 3, 2026
65ea689
fix buffer byte array extraction in stream serializer
Spagles Aug 3, 2026
b109988
add bounds check to IntegerMap remove method
Spagles Aug 3, 2026
c98fb95
guard against null player in PlayerSendingHandler queue retrieval
Spagles Aug 3, 2026
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
4 changes: 2 additions & 2 deletions src/main/java/com/comphenix/protocol/async/AsyncFilterManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,12 +323,12 @@ public synchronized void enqueueSyncPacket(PacketEvent syncPacket, AsyncMarker a

@Override
public Set<PacketType> getReceivingTypes() {
return serverProcessingQueue.keySet();
return clientProcessingQueue.keySet();
}

@Override
public Set<PacketType> getSendingTypes() {
return clientProcessingQueue.keySet();
return serverProcessingQueue.keySet();

@dmulloy2 dmulloy2 Aug 3, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mapping correction is right, but no existing test distinguishes the old inverted behavior. Please add a regression test with a sending-only whitelist and assert that its type appears in getSendingTypes() and not getReceivingTypes().

}

/**
Expand Down
8 changes: 6 additions & 2 deletions src/main/java/com/comphenix/protocol/async/PlayerSendingHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,14 +134,18 @@ public PacketSendingQueue getSendingQueue(PacketEvent packet) {
* @return The server or client sending queue the packet belongs to.
*/
public PacketSendingQueue getSendingQueue(PacketEvent packet, boolean createNew) {
QueueContainer queues = playerSendingQueues.get(packet.getPlayer());
Player player = packet.getPlayer();
if (player == null)

@dmulloy2 dmulloy2 Aug 3, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not cover the reported login/status failure: those events normally have a non-null TemporaryPlayer. The remaining reachable NPE is when ConcurrentPlayerMap.cachePlayerKey derives a null address and inserts it into the Guava cache. Please guard the derived key there. Keeping the local player variable is still useful because it removes the weak-reference TOCTOU between the lookup and putIfAbsent.

return null;

QueueContainer queues = playerSendingQueues.get(player);

// Safe concurrent initialization
if (queues == null && createNew) {
final QueueContainer newContainer = new QueueContainer();

// Attempt to map the queue
queues = playerSendingQueues.putIfAbsent(packet.getPlayer(), newContainer);
queues = playerSendingQueues.putIfAbsent(player, newContainer);

if (queues == null) {
queues = newContainer;
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/comphenix/protocol/collections/IntegerMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public T put(int key, T value) {
* @return The old associated value, or NULL.
*/
public T remove(int key) {
if (key < 0 || key >= array.length) return null;

@dmulloy2 dmulloy2 Aug 3, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add regression coverage for negative and oversized keys, asserting remove() returns null and leaves size() unchanged. There currently are no IntegerMap.remove() bounds tests, so this can regress without detection.

T old = array[key];
array[key] = null;

Expand Down
12 changes: 3 additions & 9 deletions src/main/java/com/comphenix/protocol/utility/StreamSerializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,15 +326,9 @@ public void serializeItemStack(DataOutputStream output, ItemStack stack) throws

public byte[] getBytesAndRelease(ByteBuf buf) {
try {
if (buf.hasArray()) {
// heap buffer, we can access the array directly
return buf.array();
} else {
// direct buffer, we need to copy the bytes into an array
byte[] bytes = new byte[buf.readableBytes()];
buf.readBytes(bytes);
return bytes;
}
byte[] bytes = new byte[buf.readableBytes()];
buf.readBytes(bytes);

@dmulloy2 dmulloy2 Aug 3, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readBytes advances the caller's readerIndex, while the previous heap-buffer path left it unchanged. Since this is a public method, that is an observable compatibility change for callers that retain/reuse the buffer. Could we use ByteBufUtil.getBytes(buf) instead? It returns the exact readable bytes, handles direct buffers and array offsets, and preserves the indices. Please also add a regression assertion for both exact output length and unchanged readerIndex.

return bytes;
} finally {
ReferenceCountUtil.safeRelease(buf);
}
Expand Down
Loading