Skip to content

Commit c45d53c

Browse files
inada-sclaude
inada-s
and
claude
committed
Fix nil pointer panics and bugs in lobby/room/battle code
- 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>
1 parent 8535431 commit c45d53c

8 files changed

Lines changed: 289 additions & 39 deletions

File tree

‎gdxsv/lbs.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,9 @@ func (lbs *Lbs) BroadcastLobbyUserCount(lobby *LbsLobby) {
525525
}
526526

527527
func (lbs *Lbs) BroadcastLobbyMatchEntryUserCount(lobby *LbsLobby) {
528+
if lobby == nil {
529+
return
530+
}
528531
renpo, zeon := lobby.GetLobbyMatchEntryUserCount()
529532
msg1 := NewServerNotice(lbsLobbyMatchingJoin).Writer().Write16(TeamRenpo).Write16(renpo).Msg()
530533
msg2 := NewServerNotice(lbsLobbyMatchingJoin).Writer().Write16(TeamZeon).Write16(zeon).Msg()

‎gdxsv/lbs_battle.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,23 +126,23 @@ func (b *LbsBattle) GetPosition(userID string) byte {
126126

127127
func (b *LbsBattle) GetUserByPos(pos byte) *DBUser {
128128
pos--
129-
if len(b.Users) < int(pos) {
129+
if len(b.Users) <= int(pos) {
130130
return nil
131131
}
132132
return b.Users[pos]
133133
}
134134

135135
func (b *LbsBattle) GetGameParamByPos(pos byte) []byte {
136136
pos--
137-
if len(b.GameParams) < int(pos) {
137+
if len(b.GameParams) <= int(pos) {
138138
return nil
139139
}
140140
return b.GameParams[pos]
141141
}
142142

143143
func (b *LbsBattle) GetUserRankByPos(pos byte) int {
144144
pos--
145-
if len(b.UserRanks) < int(pos) {
145+
if len(b.UserRanks) <= int(pos) {
146146
return 0
147147
}
148148
return b.UserRanks[pos]

‎gdxsv/lbs_battle_test.go‎

Lines changed: 7 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -142,18 +142,9 @@ func TestLbsBattle_GetUserByPos(t *testing.T) {
142142
u = b.GetUserByPos(0)
143143
assertEq(t, (*DBUser)(nil), u)
144144

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

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

167-
// Same boundary bug as GetUserByPos
168-
func() {
169-
defer func() {
170-
if r := recover(); r == nil {
171-
t.Error("expected panic for pos=3 with 2 params due to boundary bug, but got none")
172-
}
173-
}()
174-
b.GetGameParamByPos(3)
175-
}()
158+
// pos=3 with 2 params: out of bounds → nil
159+
assertEq(t, ([]byte)(nil), b.GetGameParamByPos(3))
176160
}
177161

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

186-
// Same boundary bug as GetUserByPos
187-
func() {
188-
defer func() {
189-
if r := recover(); r == nil {
190-
t.Error("expected panic for pos=3 with 2 ranks due to boundary bug, but got none")
191-
}
192-
}()
193-
b.GetUserRankByPos(3)
194-
}()
170+
// pos=3 with 2 ranks: out of bounds → 0
171+
assertEq(t, 0, b.GetUserRankByPos(3))
195172
}
196173

197174
func Test_genBattleCode(t *testing.T) {

‎gdxsv/lbs_lobby.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ func (l *LbsLobby) sendLobbyChat(userID, name, text string) {
135135
msg := chatMsg(userID, name, text)
136136
for user := range l.Users {
137137
peer := l.app.FindPeer(user)
138-
if peer.Room != nil {
138+
if peer == nil || peer.Room != nil {
139139
continue
140140
}
141141
peer.SendMessage(msg)
@@ -334,7 +334,7 @@ func (l *LbsLobby) NotifyLobbyEvent(kind string, text string) {
334334

335335
for userID := range l.Users {
336336
peer := l.app.FindPeer(userID)
337-
if peer.Room != nil {
337+
if peer == nil || peer.Room != nil {
338338
continue
339339
}
340340
peer.SendMessage(msg)
@@ -397,6 +397,7 @@ func (l *LbsLobby) Exit(userID string) {
397397
if id == userID {
398398
l.EntryUsers = append(l.EntryUsers[:i], l.EntryUsers[i+1:]...)
399399
l.CancelForceStart()
400+
break
400401
}
401402
}
402403
}
@@ -421,6 +422,7 @@ func (l *LbsLobby) EntryCancel(p *LbsPeer) {
421422
for i, id := range l.EntryUsers {
422423
if id == p.UserID {
423424
l.EntryUsers = append(l.EntryUsers[:i], l.EntryUsers[i+1:]...)
425+
break
424426
}
425427
}
426428
switch p.Team {
@@ -438,6 +440,7 @@ func (l *LbsLobby) EntryPicked(p *LbsPeer) {
438440
for i, id := range l.EntryUsers {
439441
if id == p.UserID {
440442
l.EntryUsers = append(l.EntryUsers[:i], l.EntryUsers[i+1:]...)
443+
break
441444
}
442445
}
443446
}
@@ -772,8 +775,6 @@ func (l *LbsLobby) makeP2PMatchingMsg(b *LbsBattle, participants []*LbsPeer) ([]
772775
addrs[p.udpAddr.String()] = true
773776
}
774777

775-
p.udpAddr.IP.IsLoopback()
776-
777778
for addr := range addrs {
778779
ip, port, err := net.SplitHostPort(addr)
779780
if err != nil {
@@ -947,11 +948,11 @@ func (l *LbsLobby) checkRoomBattleStart() {
947948
allOk := true
948949
for _, u := range room.Users {
949950
p := l.app.FindPeer(u.UserID)
950-
peers = append(peers, p)
951951
if p == nil {
952952
allOk = false
953953
break
954954
}
955+
peers = append(peers, p)
955956
}
956957
if allOk {
957958
renpoRoom = room

‎gdxsv/lbs_lobby_test.go‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package main
33
import (
44
"fmt"
55
"testing"
6+
7+
"go.uber.org/zap"
68
)
79

810
func Test_teamShuffle(t *testing.T) {
@@ -362,6 +364,133 @@ func TestLbsLobby_findBestGCPRegion(t *testing.T) {
362364
})
363365
}
364366

367+
func TestLbsLobby_sendLobbyChat_NilPeer(t *testing.T) {
368+
lbs := NewLbs()
369+
defer lbs.Quit()
370+
go lbs.eventLoop()
371+
372+
lobby := &LbsLobby{
373+
app: lbs,
374+
Users: make(map[string]*DBUser),
375+
RenpoRooms: make(map[uint16]*LbsRoom),
376+
ZeonRooms: make(map[uint16]*LbsRoom),
377+
EntryUsers: make([]string, 0),
378+
}
379+
380+
// Add a user to lobby.Users but NOT to lbs.userPeers,
381+
// so FindPeer returns nil.
382+
lobby.Users["GHOST_USER"] = &DBUser{UserID: "GHOST_USER"}
383+
384+
// This should not panic.
385+
func() {
386+
defer func() {
387+
if r := recover(); r != nil {
388+
t.Errorf("sendLobbyChat panicked with nil peer: %v", r)
389+
}
390+
}()
391+
lobby.sendLobbyChat("SENDER", "SenderName", "hello")
392+
}()
393+
}
394+
395+
func TestLbsLobby_NotifyLobbyEvent_NilPeer(t *testing.T) {
396+
lbs := NewLbs()
397+
defer lbs.Quit()
398+
go lbs.eventLoop()
399+
400+
lobby := &LbsLobby{
401+
app: lbs,
402+
Users: make(map[string]*DBUser),
403+
RenpoRooms: make(map[uint16]*LbsRoom),
404+
ZeonRooms: make(map[uint16]*LbsRoom),
405+
EntryUsers: make([]string, 0),
406+
}
407+
408+
// Add a user to lobby.Users but NOT to lbs.userPeers.
409+
lobby.Users["GHOST_USER"] = &DBUser{UserID: "GHOST_USER"}
410+
411+
// This should not panic.
412+
func() {
413+
defer func() {
414+
if r := recover(); r != nil {
415+
t.Errorf("NotifyLobbyEvent panicked with nil peer: %v", r)
416+
}
417+
}()
418+
lobby.NotifyLobbyEvent("TEST", "test message")
419+
}()
420+
}
421+
422+
func TestLbsLobby_Exit_RemovesEntryUser(t *testing.T) {
423+
lobby := &LbsLobby{
424+
Users: make(map[string]*DBUser),
425+
EntryUsers: []string{"A", "B", "C"},
426+
}
427+
lobby.Users["B"] = &DBUser{UserID: "B"}
428+
429+
lobby.Exit("B")
430+
431+
assertEq(t, []string{"A", "C"}, lobby.EntryUsers)
432+
if _, ok := lobby.Users["B"]; ok {
433+
t.Error("expected user B to be removed from Users map")
434+
}
435+
}
436+
437+
func TestLbsLobby_Exit_NonEntryUser(t *testing.T) {
438+
lobby := &LbsLobby{
439+
Users: make(map[string]*DBUser),
440+
EntryUsers: []string{"A", "C"},
441+
}
442+
lobby.Users["B"] = &DBUser{UserID: "B"}
443+
444+
lobby.Exit("B")
445+
446+
assertEq(t, []string{"A", "C"}, lobby.EntryUsers)
447+
}
448+
449+
func TestLbsLobby_EntryCancel(t *testing.T) {
450+
lbs := NewLbs()
451+
defer lbs.Quit()
452+
go lbs.eventLoop()
453+
454+
lobby := &LbsLobby{
455+
app: lbs,
456+
Users: make(map[string]*DBUser),
457+
RenpoRooms: make(map[uint16]*LbsRoom),
458+
ZeonRooms: make(map[uint16]*LbsRoom),
459+
EntryUsers: []string{"A", "B", "C"},
460+
}
461+
462+
peer := &LbsPeer{
463+
DBUser: DBUser{UserID: "B"},
464+
Team: TeamRenpo,
465+
app: lbs,
466+
PlatformInfo: map[string]string{},
467+
logger: zap.NewNop(),
468+
chWrite: make(chan bool, 1),
469+
}
470+
lbs.Locked(func(l *Lbs) {
471+
l.userPeers["B"] = peer
472+
})
473+
defer lbs.Locked(func(l *Lbs) {
474+
delete(l.userPeers, "B")
475+
})
476+
lobby.Users["B"] = &peer.DBUser
477+
478+
lobby.EntryCancel(peer)
479+
480+
assertEq(t, []string{"A", "C"}, lobby.EntryUsers)
481+
}
482+
483+
func TestLbsLobby_EntryPicked(t *testing.T) {
484+
lobby := &LbsLobby{
485+
EntryUsers: []string{"A", "B", "C"},
486+
}
487+
488+
peer := &LbsPeer{DBUser: DBUser{UserID: "B"}}
489+
lobby.EntryPicked(peer)
490+
491+
assertEq(t, []string{"A", "C"}, lobby.EntryUsers)
492+
}
493+
365494
func TestLbsLobby_canStartBattle(t *testing.T) {
366495
lbs := NewLbs()
367496
defer lbs.Quit()

‎gdxsv/lbs_room.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ func (r *LbsRoom) NotifyRoomEvent(kind string, text string) {
139139
Write8(0).Msg() // msg color
140140
for _, u := range r.Users {
141141
peer := r.app.FindPeer(u.UserID)
142-
if peer.Room == nil {
142+
if peer == nil || peer.Room == nil {
143143
continue
144144
}
145145
if peer.Room.ID != r.ID {

0 commit comments

Comments
 (0)