From b5ea548038679c6a4de5012495d9fcc24fa571bc Mon Sep 17 00:00:00 2001 From: Exynox Date: Sun, 27 Nov 2022 01:10:23 +0200 Subject: [PATCH 1/3] Fixed off-by-one buffer overrun in map_allow_copy() function which would lead to malformed initialization network packets. Added address sanitizer in db CMake. --- db/CMakeLists.txt | 3 +++ game/src/config.cpp | 12 +++++------- game/src/config.h | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/db/CMakeLists.txt b/db/CMakeLists.txt index f9307a2..b41f783 100644 --- a/db/CMakeLists.txt +++ b/db/CMakeLists.txt @@ -26,3 +26,6 @@ find_package(Libevent CONFIG REQUIRED) target_link_libraries(${PROJECT_NAME} PRIVATE libevent::core libevent::extra libevent::pthreads) target_link_libraries(${PROJECT_NAME} PRIVATE libpoly libsql libthecore) + +# Memory debugging +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address -g") diff --git a/game/src/config.cpp b/game/src/config.cpp index 4669444..ba195c9 100644 --- a/game/src/config.cpp +++ b/game/src/config.cpp @@ -169,18 +169,16 @@ void map_allow_add(int index) s_set_map_allows.insert(index); } -void map_allow_copy(LONG * pl, int size) +void map_allow_copy(int * pl, int size) { int iCount = 0; - std::set::iterator it = s_set_map_allows.begin(); - while (it != s_set_map_allows.end()) + for (auto mapId: s_set_map_allows) { - int i = *(it++); - *(pl++) = i; + if (iCount >= size) + break; - if (++iCount > size) - break; + pl[iCount++] = mapId; } } diff --git a/game/src/config.h b/game/src/config.h index 2ccfa7e..639b5f4 100644 --- a/game/src/config.h +++ b/game/src/config.h @@ -37,7 +37,7 @@ extern bool g_bTrafficProfileOn; ///< true extern BYTE g_bChannel; extern bool map_allow_find(int index); -extern void map_allow_copy(LONG * pl, int size); +extern void map_allow_copy(int * pl, int size); extern bool no_wander; extern int g_iUserLimit; From 972530f3a74a4297263f23783a13dde7a8e176a2 Mon Sep 17 00:00:00 2001 From: Exynox Date: Sun, 27 Nov 2022 10:46:56 +0200 Subject: [PATCH 2/3] Fixed serious issue where oversized packets would be split apart by libevent without proper handling by the db core. Removed Google Sanitizers --- db/CMakeLists.txt | 3 --- db/src/Peer.cpp | 20 +++++++++++++++++++- game/CMakeLists.txt | 1 - 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/db/CMakeLists.txt b/db/CMakeLists.txt index b41f783..f9307a2 100644 --- a/db/CMakeLists.txt +++ b/db/CMakeLists.txt @@ -26,6 +26,3 @@ find_package(Libevent CONFIG REQUIRED) target_link_libraries(${PROJECT_NAME} PRIVATE libevent::core libevent::extra libevent::pthreads) target_link_libraries(${PROJECT_NAME} PRIVATE libpoly libsql libthecore) - -# Memory debugging -set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address -g") diff --git a/db/src/Peer.cpp b/db/src/Peer.cpp index 8b2292e..1801102 100644 --- a/db/src/Peer.cpp +++ b/db/src/Peer.cpp @@ -63,12 +63,19 @@ void CPeer::SetUserCount(DWORD dwCount) bool CPeer::PeekPacket(int & iBytesProceed, BYTE & header, DWORD & dwHandle, DWORD & dwLength, const char ** data) { + // Return if not enough data was received to read the header if (GetRecvLength() < iBytesProceed + 9) return false; const char * buf = (const char *) GetRecvBuffer(iBytesProceed + 9); + if (!buf) { + sys_err("PeekPacket: Failed to get network buffer!"); + return false; + } + buf += iBytesProceed; + // Read the header data header = *(buf++); dwHandle = *((DWORD *) buf); @@ -77,7 +84,7 @@ bool CPeer::PeekPacket(int & iBytesProceed, BYTE & header, DWORD & dwHandle, DWO dwLength = *((DWORD *) buf); buf += sizeof(DWORD); - //sys_log(0, "%d header %d handle %u length %u", GetRecvLength(), header, dwHandle, dwLength); + // Ensure that all the data was fully received if (iBytesProceed + dwLength + 9 > (DWORD) GetRecvLength()) { sys_log(0, "PeekPacket: not enough buffer size: len %u, recv %d", @@ -85,6 +92,17 @@ bool CPeer::PeekPacket(int & iBytesProceed, BYTE & header, DWORD & dwHandle, DWO return false; } + // Ensure that all the required data is available in a contiguous area + buf = (const char *) GetRecvBuffer(iBytesProceed + dwLength + 9); + if (!buf) { + sys_err("PeekPacket: Failed to get network buffer!"); + return false; + } + + // Skip the header + buf += iBytesProceed + 9; + + // Set the data pointer *data = buf; iBytesProceed += dwLength + 9; return true; diff --git a/game/CMakeLists.txt b/game/CMakeLists.txt index 25d8b4f..c33277e 100644 --- a/game/CMakeLists.txt +++ b/game/CMakeLists.txt @@ -49,4 +49,3 @@ find_package (Threads REQUIRED) target_link_libraries (${PROJECT_NAME} Threads::Threads) target_link_libraries(${PROJECT_NAME} libgame libpoly libsql libthecore liblua) - From 868b8394bb970e2c72a689149b0aab553188156b Mon Sep 17 00:00:00 2001 From: Exynox Date: Sun, 27 Nov 2022 11:56:23 +0200 Subject: [PATCH 3/3] Fixed usage of uninitialized variables. --- db/src/PeerBase.cpp | 2 +- game/src/battle.cpp | 16 +++++----------- game/src/battle.h | 2 ++ game/src/char.h | 4 ++-- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/db/src/PeerBase.cpp b/db/src/PeerBase.cpp index a037f9d..1e039d2 100644 --- a/db/src/PeerBase.cpp +++ b/db/src/PeerBase.cpp @@ -90,7 +90,7 @@ void CPeerBase::Encode(const void* data, size_t size) return; } - if(bufferevent_write(m_bufferevent, data, size) != 0) { + if (bufferevent_write(m_bufferevent, data, size) != 0) { sys_err("Buffer write error!"); return; } diff --git a/game/src/battle.cpp b/game/src/battle.cpp index 98bf17b..68a7d2b 100644 --- a/game/src/battle.cpp +++ b/game/src/battle.cpp @@ -22,8 +22,6 @@ #include "ani.h" #include "locale_service.h" -int battle_hit(LPCHARACTER ch, LPCHARACTER victim, int & iRetDam); - bool battle_distance_valid_by_xy(int x, int y, int tx, int ty) { int distance = DISTANCE_APPROX(x - tx, y - ty); @@ -161,9 +159,7 @@ int battle_melee_attack(LPCHARACTER ch, LPCHARACTER victim) const PIXEL_POSITION & vpos = victim->GetXYZ(); ch->SetRotationToXY(vpos.x, vpos.y); - int dam; - int ret = battle_hit(ch, victim, dam); - return (ret); + return battle_hit(ch, victim); } // 실제 GET_BATTLE_VICTIM을 NULL로 만들고 이벤트를 캔슬 시킨다. @@ -633,12 +629,8 @@ void NormalAttackAffect(LPCHARACTER pkAttacker, LPCHARACTER pkVictim) AttackAffect(pkAttacker, pkVictim, POINT_SLOW_PCT, IMMUNE_SLOW, AFFECT_SLOW, POINT_MOV_SPEED, -30, AFF_SLOW, 20, "SLOW"); } -int battle_hit(LPCHARACTER pkAttacker, LPCHARACTER pkVictim, int & iRetDam) +int battle_hit(LPCHARACTER pkAttacker, LPCHARACTER pkVictim) { - //PROF_UNIT puHit("Hit"); - if (test_server) - sys_log(0, "battle_hit : [%s] attack to [%s] : dam :%d type :%d", pkAttacker->GetName(), pkVictim->GetName(), iRetDam); - int iDam = CalcMeleeDamage(pkAttacker, pkVictim); if (iDam <= 0) @@ -684,7 +676,9 @@ int battle_hit(LPCHARACTER pkAttacker, LPCHARACTER pkVictim, int & iRetDam) float tempIDam = iDam; iDam = attMul * tempIDam + 0.5f; - iRetDam = iDam; + //PROF_UNIT puHit("Hit"); + if (test_server) + sys_log(0, "battle_hit : [%s] attack to [%s] : dam: %d", pkAttacker->GetName(), pkVictim->GetName(), iDam); //PROF_UNIT puDam("Dam"); if (pkVictim->Damage(pkAttacker, iDam, DAMAGE_TYPE_NORMAL)) diff --git a/game/src/battle.h b/game/src/battle.h index 89a45c1..d0d5ebc 100644 --- a/game/src/battle.h +++ b/game/src/battle.h @@ -28,6 +28,8 @@ extern int battle_count_attackers(LPCHARACTER ch); extern void NormalAttackAffect(LPCHARACTER pkAttacker, LPCHARACTER pkVictim); +extern int battle_hit(LPCHARACTER ch, LPCHARACTER victim); + // 특성 공격 inline void AttackAffect(LPCHARACTER pkAttacker, LPCHARACTER pkVictim, diff --git a/game/src/char.h b/game/src/char.h index c2ba563..4fe039f 100644 --- a/game/src/char.h +++ b/game/src/char.h @@ -1982,8 +1982,8 @@ class CHARACTER : public CEntity, public CFSM, public CHorseRider //독일 선물 기능 패킷 임시 저장 private: - unsigned int itemAward_vnum; - char itemAward_cmd[20]; + unsigned int itemAward_vnum = 0; + char itemAward_cmd[20] = ""; //bool itemAward_flag; public: unsigned int GetItemAward_vnum() { return itemAward_vnum; }