From 30d40768085c88ddcc1ba5698935bafbbbb1b0ab Mon Sep 17 00:00:00 2001 From: jadebenn Date: Thu, 21 Nov 2024 02:05:29 -0600 Subject: [PATCH] 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 0b0d2ecf..ff1505d6 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 0d9d394b..088e0b7a 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 21a3c0c7..af1b9142 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 e11ae1de..2827e1d6 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 cb8afd5a..d3ea1989 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 ab3bb5da..4cc0580b 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 6c9811c2..855bb714 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 9b563491..cfb8357f 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 53f3b3a2..340529a5 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 4ad78d6a..f8528b2f 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 959466e9..c945a9e2 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 6886b4cd..d92f6132 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 50fea9a0..5e4288d5 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 c723a01e..30ba9521 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 79d0d496..74614c51 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);