Skip to content

Fix nil pointer panics and bugs in lobby/room/battle code #359

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.

Sign up for GitHub

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

Merged
inada-s merged 1 commit into from
Mar 2, 2026
Merged

Fix nil pointer panics and bugs in lobby/room/battle code #359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions gdxsv/lbs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,9 @@ func (lbs *Lbs) BroadcastLobbyUserCount(lobby *LbsLobby) {
}

func (lbs *Lbs) BroadcastLobbyMatchEntryUserCount(lobby *LbsLobby) {
if lobby == nil {
return
}
renpo, zeon := lobby.GetLobbyMatchEntryUserCount()
msg1 := NewServerNotice(lbsLobbyMatchingJoin).Writer().Write16(TeamRenpo).Write16(renpo).Msg()
msg2 := NewServerNotice(lbsLobbyMatchingJoin).Writer().Write16(TeamZeon).Write16(zeon).Msg()
Expand Down
6 changes: 3 additions & 3 deletions gdxsv/lbs_battle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,23 +126,23 @@ func (b *LbsBattle) GetPosition(userID string) byte {

func (b *LbsBattle) GetUserByPos(pos byte) *DBUser {
pos--
if len(b.Users) < int(pos) {
if len(b.Users) <= int(pos) {
return nil
}
return b.Users[pos]
}

func (b *LbsBattle) GetGameParamByPos(pos byte) []byte {
pos--
if len(b.GameParams) < int(pos) {
if len(b.GameParams) <= int(pos) {
return nil
}
return b.GameParams[pos]
}

func (b *LbsBattle) GetUserRankByPos(pos byte) int {
pos--
if len(b.UserRanks) < int(pos) {
if len(b.UserRanks) <= int(pos) {
return 0
}
return b.UserRanks[pos]
Expand Down
37 changes: 7 additions & 30 deletions gdxsv/lbs_battle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,18 +142,9 @@ func TestLbsBattle_GetUserByPos(t *testing.T) {
u = b.GetUserByPos(0)
assertEq(t, (*DBUser)(nil), u)

// pos=3 with 2 users: after decrement pos=2, len(Users)=2
// Boundary bug: len(b.Users) < int(pos) uses < instead of <=
// When pos=2 and len=2, 2 < 2 is false, so it tries b.Users[2] which panics.
// This documents the off-by-one bug in GetUserByPos.
func() {
defer func() {
if r := recover(); r == nil {
t.Error("expected panic for pos=3 with 2 users due to boundary bug, but got none")
}
}()
b.GetUserByPos(3)
}()
// pos=3 with 2 users: after decrement pos=2, len(Users)=2, out of bounds → nil
u = b.GetUserByPos(3)
assertEq(t, (*DBUser)(nil), u)
}

func TestLbsBattle_GetGameParamByPos(t *testing.T) {
Expand All @@ -164,15 +155,8 @@ func TestLbsBattle_GetGameParamByPos(t *testing.T) {
assertEq(t, []byte{0xAA}, b.GetGameParamByPos(1))
assertEq(t, []byte{0xBB}, b.GetGameParamByPos(2))

// Same boundary bug as GetUserByPos
func() {
defer func() {
if r := recover(); r == nil {
t.Error("expected panic for pos=3 with 2 params due to boundary bug, but got none")
}
}()
b.GetGameParamByPos(3)
}()
// pos=3 with 2 params: out of bounds → nil
assertEq(t, ([]byte)(nil), b.GetGameParamByPos(3))
}

func TestLbsBattle_GetUserRankByPos(t *testing.T) {
Expand All @@ -183,15 +167,8 @@ func TestLbsBattle_GetUserRankByPos(t *testing.T) {
assertEq(t, 5, b.GetUserRankByPos(1))
assertEq(t, 15, b.GetUserRankByPos(2))

// Same boundary bug as GetUserByPos
func() {
defer func() {
if r := recover(); r == nil {
t.Error("expected panic for pos=3 with 2 ranks due to boundary bug, but got none")
}
}()
b.GetUserRankByPos(3)
}()
// pos=3 with 2 ranks: out of bounds → 0
assertEq(t, 0, b.GetUserRankByPos(3))
}

func Test_genBattleCode(t *testing.T) {
Expand Down
11 changes: 6 additions & 5 deletions gdxsv/lbs_lobby.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func (l *LbsLobby) sendLobbyChat(userID, name, text string) {
msg := chatMsg(userID, name, text)
for user := range l.Users {
peer := l.app.FindPeer(user)
if peer.Room != nil {
if peer == nil || peer.Room != nil {
continue
}
peer.SendMessage(msg)
Expand Down Expand Up @@ -334,7 +334,7 @@ func (l *LbsLobby) NotifyLobbyEvent(kind string, text string) {

for userID := range l.Users {
peer := l.app.FindPeer(userID)
if peer.Room != nil {
if peer == nil || peer.Room != nil {
continue
}
peer.SendMessage(msg)
Expand Down Expand Up @@ -397,6 +397,7 @@ func (l *LbsLobby) Exit(userID string) {
if id == userID {
l.EntryUsers = append(l.EntryUsers[:i], l.EntryUsers[i+1:]...)
l.CancelForceStart()
break
}
}
}
Expand All @@ -421,6 +422,7 @@ func (l *LbsLobby) EntryCancel(p *LbsPeer) {
for i, id := range l.EntryUsers {
if id == p.UserID {
l.EntryUsers = append(l.EntryUsers[:i], l.EntryUsers[i+1:]...)
break
}
}
switch p.Team {
Expand All @@ -438,6 +440,7 @@ func (l *LbsLobby) EntryPicked(p *LbsPeer) {
for i, id := range l.EntryUsers {
if id == p.UserID {
l.EntryUsers = append(l.EntryUsers[:i], l.EntryUsers[i+1:]...)
break
}
Comment on lines 440 to 444

Copilot AI Mar 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
}
Expand Down Expand Up @@ -772,8 +775,6 @@ func (l *LbsLobby) makeP2PMatchingMsg(b *LbsBattle, participants []*LbsPeer) ([]
addrs[p.udpAddr.String()] = true
}

p.udpAddr.IP.IsLoopback()

for addr := range addrs {
ip, port, err := net.SplitHostPort(addr)
if err != nil {
Expand Down Expand Up @@ -947,11 +948,11 @@ func (l *LbsLobby) checkRoomBattleStart() {
allOk := true
for _, u := range room.Users {
p := l.app.FindPeer(u.UserID)
peers = append(peers, p)
if p == nil {
allOk = false
break
}
peers = append(peers, p)
}
if allOk {
renpoRoom = room
Expand Down
129 changes: 129 additions & 0 deletions gdxsv/lbs_lobby_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package main
import (
"fmt"
"testing"

"go.uber.org/zap"
)

func Test_teamShuffle(t *testing.T) {
Expand Down Expand Up @@ -362,6 +364,133 @@ func TestLbsLobby_findBestGCPRegion(t *testing.T) {
})
}

func TestLbsLobby_sendLobbyChat_NilPeer(t *testing.T) {
lbs := NewLbs()
defer lbs.Quit()
go lbs.eventLoop()

lobby := &LbsLobby{
app: lbs,
Users: make(map[string]*DBUser),
RenpoRooms: make(map[uint16]*LbsRoom),
ZeonRooms: make(map[uint16]*LbsRoom),
EntryUsers: make([]string, 0),
}

// Add a user to lobby.Users but NOT to lbs.userPeers,
// so FindPeer returns nil.
lobby.Users["GHOST_USER"] = &DBUser{UserID: "GHOST_USER"}

// This should not panic.
func() {
defer func() {
if r := recover(); r != nil {
t.Errorf("sendLobbyChat panicked with nil peer: %v", r)
}
}()
lobby.sendLobbyChat("SENDER", "SenderName", "hello")
}()
}

func TestLbsLobby_NotifyLobbyEvent_NilPeer(t *testing.T) {
lbs := NewLbs()
defer lbs.Quit()
go lbs.eventLoop()

lobby := &LbsLobby{
app: lbs,
Users: make(map[string]*DBUser),
RenpoRooms: make(map[uint16]*LbsRoom),
ZeonRooms: make(map[uint16]*LbsRoom),
EntryUsers: make([]string, 0),
}

// Add a user to lobby.Users but NOT to lbs.userPeers.
lobby.Users["GHOST_USER"] = &DBUser{UserID: "GHOST_USER"}

// This should not panic.
func() {
defer func() {
if r := recover(); r != nil {
t.Errorf("NotifyLobbyEvent panicked with nil peer: %v", r)
}
}()
lobby.NotifyLobbyEvent("TEST", "test message")
}()
}

func TestLbsLobby_Exit_RemovesEntryUser(t *testing.T) {
lobby := &LbsLobby{
Users: make(map[string]*DBUser),
EntryUsers: []string{"A", "B", "C"},
}
lobby.Users["B"] = &DBUser{UserID: "B"}

lobby.Exit("B")

assertEq(t, []string{"A", "C"}, lobby.EntryUsers)
if _, ok := lobby.Users["B"]; ok {
t.Error("expected user B to be removed from Users map")
}
}

func TestLbsLobby_Exit_NonEntryUser(t *testing.T) {
lobby := &LbsLobby{
Users: make(map[string]*DBUser),
EntryUsers: []string{"A", "C"},
}
lobby.Users["B"] = &DBUser{UserID: "B"}

lobby.Exit("B")

assertEq(t, []string{"A", "C"}, lobby.EntryUsers)
}

func TestLbsLobby_EntryCancel(t *testing.T) {
lbs := NewLbs()
defer lbs.Quit()
go lbs.eventLoop()

lobby := &LbsLobby{
app: lbs,
Users: make(map[string]*DBUser),
RenpoRooms: make(map[uint16]*LbsRoom),
ZeonRooms: make(map[uint16]*LbsRoom),
EntryUsers: []string{"A", "B", "C"},
}

peer := &LbsPeer{
DBUser: DBUser{UserID: "B"},
Team: TeamRenpo,
app: lbs,
PlatformInfo: map[string]string{},
logger: zap.NewNop(),
chWrite: make(chan bool, 1),
}
lbs.Locked(func(l *Lbs) {
l.userPeers["B"] = peer
})
defer lbs.Locked(func(l *Lbs) {
delete(l.userPeers, "B")
})
lobby.Users["B"] = &peer.DBUser

lobby.EntryCancel(peer)

assertEq(t, []string{"A", "C"}, lobby.EntryUsers)
}

func TestLbsLobby_EntryPicked(t *testing.T) {
lobby := &LbsLobby{
EntryUsers: []string{"A", "B", "C"},
}

peer := &LbsPeer{DBUser: DBUser{UserID: "B"}}
lobby.EntryPicked(peer)

assertEq(t, []string{"A", "C"}, lobby.EntryUsers)
}

func TestLbsLobby_canStartBattle(t *testing.T) {
lbs := NewLbs()
defer lbs.Quit()
Expand Down
2 changes: 1 addition & 1 deletion gdxsv/lbs_room.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func (r *LbsRoom) NotifyRoomEvent(kind string, text string) {
Write8(0).Msg() // msg color
for _, u := range r.Users {
peer := r.app.FindPeer(u.UserID)
if peer.Room == nil {
if peer == nil || peer.Room == nil {
continue
}
if peer.Room.ID != r.ID {
Expand Down
Loading