[DB] CLoginData memory leak

[Larry Watterson 484]

TL;DR: The YMIR devs wrote a mechanism to clean up duplicate logins, but they forgot to connect it to the normal logout path. So the object is never freed.

Let's say our player is Bilal.

Bilal logs into the game. What happens in the background?

QUERY_AUTH_LOGIN runs, and a CLoginData object is allocated on the free store. Four different containers point to this object with a raw pointer. In theory these are "non-owning" pointers, but that is not really true (I really wish it was), because there is no single type in the codebase that owns the object. All four of them behave like an owner. There is some kind of refcount, but there is no documentation at all, so I could not really understand it. It looks like someone tried to imitate shared_ptr by hand, probably to avoid dangling pointers. I am not going to spend 10 hours trying to understand this code, but I will still complain a little.

If you ask "how is that possible?", just look at the non-static data members:

• m_map_pkLoginData // login key -> object
• m_map_pkLoginDataByLogin // "bilal123" -> object
• m_map_pkLoginDataByAID // account id -> object
• m_map_kLogonAccount // "bilal123" -> object

So... yeah. Anyway.

Now let's say Bilal plays for a while and then logs out, or just presses Alt+F4. What happens now? QUERY_LOGOUT runs, and it calls DeleteLogonAccount("bilal123").

The flow of DeleteLogonAccount looks like this:

 if (pkLD->IsDeleted()) // hmmmmmm
	delete pkLD;

m_map_kLogonAccount.erase(it);

"What is IsDeleted()?" It is an accessor that returns the "m_bDeleted" non-static data member. The only place that sets it to true is the "SetDeleted" mutator, and "SetDeleted" is only called inside "DeleteLoginData". The only caller of "DeleteLoginData" is QUERY_AUTH_LOGIN. In other words: the same account has to log in again.

So let's think about it.

What happens if Bilal logs in and out one time on a server where the DB process never restarts, and then never plays again? Actually that would be great for the server, but here is what really happens: QUERY_AUTH_LOGIN never runs again for him, and because of that:

1. "DeleteLoginData" is never called
2. "SetDeleted" is never called
3. "IsDeleted()" always returns false
4. the "delete pkLD;" expression never runs

Btw, the same pattern is repeated in RemovePeer(), and this time it has nothing to do with Bilal:

 if (pkLD->IsDeleted()) { delete pkLD; }

m_map_kLogonAccount.erase(it++);

Here the player is not the problem. If a game server peer drops (crash or restart, you decide), the code tries to remove every account connected through that peer from m_map_kLogonAccount. But again, in normal conditions IsDeleted() is always false.

That means the moment a game server restarts for any reason, the CLoginData of every player who was online at that moment is never freed. (How many players that is, you decide.)

"So what?" Well, this is exactly CWE-772 (missing release of resource after effective lifetime). Please go and read about it.

As far as I can see, there is no dangling pointer and no use-after-free. The object is still valid and still reachable from the other three containers. The problem is different: the object has finished its job, but the memory is never released. It would be difficult to build an attack scenario around this, and someone would probably notice it. But you don't even need an attacker here: normal player traffic is already enough to make this a real problem, and fixing it helps your uptime.

Fix (for people who want to keep using naked pointers):

 

This is the hidden content, please

• Sign In
• or
• Sign Up

 

Important: you have to erase the iterator first. If you don't erase it before the call, DeleteLoginData has no meaning at all.

 

Note: I haven't checked the warp flow yet. There is clearly a problem, but deleting it directly could cause other issues. Metin2 codebase like a minefield. Let's apply this, and if anyone runs into a problem and reports it, I can analyze and fix it.

Edited September 1 by Larry Watterson