From 30d40768085c88ddcc1ba5698935bafbbbb1b0ab Mon Sep 17 00:00:00 2001 From: jadebenn Date: Thu, 21 Nov 2024 02:05:29 -0600 Subject: [PATCH 01/14] fix: Remove instances of undefined behavior --- .gitignore | 1 + CMakeLists.txt | 6 +++- dCommon/Amf3.h | 21 ++++--------- dCommon/AmfSerialize.cpp | 22 +++++++++++--- dGame/dComponents/DestroyableComponent.cpp | 12 ++++---- .../dComponents/PropertyEntranceComponent.cpp | 2 +- .../SlashCommands/GMZeroCommands.cpp | 4 +-- .../Map/General/BankInteractServer.cpp | 2 +- .../02_server/Map/General/MailBoxServer.cpp | 2 +- .../Map/General/StoryBoxInteractServer.cpp | 2 +- dScripts/02_server/Map/NS/NsLegoClubDoor.cpp | 30 +++++++++---------- dScripts/02_server/Map/NS/NsLupTeleport.cpp | 20 ++++++------- .../Map/Property/PropertyBankInteract.cpp | 2 +- dWorldServer/WorldServer.cpp | 2 +- tests/dCommonTests/Amf3Tests.cpp | 8 ++--- 15 files changed, 71 insertions(+), 65 deletions(-) diff --git a/.gitignore b/.gitignore index 0b0d2ecf7..ff1505d6a 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 0d9d394b2..088e0b7a4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,6 +4,10 @@ project(Darkflame LANGUAGES C CXX ) +#add_compile_options("-fsanitize=address,undefined") +add_compile_options("-fsanitize=undefined" "-fvisibility=default") +add_link_options("-fsanitize=undefined" "-static-libsan") + # 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 +21,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 diff --git a/dCommon/Amf3.h b/dCommon/Amf3.h index 21a3c0c74..af1b91427 100644 --- a/dCommon/Amf3.h +++ b/dCommon/Amf3.h @@ -4,6 +4,7 @@ #include "dCommonVars.h" #include "Logger.h" #include "Game.h" +#include "GeneralUtils.h" #include #include @@ -63,6 +64,10 @@ template class AMFValue; template class AMFValue; template class AMFValue; +// Blank specialization to make sure AMFValue fails +template <> +class AMFValue {}; + // AMFValue template class member function instantiations template <> [[nodiscard]] constexpr eAmf AMFValue::GetValueType() const noexcept { return eAmf::Null; } template <> [[nodiscard]] constexpr eAmf AMFValue::GetValueType() const noexcept { return m_Data ? eAmf::True : eAmf::False; } @@ -74,22 +79,6 @@ 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; diff --git a/dCommon/AmfSerialize.cpp b/dCommon/AmfSerialize.cpp index e11ae1de4..2827e1d68 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; } diff --git a/dGame/dComponents/DestroyableComponent.cpp b/dGame/dComponents/DestroyableComponent.cpp index cb8afd5a6..d3ea1989f 100644 --- a/dGame/dComponents/DestroyableComponent.cpp +++ b/dGame/dComponents/DestroyableComponent.cpp @@ -258,8 +258,8 @@ void DestroyableComponent::SetMaxHealth(float value, bool playAnim) { if (!characterComponent) return; AMFArrayValue args; - args.Insert("amount", std::to_string(difference)); - args.Insert("type", "health"); + args.Insert("amount", std::to_string(difference)); + args.Insert("type", "health"); GameMessages::SendUIMessageServerToSingleClient(m_Parent, characterComponent->GetSystemAddress(), "MaxPlayerBarUpdate", args); } @@ -300,8 +300,8 @@ void DestroyableComponent::SetMaxArmor(float value, bool playAnim) { if (!characterComponent) return; AMFArrayValue args; - args.Insert("amount", std::to_string(value)); - args.Insert("type", "armor"); + args.Insert("amount", std::to_string(value)); + args.Insert("type", "armor"); GameMessages::SendUIMessageServerToSingleClient(m_Parent, characterComponent->GetSystemAddress(), "MaxPlayerBarUpdate", args); } @@ -341,8 +341,8 @@ void DestroyableComponent::SetMaxImagination(float value, bool playAnim) { if (!characterComponent) return; AMFArrayValue args; - args.Insert("amount", std::to_string(difference)); - args.Insert("type", "imagination"); + args.Insert("amount", std::to_string(difference)); + args.Insert("type", "imagination"); GameMessages::SendUIMessageServerToSingleClient(m_Parent, characterComponent->GetSystemAddress(), "MaxPlayerBarUpdate", args); } diff --git a/dGame/dComponents/PropertyEntranceComponent.cpp b/dGame/dComponents/PropertyEntranceComponent.cpp index ab3bb5daf..4cc0580bd 100644 --- a/dGame/dComponents/PropertyEntranceComponent.cpp +++ b/dGame/dComponents/PropertyEntranceComponent.cpp @@ -36,7 +36,7 @@ void PropertyEntranceComponent::OnUse(Entity* entity) { AMFArrayValue args; - args.Insert("state", "property_menu"); + args.Insert("state", "property_menu"); GameMessages::SendUIMessageServerToSingleClient(entity, entity->GetSystemAddress(), "pushGameState", args); } diff --git a/dGame/dUtilities/SlashCommands/GMZeroCommands.cpp b/dGame/dUtilities/SlashCommands/GMZeroCommands.cpp index 6c9811c24..855bb7149 100644 --- a/dGame/dUtilities/SlashCommands/GMZeroCommands.cpp +++ b/dGame/dUtilities/SlashCommands/GMZeroCommands.cpp @@ -98,7 +98,7 @@ namespace GMZeroCommands { { AMFArrayValue args; - args.Insert("state", "Story"); + args.Insert("state", "Story"); GameMessages::SendUIMessageServerToSingleClient(entity, entity->GetSystemAddress(), "pushGameState", args); } @@ -121,7 +121,7 @@ namespace GMZeroCommands { { AMFArrayValue args; - args.Insert("state", "Story"); + args.Insert("state", "Story"); GameMessages::SendUIMessageServerToSingleClient(entity, entity->GetSystemAddress(), "pushGameState", args); } diff --git a/dScripts/02_server/Map/General/BankInteractServer.cpp b/dScripts/02_server/Map/General/BankInteractServer.cpp index 9b5634917..cfb8357f2 100644 --- a/dScripts/02_server/Map/General/BankInteractServer.cpp +++ b/dScripts/02_server/Map/General/BankInteractServer.cpp @@ -6,7 +6,7 @@ void BankInteractServer::OnUse(Entity* self, Entity* user) { AMFArrayValue args; - args.Insert("state", "bank"); + args.Insert("state", "bank"); GameMessages::SendUIMessageServerToSingleClient(user, user->GetSystemAddress(), "pushGameState", args); } diff --git a/dScripts/02_server/Map/General/MailBoxServer.cpp b/dScripts/02_server/Map/General/MailBoxServer.cpp index 53f3b3a26..340529a58 100644 --- a/dScripts/02_server/Map/General/MailBoxServer.cpp +++ b/dScripts/02_server/Map/General/MailBoxServer.cpp @@ -6,7 +6,7 @@ void MailBoxServer::OnUse(Entity* self, Entity* user) { AMFArrayValue args; - args.Insert("state", "Mail"); + args.Insert("state", "Mail"); GameMessages::SendUIMessageServerToSingleClient(user, user->GetSystemAddress(), "pushGameState", args); } diff --git a/dScripts/02_server/Map/General/StoryBoxInteractServer.cpp b/dScripts/02_server/Map/General/StoryBoxInteractServer.cpp index 4ad78d6ab..f8528b2f4 100644 --- a/dScripts/02_server/Map/General/StoryBoxInteractServer.cpp +++ b/dScripts/02_server/Map/General/StoryBoxInteractServer.cpp @@ -12,7 +12,7 @@ void StoryBoxInteractServer::OnUse(Entity* self, Entity* user) { { AMFArrayValue args; - args.Insert("state", "Story"); + args.Insert("state", "Story"); GameMessages::SendUIMessageServerToSingleClient(user, user->GetSystemAddress(), "pushGameState", args); } diff --git a/dScripts/02_server/Map/NS/NsLegoClubDoor.cpp b/dScripts/02_server/Map/NS/NsLegoClubDoor.cpp index 959466e93..c945a9e2d 100644 --- a/dScripts/02_server/Map/NS/NsLegoClubDoor.cpp +++ b/dScripts/02_server/Map/NS/NsLegoClubDoor.cpp @@ -13,27 +13,27 @@ void NsLegoClubDoor::OnStartup(Entity* self) { args = {}; args.Insert("callbackClient", std::to_string(self->GetObjectID())); - args.Insert("strIdentifier", "choiceDoor"); - args.Insert("title", "%[UI_CHOICE_DESTINATION]"); + args.Insert("strIdentifier", "choiceDoor"); + args.Insert("title", "%[UI_CHOICE_DESTINATION]"); AMFArrayValue* choiceOptions = args.InsertArray("options"); { AMFArrayValue* nsArgs = choiceOptions->PushArray(); - nsArgs->Insert("image", "textures/ui/zone_thumnails/Nimbus_Station.dds"); - nsArgs->Insert("caption", "%[UI_CHOICE_NS]"); - nsArgs->Insert("identifier", "zoneID_1200"); - nsArgs->Insert("tooltipText", "%[UI_CHOICE_NS_HOVER]"); + nsArgs->Insert("image", "textures/ui/zone_thumnails/Nimbus_Station.dds"); + nsArgs->Insert("caption", "%[UI_CHOICE_NS]"); + nsArgs->Insert("identifier", "zoneID_1200"); + nsArgs->Insert("tooltipText", "%[UI_CHOICE_NS_HOVER]"); } { AMFArrayValue* ntArgs = choiceOptions->PushArray(); - ntArgs->Insert("image", "textures/ui/zone_thumnails/Nexus_Tower.dds"); - ntArgs->Insert("caption", "%[UI_CHOICE_NT]"); - ntArgs->Insert("identifier", "zoneID_1900"); - ntArgs->Insert("tooltipText", "%[UI_CHOICE_NT_HOVER]"); + ntArgs->Insert("image", "textures/ui/zone_thumnails/Nexus_Tower.dds"); + ntArgs->Insert("caption", "%[UI_CHOICE_NT]"); + ntArgs->Insert("identifier", "zoneID_1900"); + ntArgs->Insert("tooltipText", "%[UI_CHOICE_NT_HOVER]"); } options = choiceOptions; @@ -46,8 +46,8 @@ void NsLegoClubDoor::OnUse(Entity* self, Entity* user) { AMFArrayValue multiArgs; multiArgs.Insert("callbackClient", std::to_string(self->GetObjectID())); - multiArgs.Insert("strIdentifier", "choiceDoor"); - multiArgs.Insert("title", "%[UI_CHOICE_DESTINATION]"); + multiArgs.Insert("strIdentifier", "choiceDoor"); + multiArgs.Insert("title", "%[UI_CHOICE_DESTINATION]"); multiArgs.Insert("options", static_cast(options)); GameMessages::SendUIMessageServerToSingleClient(player, player->GetSystemAddress(), "QueueChoiceBox", multiArgs); @@ -55,13 +55,13 @@ void NsLegoClubDoor::OnUse(Entity* self, Entity* user) { multiArgs.Remove("options", false); // We do not want the local amf to delete the options! } else if (self->GetVar(u"currentZone") != m_ChoiceZoneID) { AMFArrayValue multiArgs; - multiArgs.Insert("state", "Lobby"); + multiArgs.Insert("state", "Lobby"); AMFArrayValue* context = multiArgs.InsertArray("context"); context->Insert("user", std::to_string(player->GetObjectID())); context->Insert("callbackObj", std::to_string(self->GetObjectID())); - context->Insert("HelpVisible", "show"); - context->Insert("type", "Lego_Club_Valid"); + context->Insert("HelpVisible", "show"); + context->Insert("type", "Lego_Club_Valid"); GameMessages::SendUIMessageServerToSingleClient(player, player->GetSystemAddress(), "pushGameState", multiArgs); } else { diff --git a/dScripts/02_server/Map/NS/NsLupTeleport.cpp b/dScripts/02_server/Map/NS/NsLupTeleport.cpp index 6886b4cdc..d92f61325 100644 --- a/dScripts/02_server/Map/NS/NsLupTeleport.cpp +++ b/dScripts/02_server/Map/NS/NsLupTeleport.cpp @@ -13,27 +13,27 @@ void NsLupTeleport::OnStartup(Entity* self) { args = {}; args.Insert("callbackClient", std::to_string(self->GetObjectID())); - args.Insert("strIdentifier", "choiceDoor"); - args.Insert("title", "%[UI_CHOICE_DESTINATION]"); + args.Insert("strIdentifier", "choiceDoor"); + args.Insert("title", "%[UI_CHOICE_DESTINATION]"); AMFArrayValue* choiceOptions = args.InsertArray("options"); { AMFArrayValue* nsArgs = choiceOptions->PushArray(); - nsArgs->Insert("image", "textures/ui/zone_thumnails/Nimbus_Station.dds"); - nsArgs->Insert("caption", "%[UI_CHOICE_NS]"); - nsArgs->Insert("identifier", "zoneID_1200"); - nsArgs->Insert("tooltipText", "%[UI_CHOICE_NS_HOVER]"); + nsArgs->Insert("image", "textures/ui/zone_thumnails/Nimbus_Station.dds"); + nsArgs->Insert("caption", "%[UI_CHOICE_NS]"); + nsArgs->Insert("identifier", "zoneID_1200"); + nsArgs->Insert("tooltipText", "%[UI_CHOICE_NS_HOVER]"); } { AMFArrayValue* ntArgs = choiceOptions->PushArray(); - ntArgs->Insert("image", "textures/ui/zone_thumnails/Nexus_Tower.dds"); - ntArgs->Insert("caption", "%[UI_CHOICE_NT]"); - ntArgs->Insert("identifier", "zoneID_1900"); - ntArgs->Insert("tooltipText", "%[UI_CHOICE_NT_HOVER]"); + ntArgs->Insert("image", "textures/ui/zone_thumnails/Nexus_Tower.dds"); + ntArgs->Insert("caption", "%[UI_CHOICE_NT]"); + ntArgs->Insert("identifier", "zoneID_1900"); + ntArgs->Insert("tooltipText", "%[UI_CHOICE_NT_HOVER]"); } } diff --git a/dScripts/02_server/Map/Property/PropertyBankInteract.cpp b/dScripts/02_server/Map/Property/PropertyBankInteract.cpp index 50fea9a05..5e4288d54 100644 --- a/dScripts/02_server/Map/Property/PropertyBankInteract.cpp +++ b/dScripts/02_server/Map/Property/PropertyBankInteract.cpp @@ -22,7 +22,7 @@ void PropertyBankInteract::OnUse(Entity* self, Entity* user) { AMFArrayValue args; - args.Insert("state", "bank"); + args.Insert("state", "bank"); GameMessages::SendUIMessageServerToSingleClient(user, user->GetSystemAddress(), "pushGameState", args); diff --git a/dWorldServer/WorldServer.cpp b/dWorldServer/WorldServer.cpp index c723a01ef..30ba9521a 100644 --- a/dWorldServer/WorldServer.cpp +++ b/dWorldServer/WorldServer.cpp @@ -1396,7 +1396,7 @@ void HandlePacket(Packet* packet) { } default: - const auto messageId = *reinterpret_cast(&packet->data[3]); + const auto messageId = static_cast(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..74614c51f 100644 --- a/tests/dCommonTests/Amf3Tests.cpp +++ b/tests/dCommonTests/Amf3Tests.cpp @@ -61,8 +61,8 @@ TEST(dCommonTests, AMF3AssociativeArrayTest) { TEST(dCommonTests, AMF3InsertionAssociativeTest) { AMFArrayValue array; - array.Insert("CString", "string"); - array.Insert("String", std::string("string")); + array.Insert("CString", "string"); + array.Insert("String", std::string("string")); array.Insert("False", false); array.Insert("True", true); array.Insert("Integer", 42U); @@ -71,7 +71,6 @@ TEST(dCommonTests, AMF3InsertionAssociativeTest) { array.Insert>("Undefined", {}); array.Insert("Null", nullptr); - 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); @@ -85,7 +84,7 @@ TEST(dCommonTests, AMF3InsertionAssociativeTest) { TEST(dCommonTests, AMF3InsertionDenseTest) { AMFArrayValue array; array.Push("string"); - array.Push("CString"); + array.Push("CString"); array.Push(false); array.Push(true); array.Push(42U); @@ -95,7 +94,6 @@ 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(2)->GetValueType(), eAmf::False); ASSERT_EQ(array.Get(3)->GetValueType(), eAmf::True); ASSERT_EQ(array.Get(4)->GetValueType(), eAmf::Integer); From 0a06f309f685ffa71721cf8a71c8eda2c03cd2b5 Mon Sep 17 00:00:00 2001 From: jadebenn Date: Thu, 21 Nov 2024 02:05:40 -0600 Subject: [PATCH 02/14] utils --- CMakeLists.txt | 5 +++-- valgrind.sh | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 valgrind.sh diff --git a/CMakeLists.txt b/CMakeLists.txt index 088e0b7a4..86ee8509e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,9 +4,10 @@ project(Darkflame LANGUAGES C CXX ) -#add_compile_options("-fsanitize=address,undefined") +# TEMP - Sanitizer flags +# add_compile_options("-fsanitize=address,undefined" "-fvisibility=default") add_compile_options("-fsanitize=undefined" "-fvisibility=default") -add_link_options("-fsanitize=undefined" "-static-libsan") +add_link_options("-fsanitize=undefined" "-static-libasan") # check if the path to the source directory contains a space if("${CMAKE_SOURCE_DIR}" MATCHES " ") 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 From 3a0e37ee8acd2b05f1e5c2d1ec5632440f6d9ac7 Mon Sep 17 00:00:00 2001 From: jadebenn Date: Thu, 21 Nov 2024 02:08:21 -0600 Subject: [PATCH 03/14] update configure script --- CMakeLists.txt | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 86ee8509e..427564e72 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -5,9 +5,17 @@ project(Darkflame ) # TEMP - Sanitizer flags +if (UNIX) # add_compile_options("-fsanitize=address,undefined" "-fvisibility=default") -add_compile_options("-fsanitize=undefined" "-fvisibility=default") -add_link_options("-fsanitize=undefined" "-static-libasan") + add_compile_options("-fsanitize=undefined" "-fvisibility=default") + 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 " ") From 5aa8b1395b1685f76d6200c890b74e172eb96239 Mon Sep 17 00:00:00 2001 From: jadebenn Date: Thu, 21 Nov 2024 18:37:50 -0600 Subject: [PATCH 04/14] memcpy and template deduction guide --- dCommon/Amf3.h | 14 ++++----- dGame/dComponents/DestroyableComponent.cpp | 12 ++++---- .../dComponents/PropertyEntranceComponent.cpp | 2 +- .../SlashCommands/GMZeroCommands.cpp | 4 +-- .../Map/General/BankInteractServer.cpp | 2 +- .../02_server/Map/General/MailBoxServer.cpp | 2 +- .../Map/General/StoryBoxInteractServer.cpp | 2 +- dScripts/02_server/Map/NS/NsLegoClubDoor.cpp | 30 +++++++++---------- dScripts/02_server/Map/NS/NsLupTeleport.cpp | 20 ++++++------- .../Map/Property/PropertyBankInteract.cpp | 2 +- dWorldServer/WorldServer.cpp | 5 +++- tests/dCommonTests/Amf3Tests.cpp | 8 +++-- 12 files changed, 54 insertions(+), 49 deletions(-) diff --git a/dCommon/Amf3.h b/dCommon/Amf3.h index af1b91427..fc3b5d61c 100644 --- a/dCommon/Amf3.h +++ b/dCommon/Amf3.h @@ -1,5 +1,5 @@ -#ifndef __AMF3__H__ -#define __AMF3__H__ +#ifndef AMF3_H +#define AMF3_H #include "dCommonVars.h" #include "Logger.h" @@ -64,10 +64,6 @@ template class AMFValue; template class AMFValue; template class AMFValue; -// Blank specialization to make sure AMFValue fails -template <> -class AMFValue {}; - // AMFValue template class member function instantiations template <> [[nodiscard]] constexpr eAmf AMFValue::GetValueType() const noexcept { return eAmf::Null; } template <> [[nodiscard]] constexpr eAmf AMFValue::GetValueType() const noexcept { return m_Data ? eAmf::True : eAmf::False; } @@ -85,6 +81,10 @@ using AMFIntValue = AMFValue; using AMFStringValue = AMFValue; using AMFDoubleValue = AMFValue; +// Template deduction guide to ensure string literals deduce +template +AMFValue(const char (&)[N]) -> AMFStringValue; + /** * The AMFArrayValue object holds 2 types of lists: * An associative list where a key maps to a value @@ -345,4 +345,4 @@ class AMFArrayValue : public AMFBaseValue { AMFDense m_Dense; }; -#endif //!__AMF3__H__ +#endif //!AMF3_H diff --git a/dGame/dComponents/DestroyableComponent.cpp b/dGame/dComponents/DestroyableComponent.cpp index d3ea1989f..cb8afd5a6 100644 --- a/dGame/dComponents/DestroyableComponent.cpp +++ b/dGame/dComponents/DestroyableComponent.cpp @@ -258,8 +258,8 @@ void DestroyableComponent::SetMaxHealth(float value, bool playAnim) { if (!characterComponent) return; AMFArrayValue args; - args.Insert("amount", std::to_string(difference)); - args.Insert("type", "health"); + args.Insert("amount", std::to_string(difference)); + args.Insert("type", "health"); GameMessages::SendUIMessageServerToSingleClient(m_Parent, characterComponent->GetSystemAddress(), "MaxPlayerBarUpdate", args); } @@ -300,8 +300,8 @@ void DestroyableComponent::SetMaxArmor(float value, bool playAnim) { if (!characterComponent) return; AMFArrayValue args; - args.Insert("amount", std::to_string(value)); - args.Insert("type", "armor"); + args.Insert("amount", std::to_string(value)); + args.Insert("type", "armor"); GameMessages::SendUIMessageServerToSingleClient(m_Parent, characterComponent->GetSystemAddress(), "MaxPlayerBarUpdate", args); } @@ -341,8 +341,8 @@ void DestroyableComponent::SetMaxImagination(float value, bool playAnim) { if (!characterComponent) return; AMFArrayValue args; - args.Insert("amount", std::to_string(difference)); - args.Insert("type", "imagination"); + args.Insert("amount", std::to_string(difference)); + args.Insert("type", "imagination"); GameMessages::SendUIMessageServerToSingleClient(m_Parent, characterComponent->GetSystemAddress(), "MaxPlayerBarUpdate", args); } diff --git a/dGame/dComponents/PropertyEntranceComponent.cpp b/dGame/dComponents/PropertyEntranceComponent.cpp index 4cc0580bd..ab3bb5daf 100644 --- a/dGame/dComponents/PropertyEntranceComponent.cpp +++ b/dGame/dComponents/PropertyEntranceComponent.cpp @@ -36,7 +36,7 @@ void PropertyEntranceComponent::OnUse(Entity* entity) { AMFArrayValue args; - args.Insert("state", "property_menu"); + args.Insert("state", "property_menu"); GameMessages::SendUIMessageServerToSingleClient(entity, entity->GetSystemAddress(), "pushGameState", args); } diff --git a/dGame/dUtilities/SlashCommands/GMZeroCommands.cpp b/dGame/dUtilities/SlashCommands/GMZeroCommands.cpp index 855bb7149..6c9811c24 100644 --- a/dGame/dUtilities/SlashCommands/GMZeroCommands.cpp +++ b/dGame/dUtilities/SlashCommands/GMZeroCommands.cpp @@ -98,7 +98,7 @@ namespace GMZeroCommands { { AMFArrayValue args; - args.Insert("state", "Story"); + args.Insert("state", "Story"); GameMessages::SendUIMessageServerToSingleClient(entity, entity->GetSystemAddress(), "pushGameState", args); } @@ -121,7 +121,7 @@ namespace GMZeroCommands { { AMFArrayValue args; - args.Insert("state", "Story"); + args.Insert("state", "Story"); GameMessages::SendUIMessageServerToSingleClient(entity, entity->GetSystemAddress(), "pushGameState", args); } diff --git a/dScripts/02_server/Map/General/BankInteractServer.cpp b/dScripts/02_server/Map/General/BankInteractServer.cpp index cfb8357f2..9b5634917 100644 --- a/dScripts/02_server/Map/General/BankInteractServer.cpp +++ b/dScripts/02_server/Map/General/BankInteractServer.cpp @@ -6,7 +6,7 @@ void BankInteractServer::OnUse(Entity* self, Entity* user) { AMFArrayValue args; - args.Insert("state", "bank"); + args.Insert("state", "bank"); GameMessages::SendUIMessageServerToSingleClient(user, user->GetSystemAddress(), "pushGameState", args); } diff --git a/dScripts/02_server/Map/General/MailBoxServer.cpp b/dScripts/02_server/Map/General/MailBoxServer.cpp index 340529a58..53f3b3a26 100644 --- a/dScripts/02_server/Map/General/MailBoxServer.cpp +++ b/dScripts/02_server/Map/General/MailBoxServer.cpp @@ -6,7 +6,7 @@ void MailBoxServer::OnUse(Entity* self, Entity* user) { AMFArrayValue args; - args.Insert("state", "Mail"); + args.Insert("state", "Mail"); GameMessages::SendUIMessageServerToSingleClient(user, user->GetSystemAddress(), "pushGameState", args); } diff --git a/dScripts/02_server/Map/General/StoryBoxInteractServer.cpp b/dScripts/02_server/Map/General/StoryBoxInteractServer.cpp index f8528b2f4..4ad78d6ab 100644 --- a/dScripts/02_server/Map/General/StoryBoxInteractServer.cpp +++ b/dScripts/02_server/Map/General/StoryBoxInteractServer.cpp @@ -12,7 +12,7 @@ void StoryBoxInteractServer::OnUse(Entity* self, Entity* user) { { AMFArrayValue args; - args.Insert("state", "Story"); + args.Insert("state", "Story"); GameMessages::SendUIMessageServerToSingleClient(user, user->GetSystemAddress(), "pushGameState", args); } diff --git a/dScripts/02_server/Map/NS/NsLegoClubDoor.cpp b/dScripts/02_server/Map/NS/NsLegoClubDoor.cpp index c945a9e2d..959466e93 100644 --- a/dScripts/02_server/Map/NS/NsLegoClubDoor.cpp +++ b/dScripts/02_server/Map/NS/NsLegoClubDoor.cpp @@ -13,27 +13,27 @@ void NsLegoClubDoor::OnStartup(Entity* self) { args = {}; args.Insert("callbackClient", std::to_string(self->GetObjectID())); - args.Insert("strIdentifier", "choiceDoor"); - args.Insert("title", "%[UI_CHOICE_DESTINATION]"); + args.Insert("strIdentifier", "choiceDoor"); + args.Insert("title", "%[UI_CHOICE_DESTINATION]"); AMFArrayValue* choiceOptions = args.InsertArray("options"); { AMFArrayValue* nsArgs = choiceOptions->PushArray(); - nsArgs->Insert("image", "textures/ui/zone_thumnails/Nimbus_Station.dds"); - nsArgs->Insert("caption", "%[UI_CHOICE_NS]"); - nsArgs->Insert("identifier", "zoneID_1200"); - nsArgs->Insert("tooltipText", "%[UI_CHOICE_NS_HOVER]"); + nsArgs->Insert("image", "textures/ui/zone_thumnails/Nimbus_Station.dds"); + nsArgs->Insert("caption", "%[UI_CHOICE_NS]"); + nsArgs->Insert("identifier", "zoneID_1200"); + nsArgs->Insert("tooltipText", "%[UI_CHOICE_NS_HOVER]"); } { AMFArrayValue* ntArgs = choiceOptions->PushArray(); - ntArgs->Insert("image", "textures/ui/zone_thumnails/Nexus_Tower.dds"); - ntArgs->Insert("caption", "%[UI_CHOICE_NT]"); - ntArgs->Insert("identifier", "zoneID_1900"); - ntArgs->Insert("tooltipText", "%[UI_CHOICE_NT_HOVER]"); + ntArgs->Insert("image", "textures/ui/zone_thumnails/Nexus_Tower.dds"); + ntArgs->Insert("caption", "%[UI_CHOICE_NT]"); + ntArgs->Insert("identifier", "zoneID_1900"); + ntArgs->Insert("tooltipText", "%[UI_CHOICE_NT_HOVER]"); } options = choiceOptions; @@ -46,8 +46,8 @@ void NsLegoClubDoor::OnUse(Entity* self, Entity* user) { AMFArrayValue multiArgs; multiArgs.Insert("callbackClient", std::to_string(self->GetObjectID())); - multiArgs.Insert("strIdentifier", "choiceDoor"); - multiArgs.Insert("title", "%[UI_CHOICE_DESTINATION]"); + multiArgs.Insert("strIdentifier", "choiceDoor"); + multiArgs.Insert("title", "%[UI_CHOICE_DESTINATION]"); multiArgs.Insert("options", static_cast(options)); GameMessages::SendUIMessageServerToSingleClient(player, player->GetSystemAddress(), "QueueChoiceBox", multiArgs); @@ -55,13 +55,13 @@ void NsLegoClubDoor::OnUse(Entity* self, Entity* user) { multiArgs.Remove("options", false); // We do not want the local amf to delete the options! } else if (self->GetVar(u"currentZone") != m_ChoiceZoneID) { AMFArrayValue multiArgs; - multiArgs.Insert("state", "Lobby"); + multiArgs.Insert("state", "Lobby"); AMFArrayValue* context = multiArgs.InsertArray("context"); context->Insert("user", std::to_string(player->GetObjectID())); context->Insert("callbackObj", std::to_string(self->GetObjectID())); - context->Insert("HelpVisible", "show"); - context->Insert("type", "Lego_Club_Valid"); + context->Insert("HelpVisible", "show"); + context->Insert("type", "Lego_Club_Valid"); GameMessages::SendUIMessageServerToSingleClient(player, player->GetSystemAddress(), "pushGameState", multiArgs); } else { diff --git a/dScripts/02_server/Map/NS/NsLupTeleport.cpp b/dScripts/02_server/Map/NS/NsLupTeleport.cpp index d92f61325..6886b4cdc 100644 --- a/dScripts/02_server/Map/NS/NsLupTeleport.cpp +++ b/dScripts/02_server/Map/NS/NsLupTeleport.cpp @@ -13,27 +13,27 @@ void NsLupTeleport::OnStartup(Entity* self) { args = {}; args.Insert("callbackClient", std::to_string(self->GetObjectID())); - args.Insert("strIdentifier", "choiceDoor"); - args.Insert("title", "%[UI_CHOICE_DESTINATION]"); + args.Insert("strIdentifier", "choiceDoor"); + args.Insert("title", "%[UI_CHOICE_DESTINATION]"); AMFArrayValue* choiceOptions = args.InsertArray("options"); { AMFArrayValue* nsArgs = choiceOptions->PushArray(); - nsArgs->Insert("image", "textures/ui/zone_thumnails/Nimbus_Station.dds"); - nsArgs->Insert("caption", "%[UI_CHOICE_NS]"); - nsArgs->Insert("identifier", "zoneID_1200"); - nsArgs->Insert("tooltipText", "%[UI_CHOICE_NS_HOVER]"); + nsArgs->Insert("image", "textures/ui/zone_thumnails/Nimbus_Station.dds"); + nsArgs->Insert("caption", "%[UI_CHOICE_NS]"); + nsArgs->Insert("identifier", "zoneID_1200"); + nsArgs->Insert("tooltipText", "%[UI_CHOICE_NS_HOVER]"); } { AMFArrayValue* ntArgs = choiceOptions->PushArray(); - ntArgs->Insert("image", "textures/ui/zone_thumnails/Nexus_Tower.dds"); - ntArgs->Insert("caption", "%[UI_CHOICE_NT]"); - ntArgs->Insert("identifier", "zoneID_1900"); - ntArgs->Insert("tooltipText", "%[UI_CHOICE_NT_HOVER]"); + ntArgs->Insert("image", "textures/ui/zone_thumnails/Nexus_Tower.dds"); + ntArgs->Insert("caption", "%[UI_CHOICE_NT]"); + ntArgs->Insert("identifier", "zoneID_1900"); + ntArgs->Insert("tooltipText", "%[UI_CHOICE_NT_HOVER]"); } } diff --git a/dScripts/02_server/Map/Property/PropertyBankInteract.cpp b/dScripts/02_server/Map/Property/PropertyBankInteract.cpp index 5e4288d54..50fea9a05 100644 --- a/dScripts/02_server/Map/Property/PropertyBankInteract.cpp +++ b/dScripts/02_server/Map/Property/PropertyBankInteract.cpp @@ -22,7 +22,7 @@ void PropertyBankInteract::OnUse(Entity* self, Entity* user) { AMFArrayValue args; - args.Insert("state", "bank"); + args.Insert("state", "bank"); GameMessages::SendUIMessageServerToSingleClient(user, user->GetSystemAddress(), "pushGameState", args); diff --git a/dWorldServer/WorldServer.cpp b/dWorldServer/WorldServer.cpp index 30ba9521a..c6c894b4c 100644 --- a/dWorldServer/WorldServer.cpp +++ b/dWorldServer/WorldServer.cpp @@ -1396,7 +1396,10 @@ void HandlePacket(Packet* packet) { } default: - const auto messageId = static_cast(packet->data[3]); + // Need to use memcpy instead of reinterpret_cast to avoid UB + MessageType::World messageId; + std::memcpy(&messageId, &packet->data[3], sizeof(MessageType::World)); + 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 74614c51f..79d0d496d 100644 --- a/tests/dCommonTests/Amf3Tests.cpp +++ b/tests/dCommonTests/Amf3Tests.cpp @@ -61,8 +61,8 @@ TEST(dCommonTests, AMF3AssociativeArrayTest) { TEST(dCommonTests, AMF3InsertionAssociativeTest) { AMFArrayValue array; - array.Insert("CString", "string"); - array.Insert("String", std::string("string")); + array.Insert("CString", "string"); + array.Insert("String", std::string("string")); array.Insert("False", false); array.Insert("True", true); array.Insert("Integer", 42U); @@ -71,6 +71,7 @@ TEST(dCommonTests, AMF3InsertionAssociativeTest) { array.Insert>("Undefined", {}); array.Insert("Null", nullptr); + 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); @@ -84,7 +85,7 @@ TEST(dCommonTests, AMF3InsertionAssociativeTest) { TEST(dCommonTests, AMF3InsertionDenseTest) { AMFArrayValue array; array.Push("string"); - array.Push("CString"); + array.Push("CString"); array.Push(false); array.Push(true); array.Push(42U); @@ -94,6 +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(2)->GetValueType(), eAmf::False); ASSERT_EQ(array.Get(3)->GetValueType(), eAmf::True); ASSERT_EQ(array.Get(4)->GetValueType(), eAmf::Integer); From 051a0ba05ec49444b4a7345da5e22ca4adaa76ab Mon Sep 17 00:00:00 2001 From: jadebenn Date: Thu, 21 Nov 2024 18:48:02 -0600 Subject: [PATCH 05/14] default-initialize message-id buffer to an invalid (but defined) state and trust the compiler to optimize it out --- dCommon/dEnums/MessageType/World.h | 3 ++- dWorldServer/WorldServer.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) 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/dWorldServer/WorldServer.cpp b/dWorldServer/WorldServer.cpp index c6c894b4c..442655068 100644 --- a/dWorldServer/WorldServer.cpp +++ b/dWorldServer/WorldServer.cpp @@ -1397,7 +1397,7 @@ void HandlePacket(Packet* packet) { default: // Need to use memcpy instead of reinterpret_cast to avoid UB - MessageType::World messageId; + auto messageId = MessageType::World::INVALID; std::memcpy(&messageId, &packet->data[3], sizeof(MessageType::World)); const std::string_view messageIdString = StringifiedEnum::ToString(messageId); From 12387ba07d46af553e7f6113b392cab9e82a3315 Mon Sep 17 00:00:00 2001 From: jadebenn Date: Thu, 21 Nov 2024 18:52:12 -0600 Subject: [PATCH 06/14] Update Amf3.h for macos --- dCommon/Amf3.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dCommon/Amf3.h b/dCommon/Amf3.h index fc3b5d61c..b4e25058d 100644 --- a/dCommon/Amf3.h +++ b/dCommon/Amf3.h @@ -83,7 +83,7 @@ using AMFDoubleValue = AMFValue; // Template deduction guide to ensure string literals deduce template -AMFValue(const char (&)[N]) -> AMFStringValue; +AMFValue(const char (&)[N]) -> AMFValue; // AMFStringValue /** * The AMFArrayValue object holds 2 types of lists: From b799f8967c19504e146d89f76114a3be4fe7d607 Mon Sep 17 00:00:00 2001 From: jadebenn Date: Thu, 21 Nov 2024 21:06:22 -0600 Subject: [PATCH 07/14] Remove const char* templated test cases from Amf tests (no longer used) --- tests/dCommonTests/Amf3Tests.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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); From ac4fd02a6ccdaac584927991ec122fdad46686c1 Mon Sep 17 00:00:00 2001 From: jadebenn Date: Fri, 22 Nov 2024 16:33:04 -0800 Subject: [PATCH 08/14] use decltype --- dCommon/Amf3.h | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/dCommon/Amf3.h b/dCommon/Amf3.h index b4e25058d..ad41600f2 100644 --- a/dCommon/Amf3.h +++ b/dCommon/Amf3.h @@ -82,8 +82,7 @@ using AMFStringValue = AMFValue; using AMFDoubleValue = AMFValue; // Template deduction guide to ensure string literals deduce -template -AMFValue(const char (&)[N]) -> AMFValue; // AMFStringValue +AMFValue(const char*) -> AMFValue; // AMFStringValue /** * The AMFArrayValue object holds 2 types of lists: @@ -126,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); @@ -179,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); } /** @@ -234,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; } From 8eb34888120134d22a34562bdbb4dd7b72b6419f Mon Sep 17 00:00:00 2001 From: jadebenn Date: Fri, 22 Nov 2024 18:14:00 -0800 Subject: [PATCH 09/14] update AMF3 logic and disable sanitizers on darwin --- CMakeLists.txt | 4 ++-- dCommon/Amf3.h | 2 +- dCommon/GeneralUtils.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 427564e72..a2affdc03 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -5,8 +5,8 @@ project(Darkflame ) # TEMP - Sanitizer flags -if (UNIX) -# add_compile_options("-fsanitize=address,undefined" "-fvisibility=default") +if (UNIX AND NOT APPLE) + # add_compile_options("-fsanitize=address,undefined" "-fvisibility=default") add_compile_options("-fsanitize=undefined" "-fvisibility=default") add_link_options("-fsanitize=undefined") diff --git a/dCommon/Amf3.h b/dCommon/Amf3.h index ad41600f2..4c9f49d09 100644 --- a/dCommon/Amf3.h +++ b/dCommon/Amf3.h @@ -94,7 +94,7 @@ AMFValue(const char*) -> AMFValue; // AMFStringValue */ 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>; diff --git a/dCommon/GeneralUtils.h b/dCommon/GeneralUtils.h index 2c93b656d..3c4ee285f 100644 --- a/dCommon/GeneralUtils.h +++ b/dCommon/GeneralUtils.h @@ -137,7 +137,7 @@ namespace GeneralUtils { template struct overload : Bases... { using is_transparent = void; - using Bases::operator() ... ; + using Bases::operator()...; }; struct char_pointer_hash { From 7740bbbaab58bb7a540fc06c5145fd9cad3221d6 Mon Sep 17 00:00:00 2001 From: jadebenn Date: Sat, 23 Nov 2024 02:29:11 -0800 Subject: [PATCH 10/14] reinterpret_cast-based type-punning is almost always UB --- CMakeLists.txt | 2 +- dCommon/AmfSerialize.cpp | 3 +-- dCommon/BrickByBrickFix.cpp | 2 +- dCommon/GeneralUtils.cpp | 2 +- dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp | 16 ++++++++++------ 5 files changed, 14 insertions(+), 11 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a2affdc03..60c4e3f8e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -285,7 +285,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") else() message(WARNING "Unknown compiler: '${CMAKE_CXX_COMPILER_ID}' - No warning flags enabled.") endif() diff --git a/dCommon/AmfSerialize.cpp b/dCommon/AmfSerialize.cpp index 2827e1d68..db417e493 100644 --- a/dCommon/AmfSerialize.cpp +++ b/dCommon/AmfSerialize.cpp @@ -159,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 b771c9861..85f4a5587 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 522879048..e254a23a4 100644 --- a/dCommon/GeneralUtils.cpp +++ b/dCommon/GeneralUtils.cpp @@ -53,7 +53,7 @@ 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 auto* bytes = &slice.front(); if (rem > 0) { const uint8_t first = bytes[0]; if (first < 0x80) { // 1 byte character diff --git a/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp b/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp index 515603a0f..5bd0ff2bd 100644 --- a/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp +++ b/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp @@ -50,9 +50,9 @@ void NpcAgCourseStarter::OnMessageBoxResponse(Entity* self, Entity* sender, int3 if (data->values[1] != 0) return; - time_t startTime = std::time(0) + 4; // Offset for starting timer + const time_t startTime = std::time(0) + 4; // Offset for starting timer - data->values[1] = *reinterpret_cast(&startTime); + std::memcpy(&data->values[1], &startTime, sizeof(float)); Game::entityManager->SerializeEntity(self); } else if (identifier == u"FootRaceCancel") { @@ -80,10 +80,14 @@ void NpcAgCourseStarter::OnFireEventServerSide(Entity* self, Entity* sender, std LWOOBJID_EMPTY, "", sender->GetSystemAddress()); scriptedActivityComponent->RemoveActivityPlayerData(sender->GetObjectID()); } else if (args == "course_finish") { - time_t endTime = std::time(0); - time_t finish = (endTime - *reinterpret_cast(&data->values[1])); - - data->values[2] = *reinterpret_cast(&finish); + const time_t endTime = std::time(0); + + // Using memcpy since misaligned reads are UB + time_t startTime{}; + std::memcpy(&startTime, &data->values[1], sizeof(time_t)); + const time_t finish = (endTime - startTime); + + std::memcpy(&data->values[2], &finish, sizeof(float)); auto* missionComponent = sender->GetComponent(); if (missionComponent != nullptr) { From 6fa719c679dee2be86e97d3891e4fc9e350abcd4 Mon Sep 17 00:00:00 2001 From: jadebenn Date: Sat, 23 Nov 2024 13:56:47 -0800 Subject: [PATCH 11/14] minor updates --- CMakeLists.txt | 9 ++++----- cmake/toolchains/linux-clang.cmake | 1 + dCommon/GeneralUtils.cpp | 16 ++++++++-------- dWorldServer/WorldServer.cpp | 2 +- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 60c4e3f8e..ad5bd01bc 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,10 +4,9 @@ project(Darkflame LANGUAGES C CXX ) -# TEMP - Sanitizer flags -if (UNIX AND NOT APPLE) - # add_compile_options("-fsanitize=address,undefined" "-fvisibility=default") - add_compile_options("-fsanitize=undefined" "-fvisibility=default") +# Sanitizer flags - TODO: Make CMake preset before finalizing PR +if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU") + add_compile_options("-fsanitize=undefined") add_link_options("-fsanitize=undefined") if (CMAKE_CXX_COMPILER_ID MATCHES "Clang") @@ -285,7 +284,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" "-Wstrict-aliasing") + 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/GeneralUtils.cpp b/dCommon/GeneralUtils.cpp index e254a23a4..cc4c4b1ea 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 auto* bytes = &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/dWorldServer/WorldServer.cpp b/dWorldServer/WorldServer.cpp index 442655068..84a0c1a93 100644 --- a/dWorldServer/WorldServer.cpp +++ b/dWorldServer/WorldServer.cpp @@ -1396,7 +1396,7 @@ void HandlePacket(Packet* packet) { } default: - // Need to use memcpy instead of reinterpret_cast to avoid UB + // Need to use memcpy instead of reinterpret_cast to avoid misaligned reads, which are UB auto messageId = MessageType::World::INVALID; std::memcpy(&messageId, &packet->data[3], sizeof(MessageType::World)); From 66fa3ff4bab5e36066784e61632e07336de7b8d5 Mon Sep 17 00:00:00 2001 From: jadebenn Date: Mon, 25 Nov 2024 01:40:58 -0600 Subject: [PATCH 12/14] created FromBitsUnchecked memcpy wrapper --- dCommon/GeneralUtils.h | 21 +++++++++++++++++++ dCommon/dClient/AssetManager.h | 1 + .../02_server/Map/AG/NpcAgCourseStarter.cpp | 9 ++++---- dWorldServer/WorldServer.cpp | 5 ++--- 4 files changed, 28 insertions(+), 8 deletions(-) diff --git a/dCommon/GeneralUtils.h b/dCommon/GeneralUtils.h index 3c4ee285f..b1d497281 100644 --- a/dCommon/GeneralUtils.h +++ b/dCommon/GeneralUtils.h @@ -3,6 +3,7 @@ // C++ #include #include +#include #include #include #include @@ -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/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp b/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp index 5bd0ff2bd..c6c3629d9 100644 --- a/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp +++ b/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp @@ -81,11 +81,10 @@ void NpcAgCourseStarter::OnFireEventServerSide(Entity* self, Entity* sender, std scriptedActivityComponent->RemoveActivityPlayerData(sender->GetObjectID()); } else if (args == "course_finish") { const time_t endTime = std::time(0); - - // Using memcpy since misaligned reads are UB - time_t startTime{}; - std::memcpy(&startTime, &data->values[1], sizeof(time_t)); - const time_t finish = (endTime - startTime); + + // Using FromBitsUnchecked to create new time_t object since misaligned reads are UB + const time_t startTime = GeneralUtils::FromBitsUnchecked(&data->values[1]); + const time_t finish = endTime - startTime; std::memcpy(&data->values[2], &finish, sizeof(float)); diff --git a/dWorldServer/WorldServer.cpp b/dWorldServer/WorldServer.cpp index 84a0c1a93..c71c4a4fe 100644 --- a/dWorldServer/WorldServer.cpp +++ b/dWorldServer/WorldServer.cpp @@ -1396,9 +1396,8 @@ void HandlePacket(Packet* packet) { } default: - // Need to use memcpy instead of reinterpret_cast to avoid misaligned reads, which are UB - auto messageId = MessageType::World::INVALID; - std::memcpy(&messageId, &packet->data[3], sizeof(MessageType::World)); + // 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()); From a568e66e4186bbef035cc44ae45e782c6843eabc Mon Sep 17 00:00:00 2001 From: jadebenn Date: Wed, 27 Nov 2024 00:34:37 -0600 Subject: [PATCH 13/14] Stop annoying mac build failure --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index ad5bd01bc..f3cb37181 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -5,7 +5,7 @@ project(Darkflame ) # Sanitizer flags - TODO: Make CMake preset before finalizing PR -if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU") +if (CMAKE_CXX_COMPILER_ID MATCHES "Clang|GNU" AND NOT APPLE) add_compile_options("-fsanitize=undefined") add_link_options("-fsanitize=undefined") From 5599bd946bd93512579621ce68523f39039acb32 Mon Sep 17 00:00:00 2001 From: jadebenn Date: Wed, 27 Nov 2024 01:38:00 -0600 Subject: [PATCH 14/14] reduce memcpying --- dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp b/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp index c6c3629d9..e366ca960 100644 --- a/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp +++ b/dScripts/02_server/Map/AG/NpcAgCourseStarter.cpp @@ -50,9 +50,9 @@ void NpcAgCourseStarter::OnMessageBoxResponse(Entity* self, Entity* sender, int3 if (data->values[1] != 0) return; + // Convert to 32 bit time (Note: could try and fix the 2038 problem here by using a different epoch maybe?) const time_t startTime = std::time(0) + 4; // Offset for starting timer - - std::memcpy(&data->values[1], &startTime, sizeof(float)); + data->values[1] = std::bit_cast(static_cast(startTime)); Game::entityManager->SerializeEntity(self); } else if (identifier == u"FootRaceCancel") { @@ -81,12 +81,9 @@ void NpcAgCourseStarter::OnFireEventServerSide(Entity* self, Entity* sender, std scriptedActivityComponent->RemoveActivityPlayerData(sender->GetObjectID()); } else if (args == "course_finish") { const time_t endTime = std::time(0); - - // Using FromBitsUnchecked to create new time_t object since misaligned reads are UB - const time_t startTime = GeneralUtils::FromBitsUnchecked(&data->values[1]); + const time_t startTime = std::bit_cast(data->values[1]); const time_t finish = endTime - startTime; - - std::memcpy(&data->values[2], &finish, sizeof(float)); + data->values[2] = std::bit_cast(static_cast(finish)); auto* missionComponent = sender->GetComponent(); if (missionComponent != nullptr) {