From bfc5319cb67d1212ae0f49898a2f5536d0ea2033 Mon Sep 17 00:00:00 2001 From: Houmgaor Date: Fri, 6 Mar 2026 00:15:53 +0100 Subject: [PATCH] fix(guild): fix nil panics causing clan menu softlock (#171) The crash was in handleMsgMhfGetGuildManageRight where a variable shadowing bug (guild, err := instead of guild, err =) left the outer guild pointer nil after the inner GetByID lookup succeeded, panicking at guild.MemberCount. This is confirmed by the user's stack trace pointing to handlers_guild.go:234. Also fix 6 other nil-dereference risks across guild handlers: - handleMsgMhfArrangeGuildMember: guild nil after GetByID - handleMsgMhfEnumerateGuildMember: alliance nil when AllianceID > 0 - handleMsgMhfUpdateGuildIcon: guild and characterInfo nil - handleMsgMhfOperateGuild: guild nil, characterGuildInfo nil - handleAvoidLeadershipUpdate: characterGuildData nil Improve panic recovery to log opcode and full stack trace so future panics can be diagnosed from console screenshots. --- server/channelserver/handlers_guild.go | 13 +++++++------ server/channelserver/handlers_guild_ops.go | 11 ++++++----- server/channelserver/sys_session.go | 8 +++++++- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/server/channelserver/handlers_guild.go b/server/channelserver/handlers_guild.go index 5ec0ab0e5..42241bce6 100644 --- a/server/channelserver/handlers_guild.go +++ b/server/channelserver/handlers_guild.go @@ -41,7 +41,7 @@ func handleMsgMhfArrangeGuildMember(s *Session, p mhfpacket.MHFPacket) { guild, err := s.server.guildRepo.GetByID(pkt.GuildID) - if err != nil { + if err != nil || guild == nil { s.logger.Error( "failed to respond to ArrangeGuildMember message", zap.Uint32("charID", s.charID), @@ -131,7 +131,7 @@ func handleMsgMhfEnumerateGuildMember(s *Session, p mhfpacket.MHFPacket) { alliance, err := s.server.guildRepo.GetAllianceByID(guild.AllianceID) if err != nil { - s.logger.Error("Failed to get alliance data") + s.logger.Error("Failed to get alliance data", zap.Error(err)) doAckBufFail(s, pkt.AckHandle, make([]byte, 4)) return } @@ -170,7 +170,7 @@ func handleMsgMhfEnumerateGuildMember(s *Session, p mhfpacket.MHFPacket) { bf.WriteUint32(member.LastLogin) } - if guild.AllianceID > 0 { + if guild.AllianceID > 0 && alliance != nil { bf.WriteUint16(alliance.TotalMembers - uint16(len(guildMembers))) if guild.ID != alliance.ParentGuildID { mems, err := s.server.guildRepo.GetMembers(alliance.ParentGuildID, false) @@ -222,7 +222,8 @@ func handleMsgMhfGetGuildManageRight(s *Session, p mhfpacket.MHFPacket) { guild, _ := s.server.guildRepo.GetByCharID(s.charID) if guild == nil || s.prevGuildID != 0 { - guild, err := s.server.guildRepo.GetByID(s.prevGuildID) + var err error + guild, err = s.server.guildRepo.GetByID(s.prevGuildID) s.prevGuildID = 0 if guild == nil || err != nil { doAckBufSucceed(s, pkt.AckHandle, make([]byte, 4)) @@ -298,7 +299,7 @@ func handleMsgMhfUpdateGuildIcon(s *Session, p mhfpacket.MHFPacket) { guild, err := s.server.guildRepo.GetByID(pkt.GuildID) - if err != nil { + if err != nil || guild == nil { s.logger.Error("Failed to get guild info for icon update", zap.Error(err)) doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) return @@ -306,7 +307,7 @@ func handleMsgMhfUpdateGuildIcon(s *Session, p mhfpacket.MHFPacket) { characterInfo, err := s.server.guildRepo.GetCharacterMembership(s.charID) - if err != nil { + if err != nil || characterInfo == nil { s.logger.Error("Failed to get character guild data for icon update", zap.Error(err)) doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) return diff --git a/server/channelserver/handlers_guild_ops.go b/server/channelserver/handlers_guild_ops.go index 2be04dad5..4fa1ce6ed 100644 --- a/server/channelserver/handlers_guild_ops.go +++ b/server/channelserver/handlers_guild_ops.go @@ -13,7 +13,7 @@ func handleMsgMhfOperateGuild(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfOperateGuild) guild, err := s.server.guildRepo.GetByID(pkt.GuildID) - if err != nil { + if err != nil || guild == nil { doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) return } @@ -49,7 +49,8 @@ func handleMsgMhfOperateGuild(s *Session, p mhfpacket.MHFPacket) { bf.WriteUint32(0) } case mhfpacket.OperateGuildLeave: - result, err := s.server.guildService.Leave(s.charID, guild.ID, characterGuildInfo.IsApplicant, guild.Name) + isApplicant := characterGuildInfo != nil && characterGuildInfo.IsApplicant + result, err := s.server.guildService.Leave(s.charID, guild.ID, isApplicant, guild.Name) if err != nil { s.logger.Error("Failed to leave guild", zap.Error(err)) } @@ -73,7 +74,7 @@ func handleMsgMhfOperateGuild(s *Session, p mhfpacket.MHFPacket) { case mhfpacket.OperateGuildSetAvoidLeadershipFalse: handleAvoidLeadershipUpdate(s, pkt, false) case mhfpacket.OperateGuildUpdateComment: - if !characterGuildInfo.IsLeader && !characterGuildInfo.IsSubLeader() { + if characterGuildInfo == nil || (!characterGuildInfo.IsLeader && !characterGuildInfo.IsSubLeader()) { doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) return } @@ -82,7 +83,7 @@ func handleMsgMhfOperateGuild(s *Session, p mhfpacket.MHFPacket) { s.logger.Error("Failed to save guild comment", zap.Error(err)) } case mhfpacket.OperateGuildUpdateMotto: - if !characterGuildInfo.IsLeader && !characterGuildInfo.IsSubLeader() { + if characterGuildInfo == nil || (!characterGuildInfo.IsLeader && !characterGuildInfo.IsSubLeader()) { doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) return } @@ -217,7 +218,7 @@ func handleDonateRP(s *Session, amount uint16, guild *Guild, _type int) []byte { func handleAvoidLeadershipUpdate(s *Session, pkt *mhfpacket.MsgMhfOperateGuild, avoidLeadership bool) { characterGuildData, err := s.server.guildRepo.GetCharacterMembership(s.charID) - if err != nil { + if err != nil || characterGuildData == nil { doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) return } diff --git a/server/channelserver/sys_session.go b/server/channelserver/sys_session.go index 8e7cb5b73..be9d23e3d 100644 --- a/server/channelserver/sys_session.go +++ b/server/channelserver/sys_session.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "net" + "runtime/debug" "sync" "sync/atomic" "time" @@ -240,7 +241,12 @@ func (s *Session) handlePacketGroup(pktGroup []byte) { // This shouldn't be needed, but it's better to recover and let the connection die than to panic the server. defer func() { if r := recover(); r != nil { - s.logger.Error("Recovered from panic", zap.String("name", s.Name), zap.Any("panic", r)) + s.logger.Error("Recovered from panic", + zap.String("name", s.Name), + zap.Stringer("opcode", opcode), + zap.Any("panic", r), + zap.String("stack", string(debug.Stack())), + ) } }()