TCPShield / RealIP

The Spigot, Bungee and Velocity plugin that parses client IP addresses passed from the TCPShield network.
https://tcpshield.com
MIT License
145 stars 52 forks source link

Fixes searchFieldByClass looping forever and incorrect cache keys #16

Closed JasperJH closed 3 years ago

JasperJH commented 3 years ago

This addresses two issues in the ReflectionUtils#searchFieldByClass method:

paulzhng commented 3 years ago

Reaching the infinite loop actually isn't possible; testing for that case is already provided: https://github.com/TCPShield/RealIP/blob/d24a6a1a99103c91434b55014d0ffdfad14f197d/src/test/java/net/tcpshield/tcpshield/ReflectionUtilsTest.java#L52 Cache issue addressed in commit e859a1c.

Thanks for reaching out! :)

JasperJH commented 3 years ago

Reaching the infinite loop actually isn't possible; testing for that case is already provided:

https://github.com/TCPShield/RealIP/blob/d24a6a1a99103c91434b55014d0ffdfad14f197d/src/test/java/net/tcpshield/tcpshield/ReflectionUtilsTest.java#L52

Cache issue addressed in commit e859a1c. Thanks for reaching out! :)

That test only tests one specific case: a class that implicitly extends Object. However, if clazz extends another class explicitly (such as the NetworkManager class for the ProtocolLib version which extends SimpleChannelInboundHandler) then ReflectionUtils#searchFieldByClass will keep looping forever if it cannot find the field in the first two classes. This happens because currentClass will first be equal to clazz (=NetworkManager) and if the field is not found inside of this class then it will keep checking clazz.getSuperClass() (=SimpleChannelInboundHandler) forever.

Now I highly doubt that this will cause issues any time soon because this can (currently) only happen with the ProtocolLib implementation and then only if the SocketAddress field in that class is moved to another class / wrapped inside of another object which will most likely never happen. The other implementations don't call that method using a class that has an explicit superclass or don't make use this method at all.

So even though it is highly unlikely it is definitely possible to reach an infinite loop.

paulzhng commented 3 years ago

Actually, that possibility wasn't as unlikely as you said. Although I am still not aware why, sometimes the server uses NetworkManagerServer instead of NetworkManager. Your PR just fixed a bug, thanks a lot 👍