PaperMC / PaperMC/Velocity

`ConnectedPlayer#disconnected` called even when connection fails due to duplicate connection

Open
#1,740 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

type: bug
Dominant language
Java
Stars
2.3k
Forks
959
Avg merge
5d 10h
Merged PRs (30d)
2

Description

Expected Behavior

This method should'nt be called.

Actual Behavior

This method does get called, possibly resulting in undesired VelocityBossBarImplementation#viewerDisconnected calls.
For reference, the entire ConnectedPlayer#disconnected method:

public void disconnected() {
  for (final VelocityBossBarImplementation bar : this.bossBars) {
    bar.viewerDisconnected(this);
  }
}
Steps to Reproduce

Add a printline/log statement in ConnectedPlayer#disconnected, join the proxy with minecraft account A, try to re-join the proxy with the same account.
The second connection will fail, and ConnectedPlayer#disconnected still gets called.

Plugin List

n/a

Velocity Version

Compiled from latest commit e0db25664fc82eabd9fde5aac22a2311a9765975 + the log statement in disconnected().

Additional Information

This currently isn't an issue, as bossbars won't be registered to the ConnectedPlayer. However if more logic ever gets added to this method this might be an issue.

My proposed fix is to gate the connection.disconnected() call in VelocityServer#unregisterConnection:

public void unregisterConnection(ConnectedPlayer connection) {
  connectionsByName.remove(connection.getUsername().toLowerCase(Locale.US), connection);
  connectionsByUuid.remove(connection.getUniqueId(), connection);
  connection.disconnected(); // Only call when actually removed from the map(s).
}

However, this may result in unwanted behavior due to VelocityBossBarImplementation storing a Set<ConnectedPlayer> viewers. If a player is added to this list while they haven't been properly registered in VelocityServer yet, this will cause a memory leak. It could be worth it to make sure we only add viewers to bossbars when they've been properly registered (I don't think that's the case here though, because that would mean bossbar packets are being sent before the ServerLoginSuccessPacket, but this is untested).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by inspecting VelocityServer#unregisterConnection and ConnectedPlayer#disconnected, then reproduce the duplicate-account login described in the issue. Trace registration and boss-bar viewer handling to determine when the disconnect callback is valid; done means failed duplicate connections do not trigger it while genuine unregisters still do, without leaving retained viewers.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.