Skip to content

minecraft: Revert deferred packet re-check, expect ResourcePacksInfo with LoginSuccess; fix pong game type fields - #527

Merged
TwistedAsylumMC merged 5 commits into
Sandertv:masterfrom
HashimTheArab:fix/post-merge-review
Sep 26, 2026
Merged

TwistedAsylumMC merged 5 commits into
Sandertv:masterfrom
HashimTheArab:fix/post-merge-review

Conversation

@HashimTheArab

@HashimTheArab HashimTheArab commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to yesterday's merges (#406 and #515). Two independent fixes.

Revert the deferred packet re-check (#406); expect ResourcePacksInfo with LoginSuccess

#406 made expect() run every deferred packet through handle() on the calling goroutine. That breaks two assumptions the login handlers are built on:

  • A handler runs after the state change that expected it is complete. handleLogin calls expect(ClientToServerHandshake) before enableEncryption, so a handshake that was already deferred was answered inside expect(): the server wrote PlayStatus(LoginSuccess) and ResourcePacksInfo and marked the connection authenticated before ServerToClientHandshake was written. Reproduced against master with a Conn over net.Pipe: wire order [PlayStatus, ResourcePacksInfo, ServerToClientHandshake]. handleRequestNetworkSettings has the same shape (expect(Login) before NetworkSettings is written and compression enabled).
  • Handlers run on the receive goroutine only. StartGame, Dial and the pack download goroutine call expect(), so a matching deferred packet had its handler run there, concurrently with the receive loop (plain-field writes to readyToLogin, loggedIn, gameData, the decoder's compression state), and listenConn/handleConn only observe readyToLogin/loggedIn transitions around their own receive() call, so a transition made in another goroutine's drain is never signalled.

I first tried to keep the re-check and schedule it safely (flag on the receive loop, then a handler mutex). Each variant fixed those and opened another hole: a packet deferred during login stranded until the next inbound packet, spawn packets written before StartGame, a lost wake-up on the lock. The mechanism is the problem, so this reverts it.

The case #406 targeted does not need it. The vanilla client handles ResourcePacksInfo whenever it arrives and reacts to PlayStatus(LoginSuccess) only by sending ClientCacheStatus, so this expects both after Login is sent (and after ServerToClientHandshake) and LoginSuccess no longer narrows the set to ResourcePacksInfo. A server that sends ResourcePacksInfo first is handled on arrival, and PlayStatus stays expected through the pack phase so the LoginSuccess that follows still sends ClientCacheStatus. startGame now expects the client's replies before writing StartGame, which closes the pre-#406 gap where a fast reply could be deferred. The vanilla client only sends RequestChunkRadius from its StartGame handler, so nothing earlier than that needs handling; the pre-StartGame window in #462 is a separate path (read channel, not the deferred queue) and is untouched.

Checked locally (net.Pipe harness, not committed): Info before LoginSuccess enters the pack phase and a late LoginSuccess sends ClientCacheStatus without resetting it; normal order unchanged; an early plaintext handshake stays a stray and only ServerToClientHandshake follows Login; spawn packets follow StartGame/ItemRegistry; a Listen/Dial login and spawn round-trip over RakNet passes under -race.

Server list pong game type fields (#515)

The field after the game mode is a separate flag, not the game mode number: the client drops a pong with ten or more fields unless that field is exactly 1 (Cloudburst serialises it separately as nintendoLimited), so a listener with the default GameType of Survival (0), or Adventure (2), disappeared from the server list. It is 1 again.

ParsePongData also rejected any pong whose mode was not exactly Survival, Creative or Adventure, discarding the name and player counts of servers that advertise Spectator or no mode at all (PocketMine sends nine fields). The mode is now optional and unknown modes parse as Survival, matching the client; Spectator is supported in both directions via the packet.GameType* constants. The minimum field count was seven while the sub-name at index seven was read unconditionally, which could panic.

expect() ran the deferred packet handlers synchronously on whichever
goroutine called it. That let login handlers run before the state change
they belong to was finished, and off the receiving goroutine entirely:

- handleLogin calls expect(ClientToServerHandshake) before encryption is
  enabled, so an early handshake was answered with LoginSuccess and
  ResourcePacksInfo before ServerToClientHandshake was written.
- handleRequestNetworkSettings calls expect(Login) before NetworkSettings
  is written and compression enabled, with the same ordering problem.
- StartGame, Dial and the resource pack download goroutine call expect(),
  so handlers mutated the connection and the decoder concurrently with
  the receive loop.

expect() now only flags that the expected set changed; the receive loop
re-checks the deferred packets after the packet that caused the change
has been fully handled, and keeps doing so while handlers change the set
again. startGame expects the client's replies before writing StartGame
so a fast reply cannot sit in the deferred queue with no packet after it
to trigger the re-check. The drained slice is released instead of being
re-sliced to its end, which kept every drained packet reachable.
The field after the game mode in the pong is a separate flag, not the
game mode number: clients drop a pong with ten or more fields unless it
is exactly 1, so a listener advertising Survival (0) or Adventure (2)
disappeared from the server list. It is 1 again.

ParsePongData also rejected any pong without exactly Survival, Creative
or Adventure, discarding the name and player counts of servers that
advertise Spectator or no mode at all. The mode is now optional and
unknown modes parse as Survival, matching how the client treats them,
and Spectator is supported in both directions. The minimum field count
was seven while the sub-name at index seven was read unconditionally.
…them

Re-checking only from the receive loop left a packet stranded when the
expected set changed from another goroutine with no packet following it:
a RequestChunkRadius deferred during login was never handled by a later
StartGame, so the client waited for ChunkRadiusUpdated forever.

Handlers are now serialised by a mutex. The receive loop holds it while
handling a packet and re-checks afterwards; expect() re-checks itself
when it can take the lock, which is exactly when it is not called from
inside a handler. Ordering is unchanged: a handler's state change is
complete before any deferred packet is processed.

The re-check also takes only the matching packet out of the deferred
queue, one at a time, so packets meant for the user stay visible to a
concurrent ReadPacket during spawning and keep their order.
…hecking deferred packets

Reverts the deferred packet re-check from Sandertv#406 and the follow-ups that
tried to make it safe. Running deferred handlers inside expect() meant
handlers ran before the state change that expected them had finished
(a deferred ClientToServerHandshake was answered before
ServerToClientHandshake was written) and on goroutines other than the
receive loop (StartGame, Dial and the pack download goroutine), racing
the handlers and the decoder and hiding login transitions from
listenConn. Every attempt to schedule the re-check differently moved the
problem rather than removing it.

The case Sandertv#406 fixed needs none of that. The vanilla client handles
ResourcePacksInfo whenever it arrives and reacts to
PlayStatus(LoginSuccess) only by sending ClientCacheStatus, so both are
expected as soon as Login is sent and LoginSuccess no longer narrows the
set. A server that sends ResourcePacksInfo first is handled on arrival;
a LoginSuccess that follows becomes a stray packet for the user.

startGame expects the client's replies before writing StartGame so a
fast reply cannot be deferred in the gap. The vanilla client sends
RequestChunkRadius only from its StartGame handler, so nothing earlier
than that needs handling.
@HashimTheArab HashimTheArab changed the title minecraft: Re-check deferred packets on the receiving goroutine and fix pong game type fields minecraft: Revert deferred packet re-check, expect ResourcePacksInfo with LoginSuccess; fix pong game type fields Sep 26, 2026
… phase

A LoginSuccess that arrives after ResourcePacksInfo was deferred to the
user, so ClientCacheStatus was never sent for servers that send the
packs info first. PlayStatus stays expected while packs are negotiated;
handling LoginSuccess there only sends ClientCacheStatus.
HashimTheArab added a commit to HashimTheArab/gophertunnel that referenced this pull request Sep 26, 2026
…) into lunar

Brings in the 13 new upstream commits together with the fork's two
open upstream fix PRs in one commit, so the diff shows the net result:
Sandertv#527 reverts the deferred-packet re-check that master added, so that
churn cancels out here.

New upstream behaviour folded into the fork's reworked login stack:
- ListenConfig.LoginTimeout and the authenticated flag + closeTransport
  it relies on.
- ListenerGroup shared player counts across listeners.
- GameType on ServerStatus and the pong game-type field.
- ClientBoundDebugRenderer encoding/build fixes.

Kept the fork's versions where it deliberately diverges: the reworked
decoder (buffer pooling, per-batch algorithm, 1600-packet cap), the
login state machine and StartGame flow, the realms client (version
negotiation; already carries Stories settings), and dial (no pong-port
following, so DisablePortFollowing/EnableLegacyAuth are not added).
nbt allocation bounding is already present from an earlier fork change,
so Sandertv#528 is a no-op here apart from keeping checkRemaining, which the
fork's raw-message decoder still uses.
@TwistedAsylumMC
TwistedAsylumMC merged commit 05bb3a5 into Sandertv:master Sep 26, 2026
1 check passed
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.

2 participants