Better ways to handle 113 protocol batchpacket

[yefengeeeeeeeeeee]

No description provided.

[tachibana-kanade-angel]

🤖 AI Review

Summary

This PR adds an early-exit path for legacy protocol 1.1 batch packets in RakNetPlayerSession. The intent is to detect 0xfe-prefixed 1.1 batch packets by checking for the zlib magic bytes 0x78 0xda and process them with raknetProtocol=7. However, the implementation introduces several security and robustness concerns that should be addressed before merging.

Issues Found

Blocking Issues

1. Encryption/checksum bypass — The new 0x78 0xda branch executes before the existing decryption and checksum verification path (lines 144–180). Once decryptionCipher is set, inbound packets should be decrypted and authenticated before batch decoding. This branch allows any zlib-looking plaintext batch to go straight into processBatch(...) and returns early, potentially letting a client bypass checksum validation after encryption is enabled. The legacy fallback should happen only after decrypt/checksum handling, or be explicitly disallowed when encryption is active.

2. IndexOutOfBoundsException on short packets — buffer.getUnsignedByte(1) is called before verifying the buffer has at least 2 bytes. A one-byte frame containing only 0xfe will throw an unhandled exception instead of going through the controlled malformed-packet path. The length check at line 118 (buffer.readableBytes() >= 3) comes too late; the readability check must precede any absolute read.

3. Protocol 113 detection is too broad and brittle — 0x78 0xda is a standard zlib header, not a unique protocol-113 marker. Other zlib-compressed packets (including from non-113 clients) can start with the same bytes, and valid zlib streams may also use headers like 0x78 0x01 or 0x78 0x9c. Forcing raknetProtocol=7 based solely on zlib magic can misdecode packets from modern clients. This fallback should be gated by actual session/protocol state, not magic bytes.

Non-blocking Issues

1. Duplicated malformed-packet handling — The disconnect, delayed close, and address-blocking logic is copy-pasted from the normal path. This increases the risk of future divergence. Consider extracting a helper or routing through a shared method.

2. Misleading variable naming — oldPacketId actually checks zlib header bytes, not a packet ID. The magic numbers 7, 0x78, and 0xda should be named constants or documented clearly.

3. Style — if( / ){ should match the project convention: if (...) {.

Recommendations

• Move the legacy protocol-113 fallback after the decrypt/checksum step, or guard it with a decryptionCipher == null check.
• Add buffer length validation before calling buffer.getUnsignedByte(1).
• Gate the fallback on actual protocol/session state (e.g., a flag set during login for 1.1 clients) rather than zlib magic bytes.
• Add a regression test for empty 0xfe payloads and protocol 113 login/batch decoding.

Notes

• Only one file changed: RakNetPlayerSession.java
• The Chinese comment in the diff (line 118: "1.1的数据包，0xfe后面必定是 0x78 0xda这两个字节") documents the intent but the assumption does not hold for all cases.

---

🤖 AI Review