From aee53534a209324a061ae91aea4f5f2936d0bde0 Mon Sep 17 00:00:00 2001 From: Houmgaor Date: Mon, 2 Mar 2026 19:43:11 +0100 Subject: [PATCH] fix(guild): add nil guards for alliance guild lookups (#171) scanAllianceWithGuilds dereferences guild pointers returned by GetByID without checking for nil. Since GetByID returns (nil, nil) when a guild is missing, alliances referencing deleted guilds cause nil-pointer panics. The panic is caught by session recovery but no ACK is sent, softlocking the client. Add nil checks in scanAllianceWithGuilds, handleMsgMhfOperateJoint, handleMsgMhfInfoJoint, and handleMsgMhfInfoGuild so that missing guilds or alliances produce proper error responses instead of panics. --- .../channelserver/handlers_guild_alliance.go | 88 +++++++++++-------- .../handlers_guild_alliance_test.go | 67 ++++++++++++++ server/channelserver/handlers_guild_info.go | 4 +- server/channelserver/repo_guild_alliance.go | 9 ++ 4 files changed, 127 insertions(+), 41 deletions(-) diff --git a/server/channelserver/handlers_guild_alliance.go b/server/channelserver/handlers_guild_alliance.go index 53c0db800..f90607ae5 100644 --- a/server/channelserver/handlers_guild_alliance.go +++ b/server/channelserver/handlers_guild_alliance.go @@ -41,10 +41,20 @@ func handleMsgMhfOperateJoint(s *Session, p mhfpacket.MHFPacket) { if err != nil { s.logger.Error("Failed to get guild info", zap.Error(err)) } + if guild == nil { + s.logger.Error("Guild not found for alliance operation", zap.Uint32("GuildID", pkt.GuildID)) + doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) + return + } alliance, err := s.server.guildRepo.GetAllianceByID(pkt.AllianceID) if err != nil { s.logger.Error("Failed to get alliance info", zap.Error(err)) } + if alliance == nil { + s.logger.Error("Alliance not found for operation", zap.Uint32("AllianceID", pkt.AllianceID)) + doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) + return + } switch pkt.Action { case mhfpacket.OPERATE_JOINT_DISBAND: @@ -119,45 +129,45 @@ func handleMsgMhfInfoJoint(s *Session, p mhfpacket.MHFPacket) { pkt := p.(*mhfpacket.MsgMhfInfoJoint) bf := byteframe.NewByteFrame() alliance, err := s.server.guildRepo.GetAllianceByID(pkt.AllianceID) - if err != nil { + if err != nil || alliance == nil { doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) - } else { - bf.WriteUint32(alliance.ID) - bf.WriteUint32(uint32(alliance.CreatedAt.Unix())) - bf.WriteUint16(alliance.TotalMembers) - bf.WriteUint16(0x0000) // Unk - ps.Uint16(bf, alliance.Name, true) - if alliance.SubGuild1ID > 0 { - if alliance.SubGuild2ID > 0 { - bf.WriteUint8(3) - } else { - bf.WriteUint8(2) - } - } else { - bf.WriteUint8(1) - } - bf.WriteUint32(alliance.ParentGuildID) - bf.WriteUint32(alliance.ParentGuild.LeaderCharID) - bf.WriteUint16(alliance.ParentGuild.Rank(s.server.erupeConfig.RealClientMode)) - bf.WriteUint16(alliance.ParentGuild.MemberCount) - ps.Uint16(bf, alliance.ParentGuild.Name, true) - ps.Uint16(bf, alliance.ParentGuild.LeaderName, true) - if alliance.SubGuild1ID > 0 { - bf.WriteUint32(alliance.SubGuild1ID) - bf.WriteUint32(alliance.SubGuild1.LeaderCharID) - bf.WriteUint16(alliance.SubGuild1.Rank(s.server.erupeConfig.RealClientMode)) - bf.WriteUint16(alliance.SubGuild1.MemberCount) - ps.Uint16(bf, alliance.SubGuild1.Name, true) - ps.Uint16(bf, alliance.SubGuild1.LeaderName, true) - } - if alliance.SubGuild2ID > 0 { - bf.WriteUint32(alliance.SubGuild2ID) - bf.WriteUint32(alliance.SubGuild2.LeaderCharID) - bf.WriteUint16(alliance.SubGuild2.Rank(s.server.erupeConfig.RealClientMode)) - bf.WriteUint16(alliance.SubGuild2.MemberCount) - ps.Uint16(bf, alliance.SubGuild2.Name, true) - ps.Uint16(bf, alliance.SubGuild2.LeaderName, true) - } - doAckBufSucceed(s, pkt.AckHandle, bf.Data()) + return } + bf.WriteUint32(alliance.ID) + bf.WriteUint32(uint32(alliance.CreatedAt.Unix())) + bf.WriteUint16(alliance.TotalMembers) + bf.WriteUint16(0x0000) // Unk + ps.Uint16(bf, alliance.Name, true) + if alliance.SubGuild1ID > 0 { + if alliance.SubGuild2ID > 0 { + bf.WriteUint8(3) + } else { + bf.WriteUint8(2) + } + } else { + bf.WriteUint8(1) + } + bf.WriteUint32(alliance.ParentGuildID) + bf.WriteUint32(alliance.ParentGuild.LeaderCharID) + bf.WriteUint16(alliance.ParentGuild.Rank(s.server.erupeConfig.RealClientMode)) + bf.WriteUint16(alliance.ParentGuild.MemberCount) + ps.Uint16(bf, alliance.ParentGuild.Name, true) + ps.Uint16(bf, alliance.ParentGuild.LeaderName, true) + if alliance.SubGuild1ID > 0 { + bf.WriteUint32(alliance.SubGuild1ID) + bf.WriteUint32(alliance.SubGuild1.LeaderCharID) + bf.WriteUint16(alliance.SubGuild1.Rank(s.server.erupeConfig.RealClientMode)) + bf.WriteUint16(alliance.SubGuild1.MemberCount) + ps.Uint16(bf, alliance.SubGuild1.Name, true) + ps.Uint16(bf, alliance.SubGuild1.LeaderName, true) + } + if alliance.SubGuild2ID > 0 { + bf.WriteUint32(alliance.SubGuild2ID) + bf.WriteUint32(alliance.SubGuild2.LeaderCharID) + bf.WriteUint16(alliance.SubGuild2.Rank(s.server.erupeConfig.RealClientMode)) + bf.WriteUint16(alliance.SubGuild2.MemberCount) + ps.Uint16(bf, alliance.SubGuild2.Name, true) + ps.Uint16(bf, alliance.SubGuild2.LeaderName, true) + } + doAckBufSucceed(s, pkt.AckHandle, bf.Data()) } diff --git a/server/channelserver/handlers_guild_alliance_test.go b/server/channelserver/handlers_guild_alliance_test.go index 0e483d6b8..fd40169b5 100644 --- a/server/channelserver/handlers_guild_alliance_test.go +++ b/server/channelserver/handlers_guild_alliance_test.go @@ -438,3 +438,70 @@ func TestInfoJoint_NotFound(t *testing.T) { t.Error("No response packet queued") } } + +func TestInfoJoint_NilAlliance(t *testing.T) { + server := createMockServer() + // alliance is nil, no error — simulates deleted alliance + guildMock := &mockGuildRepo{} + server.guildRepo = guildMock + session := createMockSession(1, server) + + pkt := &mhfpacket.MsgMhfInfoJoint{AckHandle: 100, AllianceID: 999} + + handleMsgMhfInfoJoint(session, pkt) + + select { + case <-session.sendPackets: + default: + t.Error("No response packet queued — would softlock the client") + } +} + +func TestOperateJoint_NilGuild(t *testing.T) { + server := createMockServer() + // guild is nil — simulates deleted guild + guildMock := &mockGuildRepo{ + alliance: &GuildAlliance{ID: 5, ParentGuildID: 10}, + } + server.guildRepo = guildMock + session := createMockSession(1, server) + + pkt := &mhfpacket.MsgMhfOperateJoint{ + AckHandle: 100, + AllianceID: 5, + GuildID: 10, + Action: mhfpacket.OPERATE_JOINT_DISBAND, + } + + handleMsgMhfOperateJoint(session, pkt) + + select { + case <-session.sendPackets: + default: + t.Error("No response packet queued — would softlock the client") + } +} + +func TestOperateJoint_NilAlliance(t *testing.T) { + server := createMockServer() + guildMock := &mockGuildRepo{} + guildMock.guild = &Guild{ID: 10} + guildMock.guild.LeaderCharID = 1 + server.guildRepo = guildMock + session := createMockSession(1, server) + + pkt := &mhfpacket.MsgMhfOperateJoint{ + AckHandle: 100, + AllianceID: 999, + GuildID: 10, + Action: mhfpacket.OPERATE_JOINT_DISBAND, + } + + handleMsgMhfOperateJoint(session, pkt) + + select { + case <-session.sendPackets: + default: + t.Error("No response packet queued — would softlock the client") + } +} diff --git a/server/channelserver/handlers_guild_info.go b/server/channelserver/handlers_guild_info.go index 2153a2d08..dc7dc8182 100644 --- a/server/channelserver/handlers_guild_info.go +++ b/server/channelserver/handlers_guild_info.go @@ -135,8 +135,8 @@ func handleMsgMhfInfoGuild(s *Session, p mhfpacket.MHFPacket) { if guild.AllianceID > 0 { alliance, err := s.server.guildRepo.GetAllianceByID(guild.AllianceID) - if err != nil { - bf.WriteUint32(0) // Error, no alliance + if err != nil || alliance == nil { + bf.WriteUint32(0) // Error or missing alliance } else { bf.WriteUint32(alliance.ID) bf.WriteUint32(uint32(alliance.CreatedAt.Unix())) diff --git a/server/channelserver/repo_guild_alliance.go b/server/channelserver/repo_guild_alliance.go index 744394a07..0b47e7549 100644 --- a/server/channelserver/repo_guild_alliance.go +++ b/server/channelserver/repo_guild_alliance.go @@ -97,6 +97,9 @@ func (r *GuildRepository) scanAllianceWithGuilds(rows *sqlx.Rows) (*GuildAllianc if err != nil { return nil, err } + if parentGuild == nil { + return nil, fmt.Errorf("alliance %d references non-existent parent guild %d", alliance.ID, alliance.ParentGuildID) + } alliance.ParentGuild = *parentGuild alliance.TotalMembers += parentGuild.MemberCount @@ -105,6 +108,9 @@ func (r *GuildRepository) scanAllianceWithGuilds(rows *sqlx.Rows) (*GuildAllianc if err != nil { return nil, err } + if subGuild1 == nil { + return nil, fmt.Errorf("alliance %d references non-existent sub guild 1 %d", alliance.ID, alliance.SubGuild1ID) + } alliance.SubGuild1 = *subGuild1 alliance.TotalMembers += subGuild1.MemberCount } @@ -114,6 +120,9 @@ func (r *GuildRepository) scanAllianceWithGuilds(rows *sqlx.Rows) (*GuildAllianc if err != nil { return nil, err } + if subGuild2 == nil { + return nil, fmt.Errorf("alliance %d references non-existent sub guild 2 %d", alliance.ID, alliance.SubGuild2ID) + } alliance.SubGuild2 = *subGuild2 alliance.TotalMembers += subGuild2.MemberCount }