Fix nil pointer panics and bugs in lobby/room/battle code#359
Conversation
- Add nil checks for FindPeer in sendLobbyChat, NotifyLobbyEvent, NotifyRoomEvent - Add nil lobby guard in BroadcastLobbyMatchEntryUserCount - Add break after slice removal in Exit, EntryCancel, EntryPicked - Remove dead code (IsLoopback no-op) in makeP2PMatchingMsg - Fix nil-before-append order in checkRoomBattleStart renpo scan - Fix off-by-one boundary check in GetUserByPos, GetGameParamByPos, GetUserRankByPos - Add tests for all fixed issues Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the lobby/room/battle flows against nil pointer panics and a few boundary/iteration bugs, with accompanying regression tests to ensure the server no longer panics in common disconnect/stale-user scenarios.
Changes:
- Add nil guards around
FindPeer()results (lobby chat/events, room events) and aroundBroadcastLobbyMatchEntryUserCount(nil). - Fix off-by-one bounds checks in
LbsBattlegetters and adjust tests to assert safe returns rather than panics. - Fix slice-modification iteration issues in lobby entry removal paths (Exit/EntryCancel/EntryPicked) and adjust room battle peer collection to avoid nil-before-append.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| gdxsv/lbs_lobby.go | Adds nil checks for peers, fixes iteration after slice removal, removes dead code, and fixes peer collection ordering. |
| gdxsv/lbs_room.go | Prevents nil deref in NotifyRoomEvent when FindPeer returns nil. |
| gdxsv/lbs_battle.go | Fixes off-by-one bounds checks to avoid out-of-range panics. |
| gdxsv/lbs.go | Adds nil guard to BroadcastLobbyMatchEntryUserCount. |
| gdxsv/lbs_lobby_test.go | Adds regression tests for nil-peer handling and entry removal behaviors. |
| gdxsv/lbs_room_test.go | Adds new room behavior/state transition and nil-peer tests. |
| gdxsv/lbs_battle_test.go | Updates tests to validate safe out-of-bounds behavior instead of expecting panics. |
| gdxsv/lbs_test.go | Adds regression test for nil lobby in BroadcastLobbyMatchEntryUserCount. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| delete(l.Users, userID) | ||
| for i, id := range l.EntryUsers { | ||
| if id == userID { | ||
| l.EntryUsers = append(l.EntryUsers[:i], l.EntryUsers[i+1:]...) | ||
| l.CancelForceStart() | ||
| break | ||
| } |
There was a problem hiding this comment.
The new break means only the first matching occurrence is removed from EntryUsers. Since Entry() appends p.UserID without checking for duplicates, EntryUsers can contain the same user multiple times (e.g., repeated lbsLobbyMatchingEntry enable=1), leaving stale extra entries after Exit and causing incorrect counts/force-start logic. Consider enforcing uniqueness on insert, or removing all occurrences safely when deleting.
| l.CancelForceStart() | ||
| for i, id := range l.EntryUsers { | ||
| if id == p.UserID { | ||
| l.EntryUsers = append(l.EntryUsers[:i], l.EntryUsers[i+1:]...) | ||
| break | ||
| } |
There was a problem hiding this comment.
EntryCancel now breaks after removing the first matching element. If EntryUsers contains duplicates (possible because Entry() appends without deduping), this will leave additional occurrences behind and can inflate GetLobbyMatchEntryUserCount() / allow a user to be picked multiple times. Consider deduping on Entry() or removing all occurrences here.
| for i, id := range l.EntryUsers { | ||
| if id == p.UserID { | ||
| l.EntryUsers = append(l.EntryUsers[:i], l.EntryUsers[i+1:]...) | ||
| break | ||
| } |
There was a problem hiding this comment.
EntryPicked now removes only the first occurrence due to the added break. If duplicates can be present in EntryUsers (since Entry() unconditionally appends), one user may remain "entered" after being picked, affecting later counts and selection. Consider ensuring EntryUsers is unique or removing all matches.
Summary
sendLobbyChat,NotifyLobbyEvent,NotifyRoomEventwhenFindPeerreturns nil for a disconnected user still in the Users mapBroadcastLobbyMatchEntryUserCount(unlikeBroadcastLobbyUserCountwhich already had one)breakafter slice removal inExit,EntryCancel,EntryPickedto prevent incorrect iteration after modifying the slicep.udpAddr.IP.IsLoopback()inmakeP2PMatchingMsgthat was a no-op and potential nil paniccheckRoomBattleStartrenpo room scan to match the zeon side pattern<→<=) inGetUserByPos,GetGameParamByPos,GetUserRankByPosTest plan
TestLbsLobby_sendLobbyChat_NilPeer- verifies no panic with nil peerTestLbsLobby_NotifyLobbyEvent_NilPeer- verifies no panic with nil peerTestLbsRoom_NotifyRoomEvent_NilPeer- verifies no panic with nil peerTestBroadcastLobbyMatchEntryUserCount_NilLobby- verifies no panic with nil lobbyTestLbsLobby_Exit_RemovesEntryUser/TestLbsLobby_Exit_NonEntryUser- verifies correct slice removalTestLbsLobby_EntryCancel/TestLbsLobby_EntryPicked- verifies correct entry removalTestLbsRoom_Enter/TestLbsRoom_Exit/TestLbsRoom_Remove- verifies room state transitionsTestLbsBattle_GetUserByPos/GetGameParamByPos/GetUserRankByPos- boundary returns nil/0 instead of panicmake test)make lint)