fix async packet type logic, buffer slicing, and null/out-of-bounds guards - #3655
fix async packet type logic, buffer slicing, and null/out-of-bounds guards#3655Spagles wants to merge 4 commits into
Conversation
dmulloy2
left a comment
dmulloy2
left a comment
There was a problem hiding this comment.
Review findings are attached inline.
| return bytes; | ||
| } | ||
| byte[] bytes = new byte[buf.readableBytes()]; | ||
| buf.readBytes(bytes); |
There was a problem hiding this comment.
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.
| public PacketSendingQueue getSendingQueue(PacketEvent packet, boolean createNew) { | ||
| QueueContainer queues = playerSendingQueues.get(packet.getPlayer()); | ||
| Player player = packet.getPlayer(); | ||
| if (player == null) |
There was a problem hiding this comment.
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.
| @Override | ||
| public Set<PacketType> getSendingTypes() { | ||
| return clientProcessingQueue.keySet(); | ||
| return serverProcessingQueue.keySet(); |
There was a problem hiding this comment.
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().
| * @return The old associated value, or NULL. | ||
| */ | ||
| public T remove(int key) { | ||
| if (key < 0 || key >= array.length) return null; |
There was a problem hiding this comment.
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.
2 participants
getSendingTypes()andgetReceivingTypes()inAsyncFilterManagerStreamSerializer.getBytesAndRelease()returning un-trimmed backing arrays with trailing capacity bytesIntegerMap.remove()to preventArrayIndexOutOfBoundsExceptionPlayerSendingHandler.getSendingQueue()to preventNullPointerExceptionon login/status packets