[DB Crash Fix] Marriage Manager Use-After-Free

[Distraught 9951]

Hello,

I found a crash in the DB’s marriage manager today. It’s pretty simple: we keep a reference to a value, then erase it from the map, and later try to access it again.
I’ve attached a picture to show what’s happening.

 

To fix this crash, open Marriage.cpp in db and search for CManager::Update function.
Then remove the reference (&) and move the erase after it (the latter is just for code clarity).

Before:

TWedding& w = it->second;

TPacketWeddingEnd p;
p.dwPID1 = w.dwPID1;
p.dwPID2 = w.dwPID2;
CClientManager::instance().ForwardPacket(HEADER_DG_WEDDING_END, &p, sizeof(p));
m_mapRunningWedding.erase(it);

 

After:

TWedding w = it->second;
m_mapRunningWedding.erase(it);

TPacketWeddingEnd p;
p.dwPID1 = w.dwPID1;
p.dwPID2 = w.dwPID2;
CClientManager::instance().ForwardPacket(HEADER_DG_WEDDING_END, &p, sizeof(p));

 

[Metin2 Dev 9196]

Nice catch 

There is also one in GuildManager.cpp in void CGuildManager::Update():

const TGuildWaitStartInfo & ws = m_pqWaitStart.top().second;

 

[Kidro 188]

void CGuildManager::Update()
{
    ProcessReserveWar();
    
    time_t now = CClientManager::instance().GetCurrentTime();

    if (!m_pqOnWar.empty())
    {
        while (!m_pqOnWar.empty() && (m_pqOnWar.top().first <= now || (m_pqOnWar.top().second && m_pqOnWar.top().second->bEnd)))
        {
            TGuildWarPQElement* e = m_pqOnWar.top().second;
            m_pqOnWar.pop();

            if (!e)
                continue;

            auto itOuter = m_WarMap.find(e->GID[0]);
            if (itOuter != m_WarMap.end())
            {
                auto& innerMap = itOuter->second;
                auto itInner = innerMap.find(e->GID[1]);
                if (itInner != innerMap.end())
                {
                    innerMap.erase(itInner);
                    if (innerMap.empty())
                        m_WarMap.erase(itOuter);
                }
            }

            if (!e->bEnd)
                WarEnd(e->GID[0], e->GID[1], false);

            delete e;
        }
    }

    while (!m_pqSkill.empty() && m_pqSkill.top().first <= now)
    {
        const TGuildSkillUsed& s = m_pqSkill.top().second;

        if (s.GID == 0)
        {
            sys_err("Invalid GuildSkillUsed GID: 0, skipping");
            m_pqSkill.pop();
            continue;
        }

        CClientManager::instance().SendGuildSkillUsable(s.GID, s.dwSkillVnum, true);
        m_pqSkill.pop();
    }

    while (!m_pqWaitStart.empty() && m_pqWaitStart.top().first <= now)
    {
        const TGuildWaitStartInfo ws = m_pqWaitStart.top().second;
        m_pqWaitStart.pop();

        if (ws.GID[0] == 0 || ws.GID[1] == 0)
        {
            sys_err("Invalid GuildWaitStartInfo GID: [%u, %u], skipping", ws.GID[0], ws.GID[1]);
            continue;
        }

#ifdef __IMPROVED_GUILD_WAR__
        StartWar(ws.bType, ws.GID[0], ws.GID[1], ws.pkReserve, ws.iMaxPlayer, ws.iMaxScore, ws.flags, ws.custom_map_index);
#else
        StartWar(ws.bType, ws.GID[0], ws.GID[1], ws.pkReserve);
#endif

        if (ws.lInitialScore)
        {
            UpdateScore(ws.GID[0], ws.GID[1], ws.lInitialScore, 0);
            UpdateScore(ws.GID[1], ws.GID[0], ws.lInitialScore, 0);
        }

        TPacketGuildWar p{};
        p.bType = ws.bType;
        p.bWar = GUILD_WAR_ON_WAR;
        p.dwGuildFrom = ws.GID[0];
        p.dwGuildTo = ws.GID[1];
#ifdef __IMPROVED_GUILD_WAR__
        p.iMaxPlayer = ws.iMaxPlayer;
        p.iMaxScore = ws.iMaxScore;
        p.flags = ws.flags;
        p.custom_map_index = ws.custom_map_index;
#endif

        CClientManager::instance().ForwardPacket(HEADER_DG_GUILD_WAR, &p, sizeof(p));
        sys_log(0, "GuildWar: GUILD sending start of wait start war %d %d", ws.GID[0], ws.GID[1]);
    }
}

 

Edited October 17, 2025 by Kidro

[aXseee 0]

On 9/16/2025 at 2:46 PM, Distraught said:

Hello,

I found a crash in the DB’s marriage manager today. It’s pretty simple: we keep a reference to a value, then erase it from the map, and later try to access it again.
I’ve attached a picture to show what’s happening.

 

To fix this crash, open Marriage.cpp in db and search for CManager::Update function.
Then remove the reference (&) and move the erase after it (the latter is just for code clarity).

Before:

TWedding& w = it->second;

TPacketWeddingEnd p;
p.dwPID1 = w.dwPID1;
p.dwPID2 = w.dwPID2;
CClientManager::instance().ForwardPacket(HEADER_DG_WEDDING_END, &p, sizeof(p));
m_mapRunningWedding.erase(it);

 

After:

TWedding w = it->second;
m_mapRunningWedding.erase(it);

TPacketWeddingEnd p;
p.dwPID1 = w.dwPID1;
p.dwPID2 = w.dwPID2;
CClientManager::instance().ForwardPacket(HEADER_DG_WEDDING_END, &p, sizeof(p));

 

Naah, just move the erase right after the find. why would you create a temporary copy in memory when you can simply use a reference?

[Distraught 9951]

6 hours ago, DenizCALISKAN said:

Naah, just move the erase right after the find. why would you create a temporary copy in memory when you can simply use a reference?

It's a trivially copyable type, only consists of a few integers. This way, it's much more readable and easier for people to follow what I wrote.

Btw, compilers are clever and will only use one instruction to copy all the 16 bytes as the default allocator of the standard library must guarantee aligned allocations. So, this means copying them or using a reference is not much different but the intention is much more readable this way.

Also, if you wanna go down the micro-optimization path, using a local variable generally opens up the gate for more extensive optimizations by the compiler.

 

So, if your intention was to flex expertise, learning how compilers work would be a better way to spend your time.

Edited November 1, 2025 by Distraught

[Matteo 69]

4 hours ago, Distraught said:

It's a trivially copyable type, only consists of a few integers. This way, it's much more readable and easier for people to follow what I wrote.

Btw, compilers are clever and will only use one instruction to copy all the 16 bytes as the default allocator of the standard library must guarantee aligned allocations. So, this means copying them or using a reference is not much different but the intention is much more readable this way.

Also, if you wanna go down the micro-optimization path, using a local variable generally opens up the gate for more extensive optimizations by the compiler.

 

So, if your intention was to flex expertise, learning how compilers work would be a better way to spend your time.

Edited February 18 by Metin2 Dev International
Core X - External 2 Internal