diff --git a/.gitignore b/.gitignore index d7af5d1f6..39f7c74f6 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ temp/ cmake-build-debug/ RelWithDebInfo/ docker/configs +valgrind-out.txt # Third party libraries thirdparty/mysql/ diff --git a/CMakeLists.txt b/CMakeLists.txt index be72a3a2d..da9245b04 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,6 +4,18 @@ project(Darkflame LANGUAGES C CXX ) +# Sanitizer flags - TODO: Make CMake preset before finalizing PR +if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU" AND NOT APPLE) + add_compile_options("-fsanitize=undefined") + add_link_options("-fsanitize=undefined") + + if (CMAKE_CXX_COMPILER_ID MATCHES "Clang") + add_link_options("-static-libsan") + else() + add_link_options("-static-libasan") + endif() +endif() + # check if the path to the source directory contains a space if("${CMAKE_SOURCE_DIR}" MATCHES " ") message(FATAL_ERROR "The server cannot build in the path (" ${CMAKE_SOURCE_DIR} ") because it contains a space. Please move the server to a path without spaces.") @@ -17,7 +29,7 @@ set(CMAKE_C_STANDARD_REQUIRED ON) set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_EXPORT_COMPILE_COMMANDS ON) # Export the compile commands for debugging set(CMAKE_POLICY_DEFAULT_CMP0063 NEW) # Set CMAKE visibility policy to NEW on project and subprojects -set(CMAKE_VISIBILITY_INLINES_HIDDEN ON) # Set C and C++ symbol visibility to hide inlined functions +set(CMAKE_VISIBILITY_INLINES_HIDDEN OFF) # Set C and C++ symbol visibility to hide inlined functions set(CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake") # Read variables from file @@ -262,7 +274,7 @@ if(MSVC) # add_compile_options("/W4") # Want to enable warnings eventually, but WAY too much noise right now elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU") - add_compile_options("-Wuninitialized" "-Wold-style-cast") + add_compile_options("-Wuninitialized" "-Wold-style-cast" "-Wstrict-aliasing=01") else() message(WARNING "Unknown compiler: '${CMAKE_CXX_COMPILER_ID}' - No warning flags enabled.") endif() diff --git a/cmake/toolchains/linux-clang.cmake b/cmake/toolchains/linux-clang.cmake index b4578a38b..1e1ed5c3f 100644 --- a/cmake/toolchains/linux-clang.cmake +++ b/cmake/toolchains/linux-clang.cmake @@ -1,6 +1,7 @@ # Try and find a clang-16 install, falling back to a generic clang install otherwise find_program(CLANG_C_COMPILER clang-16 | clang REQUIRED) find_program(CLANG_CXX_COMPILER clang++-16 | clang++ REQUIRED) +find_program(CLANG_CXX_LINKER lld REQUIRED) # Debug messages message(DEBUG "CLANG_C_COMPILER = ${CLANG_C_COMPILER}") diff --git a/dCommon/Amf3.h b/dCommon/Amf3.h index 21a3c0c74..4c9f49d09 100644 --- a/dCommon/Amf3.h +++ b/dCommon/Amf3.h @@ -1,9 +1,10 @@ -#ifndef __AMF3__H__ -#define __AMF3__H__ +#ifndef AMF3_H +#define AMF3_H #include "dCommonVars.h" #include "Logger.h" #include "Game.h" +#include "GeneralUtils.h" #include #include @@ -74,28 +75,15 @@ template <> [[nodiscard]] constexpr eAmf AMFValue::GetValueType() const template [[nodiscard]] constexpr eAmf AMFValue::GetValueType() const noexcept { return eAmf::Undefined; } -// As a string this is much easier to write and read from a BitStream. -template <> -class AMFValue : public AMFBaseValue { -public: - AMFValue() = default; - AMFValue(const char* value) { m_Data = value; } - virtual ~AMFValue() override = default; - - [[nodiscard]] constexpr eAmf GetValueType() const noexcept override { return eAmf::String; } - - [[nodiscard]] const std::string& GetValue() const { return m_Data; } - void SetValue(const std::string& value) { m_Data = value; } -protected: - std::string m_Data; -}; - using AMFNullValue = AMFValue; using AMFBoolValue = AMFValue; using AMFIntValue = AMFValue; using AMFStringValue = AMFValue; using AMFDoubleValue = AMFValue; +// Template deduction guide to ensure string literals deduce +AMFValue(const char*) -> AMFValue; // AMFStringValue + /** * The AMFArrayValue object holds 2 types of lists: * An associative list where a key maps to a value @@ -106,7 +94,7 @@ using AMFDoubleValue = AMFValue; */ class AMFArrayValue : public AMFBaseValue { using AMFAssociative = - std::unordered_map, GeneralUtils::transparent_string_hash, std::equal_to<>>; + std::unordered_map, GeneralUtils::transparent_string_hash, std::equal_to>; using AMFDense = std::vector>; @@ -137,17 +125,20 @@ class AMFArrayValue : public AMFBaseValue { * @return The inserted element if the type matched, * or nullptr if a key existed and was not the same type */ - template - [[maybe_unused]] std::pair*, bool> Insert(const std::string_view key, const ValueType value) { + template + [[maybe_unused]] auto Insert(const std::string_view key, const T value) -> std::pair { + // This ensures the deduced type matches the AMFValue constructor + using AMFValueType = decltype(AMFValue(value)); + const auto element = m_Associative.find(key); - AMFValue* val = nullptr; + AMFValueType* val = nullptr; bool found = true; if (element == m_Associative.cend()) { - auto newVal = std::make_unique>(value); + auto newVal = std::make_unique(value); val = newVal.get(); m_Associative.emplace(key, std::move(newVal)); } else { - val = dynamic_cast*>(element->second.get()); + val = dynamic_cast(element->second.get()); found = false; } return std::make_pair(val, found); @@ -190,15 +181,18 @@ class AMFArrayValue : public AMFBaseValue { * @return The inserted element, or nullptr if the type did not match * what was at the index. */ - template - [[maybe_unused]] std::pair*, bool> Insert(const size_t index, const ValueType value) { + template + [[maybe_unused]] auto Insert(const size_t index, const T value) -> std::pair { + // This ensures the deduced type matches the AMFValue constructor + using AMFValueType = decltype(AMFValue(value)); + bool inserted = false; if (index >= m_Dense.size()) { m_Dense.resize(index + 1); - m_Dense.at(index) = std::make_unique>(value); + m_Dense.at(index) = std::make_unique(value); inserted = true; } - return std::make_pair(dynamic_cast*>(m_Dense.at(index).get()), inserted); + return std::make_pair(dynamic_cast(m_Dense.at(index).get()), inserted); } /** @@ -245,8 +239,8 @@ class AMFArrayValue : public AMFBaseValue { * * @return The inserted pointer, or nullptr should the key already be in use. */ - template - [[maybe_unused]] inline AMFValue* Push(const ValueType value) { + template + [[maybe_unused]] inline auto Push(const T value) -> decltype(AMFValue(value))* { return Insert(m_Dense.size(), value).first; } @@ -356,4 +350,4 @@ class AMFArrayValue : public AMFBaseValue { AMFDense m_Dense; }; -#endif //!__AMF3__H__ +#endif //!AMF3_H diff --git a/dCommon/AmfSerialize.cpp b/dCommon/AmfSerialize.cpp index e11ae1de4..db417e493 100644 --- a/dCommon/AmfSerialize.cpp +++ b/dCommon/AmfSerialize.cpp @@ -10,40 +10,54 @@ void RakNet::BitStream::Write(AMFBaseValue& value) { this->Write(type); switch (type) { case eAmf::Integer: { - this->Write(*static_cast(&value)); + this->Write(static_cast(value)); break; } case eAmf::Double: { - this->Write(*static_cast(&value)); + this->Write(static_cast(value)); break; } case eAmf::String: { - this->Write(*static_cast(&value)); + this->Write(static_cast(value)); break; } case eAmf::Array: { - this->Write(*static_cast(&value)); + this->Write(static_cast(value)); break; } default: { LOG("Encountered unwritable AMFType %i!", type); + [[fallthrough]]; } case eAmf::Undefined: + [[fallthrough]]; case eAmf::Null: + [[fallthrough]]; case eAmf::False: + [[fallthrough]]; case eAmf::True: + [[fallthrough]]; case eAmf::Date: + [[fallthrough]]; case eAmf::Object: + [[fallthrough]]; case eAmf::XML: + [[fallthrough]]; case eAmf::XMLDoc: + [[fallthrough]]; case eAmf::ByteArray: + [[fallthrough]]; case eAmf::VectorInt: + [[fallthrough]]; case eAmf::VectorUInt: + [[fallthrough]]; case eAmf::VectorDouble: + [[fallthrough]]; case eAmf::VectorObject: + [[fallthrough]]; case eAmf::Dictionary: break; } @@ -145,8 +159,7 @@ void RakNet::BitStream::Write(AMFIntValue& value) { // Writes an AMFDoubleValue to BitStream template<> void RakNet::BitStream::Write(AMFDoubleValue& value) { - double d = value.GetValue(); - WriteAMFU64(*this, *reinterpret_cast(&d)); + WriteAMFU64(*this, std::bit_cast(value.GetValue())); } // Writes an AMFStringValue to BitStream diff --git a/dCommon/BrickByBrickFix.cpp b/dCommon/BrickByBrickFix.cpp index 5aef091a8..7d64760f3 100644 --- a/dCommon/BrickByBrickFix.cpp +++ b/dCommon/BrickByBrickFix.cpp @@ -146,7 +146,7 @@ void WriteSd0Magic(char* input, uint32_t chunkSize) { input[2] = '0'; input[3] = 0x01; input[4] = 0xFF; - *reinterpret_cast(input + 5) = chunkSize; // Write the integer to the character array + std::memcpy(&input[5], &chunkSize, sizeof(uint32_t)); // Write the integer to the character array } bool CheckSd0Magic(std::istream& streamToCheck) { diff --git a/dCommon/GeneralUtils.cpp b/dCommon/GeneralUtils.cpp index 34d0aefba..f860cdba3 100644 --- a/dCommon/GeneralUtils.cpp +++ b/dCommon/GeneralUtils.cpp @@ -53,9 +53,9 @@ bool static _IsSuffixChar(const uint8_t c) { bool GeneralUtils::details::_NextUTF8Char(std::string_view& slice, uint32_t& out) { const size_t rem = slice.length(); if (slice.empty()) return false; - const uint8_t* bytes = reinterpret_cast(&slice.front()); + const char* const bytes = slice.data(); if (rem > 0) { - const uint8_t first = bytes[0]; + const uint8_t first = static_cast(bytes[0]); if (first < 0x80) { // 1 byte character out = static_cast(first & 0x7F); slice.remove_prefix(1); @@ -64,7 +64,7 @@ bool GeneralUtils::details::_NextUTF8Char(std::string_view& slice, uint32_t& out // middle byte, not valid at start, fall through } else if (first < 0xE0) { // two byte character if (rem > 1) { - const uint8_t second = bytes[1]; + const uint8_t second = static_cast(bytes[1]); if (_IsSuffixChar(second)) { out = (static_cast(first & 0x1F) << 6) + static_cast(second & 0x3F); @@ -74,8 +74,8 @@ bool GeneralUtils::details::_NextUTF8Char(std::string_view& slice, uint32_t& out } } else if (first < 0xF0) { // three byte character if (rem > 2) { - const uint8_t second = bytes[1]; - const uint8_t third = bytes[2]; + const uint8_t second = static_cast(bytes[1]); + const uint8_t third = static_cast(bytes[2]); if (_IsSuffixChar(second) && _IsSuffixChar(third)) { out = (static_cast(first & 0x0F) << 12) + (static_cast(second & 0x3F) << 6) @@ -86,9 +86,9 @@ bool GeneralUtils::details::_NextUTF8Char(std::string_view& slice, uint32_t& out } } else if (first < 0xF8) { // four byte character if (rem > 3) { - const uint8_t second = bytes[1]; - const uint8_t third = bytes[2]; - const uint8_t fourth = bytes[3]; + const uint8_t second = static_cast(bytes[1]); + const uint8_t third = static_cast(bytes[2]); + const uint8_t fourth = static_cast(bytes[3]); if (_IsSuffixChar(second) && _IsSuffixChar(third) && _IsSuffixChar(fourth)) { out = (static_cast(first & 0x07) << 18) + (static_cast(second & 0x3F) << 12) diff --git a/dCommon/GeneralUtils.h b/dCommon/GeneralUtils.h index 2c93b656d..b1d497281 100644 --- a/dCommon/GeneralUtils.h +++ b/dCommon/GeneralUtils.h @@ -3,6 +3,7 @@ // C++ #include #include +#include #include #include #include @@ -137,7 +138,7 @@ namespace GeneralUtils { template struct overload : Bases... { using is_transparent = void; - using Bases::operator() ... ; + using Bases::operator()...; }; struct char_pointer_hash { @@ -152,6 +153,26 @@ namespace GeneralUtils { char_pointer_hash >; + /** + * A convenience wrapper around std::memcpy that creates a new object + * from the value representation of the provided bytes.Use when + * std::bit_cast is not applicable. + * @warning All restrictions of std::memcpy still apply. Accessing + * outside of the source object is undefined behavior. + * @param from The source of the value representation + * @returns A new object with the given value representation + */ + template + [[nodiscard]] + inline To FromBitsUnchecked(const From* from) noexcept + requires (std::is_trivially_copyable_v + && std::is_trivially_copyable_v) + { + To to{}; + std::memcpy(&to, from, sizeof(To)); + return to; + } + // Concept constraining to enum types template concept Enum = std::is_enum_v; diff --git a/dCommon/dClient/AssetManager.h b/dCommon/dClient/AssetManager.h index 2116f2237..73e6162a9 100644 --- a/dCommon/dClient/AssetManager.h +++ b/dCommon/dClient/AssetManager.h @@ -54,6 +54,7 @@ struct AssetStream : std::istream { } operator bool() { + // NEED TO FIX THIS return reinterpret_cast(rdbuf())->m_Success; } }; diff --git a/dCommon/dEnums/MessageType/World.h b/dCommon/dEnums/MessageType/World.h index acca62466..00b787b93 100644 --- a/dCommon/dEnums/MessageType/World.h +++ b/dCommon/dEnums/MessageType/World.h @@ -5,7 +5,8 @@ namespace MessageType { enum class World : uint32_t { - VALIDATION = 1, // Session info + INVALID = 0, + VALIDATION, // Session info CHARACTER_LIST_REQUEST, CHARACTER_CREATE_REQUEST, LOGIN_REQUEST, // Character selected diff --git a/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp b/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp index fc724fb9e..e2dbb24db 100644 --- a/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp +++ b/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp @@ -82,6 +82,7 @@ void NpcAgCourseStarter::OnFireEventServerSide(Entity* self, Entity* sender, std LWOOBJID_EMPTY, "", senderSysAddr); scriptedActivityComponent->RemoveActivityPlayerData(senderId); } else if (args == "course_finish") { + const auto raceEndTime = Game::server->GetUptime(); const auto fRaceEndTime = std::chrono::duration>(raceEndTime).count(); const auto raceTimeElapsed = fRaceEndTime - data->values[1]; diff --git a/dWorldServer/WorldServer.cpp b/dWorldServer/WorldServer.cpp index c3dbeaca9..5610b23b3 100644 --- a/dWorldServer/WorldServer.cpp +++ b/dWorldServer/WorldServer.cpp @@ -1396,7 +1396,9 @@ void HandlePacket(Packet* packet) { } default: - const auto messageId = *reinterpret_cast(&packet->data[3]); + // Need to use FromBitsUnchecked (aka memcpy) instead of reinterpret_cast to avoid misaligned reads, which are UB + const auto messageId = GeneralUtils::FromBitsUnchecked(&packet->data[3]); + const std::string_view messageIdString = StringifiedEnum::ToString(messageId); LOG("Unknown world packet received: %4i, %s", messageId, messageIdString.data()); } diff --git a/tests/dCommonTests/Amf3Tests.cpp b/tests/dCommonTests/Amf3Tests.cpp index 79d0d496d..a12d2ab8b 100644 --- a/tests/dCommonTests/Amf3Tests.cpp +++ b/tests/dCommonTests/Amf3Tests.cpp @@ -71,7 +71,7 @@ TEST(dCommonTests, AMF3InsertionAssociativeTest) { array.Insert>("Undefined", {}); array.Insert("Null", nullptr); - ASSERT_EQ(array.Get("CString")->GetValueType(), eAmf::String); + ASSERT_EQ(array.Get("CString")->GetValueType(), eAmf::String); ASSERT_EQ(array.Get("String")->GetValueType(), eAmf::String); ASSERT_EQ(array.Get("False")->GetValueType(), eAmf::False); ASSERT_EQ(array.Get("True")->GetValueType(), eAmf::True); @@ -95,7 +95,7 @@ TEST(dCommonTests, AMF3InsertionDenseTest) { array.Push>({}); ASSERT_EQ(array.Get(0)->GetValueType(), eAmf::String); - ASSERT_EQ(array.Get(1)->GetValueType(), eAmf::String); + ASSERT_EQ(array.Get(1)->GetValueType(), eAmf::String); ASSERT_EQ(array.Get(2)->GetValueType(), eAmf::False); ASSERT_EQ(array.Get(3)->GetValueType(), eAmf::True); ASSERT_EQ(array.Get(4)->GetValueType(), eAmf::Integer); diff --git a/valgrind.sh b/valgrind.sh new file mode 100644 index 000000000..17499486c --- /dev/null +++ b/valgrind.sh @@ -0,0 +1 @@ +valgrind --leak-check=full --show-leak-kinds=all --track-origins=yes --verbose --log-file=valgrind-out.txt build/gnu-debug/MasterServer