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 possible NPE when trying to get encoder protocol #3760

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BoomEaro
Copy link
Contributor

This pull request fixes a rare race condition where the proxy can try to send a packet to a queue when the connection to the player has already been closed and netty has managed to clear all handlers, including MinecraftEncoder and MinecraftDecoder, causing a NullPointerException.

This can usually happen when a player who is already trying to connect to another server unexpectedly disconnects from the proxy, and when the server the player was connecting to kicks the player, which in rare cases causes a connection error message to be sent to the player whose connection is already closed.

The solution to the problem seems simple: Just return null for those methods that return Protocol if there are no handlers.

@NEZNAMY
Copy link

NEZNAMY commented Oct 28, 2024

I can confirm that this issue is indeed happening currently, even when using the official API. The PR looks good.

@Outfluencer
Copy link
Collaborator

i would just check

if ch.isClosed() { return }

in serverconnection and userconnection #sendPacketQueued

@BoomEaro
Copy link
Contributor Author

i would just check

if ch.isClosed() { return }

in serverconnection and userconnection #sendPacketQueued

I don't think this will fix this problem.
It may happen that isClosed will return false at first, but then at the same time the channel will close and the handlers will be removed from netty and all this in the same context while the sendPacketQueued method was being executed.
It looks like this is exactly what is happening, since I looked at the source code of the Tab plugin from NEZNAMY, there is already such a check and it does not change the situation in any way.
In other words, there will be another race condition where isClosed can become outdated while this method is being executed.

@Outfluencer
Copy link
Collaborator

Outfluencer commented Oct 28, 2024

everything can be an racecondition for this specific case, as it can be invoked indirect via api (that can be executed async). I think just wrap it with an eventloop execute is the most secure way

edit: and check if the connection is already closed inside the eventloop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants