Larry Watterson

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: "DeleteLoginData" is never called "SetDeleted" is never called "IsDeleted()" always returns false 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): [Hidden Content] 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.

Found this while going through CreateItem. Near the end of the function: item->SetVID(++m_dwVIDCount); if (bSkipSave == false) m_VIDMap.emplace(item->GetVID(), item); if (item->GetID() != 0 && bSkipSave == false) m_map_pkItemByID.emplace(item->GetID(), item); if (!item->SetCount(count)) // LOL return NULL; The item gets a VID and gets registered(emplace) in both maps before "SetCount" is even checked. If "SetCount" fails, the function just returns NULL but the item is still in those two maps. The only place that cleans them up is "DestroyItem()", which never gets called here. Right now this isn't actively broken, because the only path where "SetCount" returns false (count == 0 with no owner) already calls M2_DESTROY_ITEM on itself before returning. But that's the only thing saving you. It's relying entirely on "SetCount" funcs current behavior. Add another false-return case to "SetCount" later without cleaning up after itself, and you get a permanent ghost item stuck in m_VIDMap/m_map_pkItemByID, still reachable through Find()/FindByVID(), never saved, never freed. Fix is just reordering, check "SetCount" before registering anything: if (!item->SetCount(count)) [[unlikely]] return nullptr; item->SetVID(++m_dwVIDCount); if (bSkipSave == false) m_VIDMap.emplace(item->GetVID(), item); if (item->GetID() != 0 && bSkipSave == false) m_map_pkItemByID.emplace(item->GetID(), item); This is a cold path so statement probably never triggered right now, but it could turn into a real problem down the line. Found this reading the code, not from a live crash/dupe report, so test it on your own source before dropping it in(especially if you've touched "SetCount" yourselves).

There's a problem here; if the ENABLE_TRANSMUTATION definition doesn't exist, there won't be a memset call for pack2. Btw, you don't need that many memset calls. Use brace initialization. "T x{};" is value-initialization and since structs in Metin2 provide the POD and aggregate definition, the object will be zero-initialized. The result will look something like this: void CShop::BroadcastUpdateItem(BYTE pos) { if (pos >= m_itemVector.size()) // why haven't they added this check? comments are really important. { sys_err("CShop::BroadcastUpdateItem: invalid pos %u (size %u)", pos, m_itemVector.size()); return; } TPacketGCShop pack{}; TPacketGCShopUpdateItem pack2{}; TEMP_BUFFER buf; pack.header = HEADER_GC_SHOP; pack.subheader = SHOP_SUBHEADER_GC_UPDATE_ITEM; pack.size = sizeof(pack) + sizeof(pack2); pack2.pos = pos; const SHOP_ITEM& item = m_itemVector[pos]; if (!(m_pkPC && !item.pkItem)) { pack2.item.vnum = item.vnum; if (item.pkItem) { thecore_memcpy(pack2.item.alSockets, item.pkItem->GetSockets(), sizeof(pack2.item.alSockets)); thecore_memcpy(pack2.item.aAttr, item.pkItem->GetAttributes(), sizeof(pack2.item.aAttr)); #if defined(ENABLE_TRANSMUTATION) pack2.item.dwTransmutationVnum = item.pkItem->GetTransmutationVnum(); #endif } } pack2.item.price = item.price; #if defined(ENABLE_CHEQUE_SYSTEM) pack2.item.cheque = item.cheque; #endif pack2.item.count = item.count; buf.write(&pack, sizeof(pack)); buf.write(&pack2, sizeof(pack2)); Broadcast(buf.read_peek(), buf.size()); }

template <class _Func> void for_each_entity(_Func & func) { itertype(m_set_entity) it = m_set_entity.begin(); for ( ; it != m_set_entity.end(); ++it) { LPENTITY entity = *it; // <Factor> Sanity check if (entity->GetSectree() != this) { sys_err("<Factor> SECTREE-ENTITY relationship mismatch"); m_set_entity.erase(it); continue; } func(entity); } } There is an Undefined Behavior (UB) in this code. The iterator it is being erased, and then a continue statement is executed. Calling erase(it) invalidates the iterator pointing to the erased element, but the for loop's increment expression (++it) then attempts to increment this already invalidated iterator. The traditional fix for this would be writing it = m_set_entity.erase(it) and refactoring the for loop into a while loop (since erase already returns the next valid iterator). Anyway, looking at it from a modern perspective, here is a much cleaner solution utilizing C++20: template <class _Func> void for_each_entity(_Func & func) { std::erase_if(m_set_entity, [this](LPENTITY entity) { if (!entity || entity->GetSectree() != this) { sys_err("<Factor> SECTREE-ENTITY relationship mismatch"); return true; } return false; }); std::ranges::for_each(m_set_entity, std::ref(func)); } Btw you can choice universal ref or const lvalue ref for func parameter, it's related with your use case. It's just example solution.

Long story short, normally you have to call the "Set" member function from CPacketInfoCG's constructor every time you add a new packet. If you forget it, you end up with packet errors at runtime. With this thing, adding an annotation to your new packets is enough. The array gets generated at compile-time. On top of that, you can also add checks like whether there is padding, whether the annotation is there or not, and so on. If you have questions, leave a comment and I'll answer when I have time. Note: I only recently started working with C++26. I was reading the papers and this idea came to my mind. I'm still not very familiar with it, so there might be other use cases I don't know about. If you know any, feel free to share. You need C++26 - GCC16, otherwise it won't work. You can use your brain and turn this into something different too. Mock: refl namespace gist:

A fluent, move-only builder that serializes trivially-copyable packet structs and variable payloads into a buffer, then dispatches it through a send strategy of your choice (as long as it's a stateless, default-initializable functor/stateless lambda). "Why would I need this?", the following mistakes get caught at compile time: -> writing a struct with padding into the buffer as raw bytes -> trying to serialize a pointer You can pass a contiguous range, a forward range, or a single POD as the argument. Contiguous ranges are written in a single block; other forward ranges element-by-element, with byte-identical output. Main benefits: compile-time validation and type safety. Of course, if your take is "99% of the codebase doesn't follow this, why should we" — that's up to you. It works for me, and I just wanted to share something simple. C++23 and GCC 14 will do the job; adapting it to older standards is left to you. The buffer type is customizable, satisfying the concept is enough. I wrote the concept for the default TEMP_BUFFER; adjust it to your own API as needed. Example usage: constexpr auto my_send{[](Character ch, std::span<const std::uint8_t> data, const TEMP_BUFFER& buffer) noexcept { ch.print(); for (auto elm : data) std::cout << static_cast<int>(elm) << '\n'; return 1; }}; const auto built{ make_builder<my_send>() .add_payload(payload_bytes) .add_payload(Payload{.i1 = 1, .i2 = true, .i4 = 2}) .send(ch, payload_bytes)}; const auto pkt{make_builder() .add_payload(std::uint8_t{42}) .add_payload(std::uint8_t{42}) .add_member_val(&MyPacket::size, 3) .build_as<MyPacket>()}; if (pkt) std::cout << pkt->size; PacketBuilder moves the buffer along the chain via std::forward_like. So if you plan to use TEMP_BUFFER with this builder, you'll need to fix the copy/move issue I described in this thread first: If I missed anything, let me know and I'll fix it. Example: [Hidden Content] Source: [Hidden Content]

In C++, if you write a user-declared destructor (which you usually do in resource-managing classes), the compiler implicitly declares the copy constructor and copy assignment operator. `TEMP_BUFFER` manages a resource (has-a `LPBUFFER`), and returns that resource in its destructor (the idiom known as RAII, by Bjarne). But if you have a resource-managing class and you use the compiler-generated copy special member functions, different things can happen to you depending on the scenario. For our case, let's assume we wrote code like this: TEMP_BUFFER b1, b2; b1 = b2; The compiler-generated copy-assign does a shallow copy: `b1`'s old buffer gets overwritten with `b2.buf` without its destructor ever running (i.e. without `buffer_delete` being called). Since `b1.buf == b2.buf`, the same corruption that would occur in the copy-ctor happens here too. So assign = leak + corruption. In the copy-ctor, as far as I can see, there's only corruption. Two objects share the same `buf`, and in the destructor that `buf` goes to `buffer_delete` twice. If the size fits in the pool, `buffer_delete` doesn't call `free`; it puts the buffer back on the free list. When the same `buf` is pushed onto the list twice, logically `buf->next = buf`, meaning the next node points to itself. Then `buffer_new` hands out the same `buf` to two separate calls, and you can potentially run into something like a heap overflow or an infinite loop. Classic corruption, blowing up in some unrelated place, is my guess. The simplest fix is to explicitly delete the copy-ctor and copy-assign operator as a preventive measure; I doubt anyone in their right mind would copy a `TEMP_BUFFER` anyway. TEMP_BUFFER(const TEMP_BUFFER&) = delete; TEMP_BUFFER& operator=(const TEMP_BUFFER&) = delete; But if you want following the rule of five: TEMP_BUFFER::TEMP_BUFFER(const TEMP_BUFFER& other) : buf{(other.buf != nullptr) ? buffer_new(other.buf->mem_size) : nullptr} , forceDelete{other.forceDelete} { if (other.buf != nullptr) { buffer_write(buf, other.buf->mem_data, other.buf->write_point_pos); } } TEMP_BUFFER& TEMP_BUFFER::operator=(const TEMP_BUFFER& other) { if (this != &other) { if (buf != nullptr) { buffer_delete(buf); } forceDelete = other.forceDelete; buf = (other.buf != nullptr) ? buffer_new(other.buf->mem_size) : nullptr; if (other.buf != nullptr) { buffer_write(buf, other.buf->mem_data, other.buf->write_point_pos); } } return *this; } TEMP_BUFFER::TEMP_BUFFER(TEMP_BUFFER&& other) noexcept : buf{std::exchange(other.buf, nullptr)} , forceDelete{std::exchange(other.forceDelete, false)} { } TEMP_BUFFER& TEMP_BUFFER::operator=(TEMP_BUFFER&& other) noexcept { if (this != &other) { if (buf != nullptr) { buffer_delete(buf); } buf = std::exchange(other.buf, nullptr); forceDelete = std::exchange(other.forceDelete, false); } return *this; } Note: this deep copy only copy over the written data; `read_point` and the flag are not copied, so it doesn't preserve the full state of a half-read buffer. Since it's used for writing when building packets it's not a problem, but use it knowing this. Note 2: as I said, it's not a critical thing, you've most likely never copied the buffer at all. But keep similar situations in mind for other resource-managing classes as well. One warning: when there's a user-declared destructor, the move ctor and move assign operator are not implicitly declared by the compiler, meaning once you delete the copies the type becomes entirely non-movable. If you use it somewhere that `std::move`s the buffer (like a fluent API), you'll need to write the move ctor/assign yourself, I've written an example above, but take a look before using it, there might be spots in the buffer logic I missed; if there are, mention them in the comments and i'll fix it. Deleted copy move and assign operator alone is enough for pure prevention, but if it were me I'd write a more reasonable, modern buffer mechanism. Btw, in the copy assignment operator, you can prefer the copy-and-swap idiom. I wrote the special member functions simply, just as examples, so don't focus on them too much. If you are going to use them, the implementation is up to you. Don't act without checking the APIs like buffer_new, etc. The responsibility is yours. mock: [Hidden Content] Enjoy.

In short, when parsing DB data, functions like std::stoi are used. These throw exceptions if there is an error, but nobody wraps them in a try-catch block. Without getting into too much technical detail, this approach gives you a noexcept guarantee. It prevents potential terminate issues that happen when an exception isn't caught Parser: [Hidden Content] Demo: [Hidden Content]

Just follow document, it's easy. What's your problem?

Hi, i coded in October, and iI was planning to update it, but I haven't had much time. If I find the time, I might release an update. Enjoy using it! Ranks the damage dealt by players attacking a mob. Any mob can be dynamically added to or removed from the ranking system in real-time. Displays the top 5 players with the highest damage on a table (configurable). The damage bar of the player who poisons the mob is displayed in "green." Can be used for mobs with the same vnum on the same map, even if they are located at different points (works completely dynamically). Mobs included in the ranking cannot recovery hp. Minimum C++ version required: 17 (some parts use C++20 features; you can adjust it as needed). The PythonHelper file is not mine. [Hidden Content] [Hidden Content]

I think it's better, thanks for idea. Best regards. [Hidden Content]

Still have memory leak. Check "CreateDropItem" function definition.

Download Alternative download links → Github Working like JS SetInterval function. You can use for event and character timers. Container for events if u using on characters i think u create smart pointer in character class, codes are developable.

Download Metin2 Download This is just a example, developable. The system determines the winner by randomly choosing one of the numbers on the map, it can be done as an instant event. [Hidden Content]

Yea i know but thi is just a simple example ^^ you're welcome