Skip to content
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.

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

Fix the bug with the lack of search accuracy in the session map. #639

Closed
wants to merge 1 commit into from

Conversation

Rollczi
Copy link

@Rollczi Rollczi commented Oct 28, 2021

Linked bug: TCPShield/RealIP#72 (a bug was found while fixing dynamic player IP changing)

Problem

See ConnectListener.onGameprofileRequest() to know the context

print sessions map
[map key] ? [current ip] -> compare by .equals()

plugin.getSession().forEach((inetSocketAddress, velocityLoginSession) -> {
    System.err.println(inetSocketAddress.toString() + " ? " + event.getConnection().getRemoteAddress().toString() + " -> " + inetSocketAddress.equals(event.getConnection().getRemoteAddress()));
});

result:

 /5.173.249.24:39437 ? /5.173.249.24:30028 -> false
 /5.173.249.24:50033 ? /5.173.249.24:30028 -> false
 /5.173.249.24:39426 ? /5.173.249.24:30028 -> false
 /5.173.249.24:30028 ? /5.173.249.24:30028 -> true <- key is present
 /5.173.249.24:30045 ? /5.173.249.24:30028 -> false
 /5.173.249.24:23523 ? /5.173.249.24:30028 -> false
 /5.173.249.24:50021 ? /5.173.249.24:30028 -> false
 /5.173.249.24:24282 ? /5.173.249.24:30028 -> false
 /5.173.249.24:24256 ? /5.173.249.24:30028 -> false

key is present

error:

[23:10:50] [Velocity Async Event Executor - #0/ERROR]: Couldn't pass GameProfileRequestEvent to fastlogin
java.lang.NullPointerException: Cannot invoke "com.github.games647.fastlogin.core.shared.LoginSession.setUuid(java.util.UUID)" because "session" is null
	at com.github.games647.fastlogin.velocity.listener.ConnectListener.onGameprofileRequest(ConnectListener.java:91) ~[?:?]
	at com.github.games647.fastlogin.velocity.listener.Lmbda$30.execute(Unknown Source) ~[?:?]
	at com.velocitypowered.proxy.event.UntargetedEventHandler$VoidHandler.lambda$buildHandler$0(UntargetedEventHandler.java:47) ~[velocity.jar:3.0.2-SNAPSHOT (git-996ada1f-b82)]
	at com.velocitypowered.proxy.event.VelocityEventManager.fire(VelocityEventManager.java:585) ~[velocity.jar:3.0.2-SNAPSHOT (git-996ada1f-b82)]
	at com.velocitypowered.proxy.event.VelocityEventManager.lambda$fire$5(VelocityEventManager.java:466) ~[velocity.jar:3.0.2-SNAPSHOT (git-996ada1f-b82)]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
	at java.lang.Thread.run(Thread.java:833) [?:?]
LoginSession session = plugin.getSession().get(event.getConnection().getRemoteAddress());
UUID verifiedUUID = event.getGameProfile().getId();
String verifiedUsername = event.getUsername();

session.setUuid(verifiedUUID); <- NPE

but get() method doesn't return the object

@games647
Copy link
Owner

How does your PR fixes this? Wouldn't a changed InetSocketAddress still doesn't match any existing entries in the map even we are no longer invalidating weak/GC collected keys?

BTW: I tried to address this issue in branch tcpshield based for #595. Maybe this fixes it here too. What I like to see more is somehow a way where we actually track the login session and get a notification on disconnect like an event.

@games647 games647 added the bug Something isn't working label Oct 29, 2021
@Rollczi
Copy link
Author

Rollczi commented Oct 29, 2021

@games647
before: a map that contains, for example, the keys 'A', 'B', 'C', 'D', had a chance not to return the object with the get('C') method (Although the "equals() and hashCode()" contract is valid.)
after: the normal hashmap implementation correctly always returns an object if the given key is in the map.

As for the TPCShield plug, it still has a problem with proper IP delivery.
This pull request fixes the plugin's IP handling.

@games647
Copy link
Owner

after: the normal hashmap implementation correctly always returns an object if the given key is in the map.

Yes likely, because that key got garbage collected.

I noticed you changed only the implementation on Velocity? So is this only about Velocity? Your implementation would also cause issues, because the session is also accessed later when switching the server, so the key has still to be valid on that time which could be later than 5 minutes.

There needs to be better solution to this, that uniquely associates a connection to FastLogin session without memory leak/exhaustion issues on bot attacks.

@xism4
Copy link

xism4 commented Jun 1, 2022

@games647 before: a map that contains, for example, the keys 'A', 'B', 'C', 'D', had a chance not to return the object with the get('C') method (Although the "equals() and hashCode()" contract is valid.) after: the normal hashmap implementation correctly always returns an object if the given key is in the map.

As for the TPCShield plug, it still has a problem with proper IP delivery. This pull request fixes the plugin's IP handling.

I toke a look for the pr and seems like a cache login speed lmao xD

@games647
Copy link
Owner

games647 commented Jul 28, 2022

Fixed by access the raw address behind the Spigot API.

@games647 games647 closed this Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants