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 potential race conditions when connecting to multiple servers at the same time #3746

Merged
merged 3 commits into from
Oct 6, 2024

Conversation

BoomEaro
Copy link
Contributor

This pull request fixes 2 very rare bugs that cause a race condition when connecting to a server.

  1. It is possible to open multiple connections to the same server at the same time if you call the UserConnection#connect method from different threads.

This pull request fixes this by scheduling the connection to the server in an eventloop.

  1. There is a possibility that the proxy could send a StartConfiguration packet in a situation where the client is still in the Configuration phase, causing the client to disconnect from the proxy with a "CookieRequest" decoding error.
    This most often happens when the proxy opens multiple connections to different servers at the same time.

This pull request fixes this by ensuring that the StartConfiguration packet is only sent when the client is not in the Configuration phase.

…iguration when the connection is in Configuration phase
@BoomEaro
Copy link
Contributor Author

Throwing an IllegalStateException when canceling a ServerConnectEvent often makes no sense. After the LoginEvent completes, the proxy continues to initialize the connection by scheduling a task in the eventloop:

ch.getHandle().eventLoop().execute( new Runnable()
{
@Override
public void run()
{
if ( !ch.isClosing() )
{
userCon = new UserConnection( bungee, ch, getName(), InitialHandler.this );
userCon.setCompressionThreshold( BungeeCord.getInstance().config.getCompressionThreshold() );
if ( getVersion() < ProtocolConstants.MINECRAFT_1_20_2 )
{
unsafe.sendPacket( new LoginSuccess( getRewriteId(), getName(), ( loginProfile == null ) ? null : loginProfile.getProperties() ) );
ch.setProtocol( Protocol.GAME );
}
finish2();
}
}
} );

Making throwing an exception completely pointless, since it won't disconnect the player since it happens outside the context of the handler.
Additionally, it may confuse developers who previously called this method on another thread when it throws such an exception.

@Janmm14
Copy link
Contributor

Janmm14 commented Sep 19, 2024

How is such a wrong state being handled now by the code that a kick from this check is no longer needed?
@BoomEaro

i. e. why remove the check instead of making it a kick?

@BoomEaro
Copy link
Contributor Author

How is such a wrong state being handled now by the code that a kick from this check is no longer needed? @BoomEaro

i. e. why remove the check instead of making it a kick?

Because the logic itself almost never worked correctly it seemed to me that the best solution was not to close the connection if the ServerConnectEvent was cancelled with a null server, allowing the plugins to decide what to do next in such a situation, keeping the player on the proxy server.

Alternatively, we could add a flag to the ServerConnectEvent that would allow the player to remain in this state, but now I'm not sure what the best way to handle this situation is.

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