fix: Remove instances of undefined behavior

.gitignore

@@ -2,6 +2,7 @@ temp/
 cmake-build-debug/
 RelWithDebInfo/
 docker/configs
+valgrind-out.txt
 
 # Third party libraries
 thirdparty/mysql/

CMakeLists.txt

@@ -4,6 +4,19 @@ 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")
+	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 +30,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
@@ -272,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()

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 <unordered_map>
 #include <vector>
@@ -74,28 +75,15 @@ template <> [[nodiscard]] constexpr eAmf AMFValue<double>::GetValueType() const
 template <typename ValueType>
 [[nodiscard]] constexpr eAmf AMFValue<ValueType>::GetValueType() const noexcept { return eAmf::Undefined; }
 
-// As a string this is much easier to write and read from a BitStream.
-template <>
-class AMFValue<const char*> : 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<std::nullptr_t>;
 using AMFBoolValue = AMFValue<bool>;
 using AMFIntValue = AMFValue<int32_t>;
 using AMFStringValue = AMFValue<std::string>;
 using AMFDoubleValue = AMFValue<double>;
 
+// Template deduction guide to ensure string literals deduce
+AMFValue(const char*) -> AMFValue<std::string>; // 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<double>;
  */
 class AMFArrayValue : public AMFBaseValue {
 	using AMFAssociative =
-		std::unordered_map<std::string, std::unique_ptr<AMFBaseValue>, GeneralUtils::transparent_string_hash, std::equal_to<>>;
+		std::unordered_map<std::string, std::unique_ptr<AMFBaseValue>, GeneralUtils::transparent_string_hash, std::equal_to<void>>;
 
 	using AMFDense = std::vector<std::unique_ptr<AMFBaseValue>>;
 
@@ -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 <typename ValueType>
-	[[maybe_unused]] std::pair<AMFValue<ValueType>*, bool> Insert(const std::string_view key, const ValueType value) {
+	template <typename T>
+	[[maybe_unused]] auto Insert(const std::string_view key, const T value) -> std::pair<decltype(AMFValue(value))*, bool> {
+		// This ensures the deduced type matches the AMFValue constructor
+		using AMFValueType = decltype(AMFValue(value));
+
 		const auto element = m_Associative.find(key);
-		AMFValue<ValueType>* val = nullptr;
+		AMFValueType* val = nullptr;
 		bool found = true;
 		if (element == m_Associative.cend()) {
-			auto newVal = std::make_unique<AMFValue<ValueType>>(value);
+			auto newVal = std::make_unique<AMFValueType>(value);
 			val = newVal.get();
 			m_Associative.emplace(key, std::move(newVal));
 		} else {
-			val = dynamic_cast<AMFValue<ValueType>*>(element->second.get());
+			val = dynamic_cast<AMFValueType*>(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 <typename ValueType>
-	[[maybe_unused]] std::pair<AMFValue<ValueType>*, bool> Insert(const size_t index, const ValueType value) {
+	template <typename T>
+	[[maybe_unused]] auto Insert(const size_t index, const T value) -> std::pair<decltype(AMFValue(value))*, bool> {
+		// 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<AMFValue<ValueType>>(value);
+			m_Dense.at(index) = std::make_unique<AMFValueType>(value);
 			inserted = true;
 		}
-		return std::make_pair(dynamic_cast<AMFValue<ValueType>*>(m_Dense.at(index).get()), inserted);
+		return std::make_pair(dynamic_cast<AMFValueType*>(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 <typename ValueType>
-	[[maybe_unused]] inline AMFValue<ValueType>* Push(const ValueType value) {
+	template <typename T>
+	[[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

dCommon/AmfSerialize.cpp

@@ -10,40 +10,54 @@ void RakNet::BitStream::Write<AMFBaseValue&>(AMFBaseValue& value) {
 	this->Write(type);
 	switch (type) {
 	case eAmf::Integer: {
-		this->Write<AMFIntValue&>(*static_cast<AMFIntValue*>(&value));
+		this->Write<AMFIntValue&>(static_cast<AMFIntValue&>(value));
 		break;
 	}
 
 	case eAmf::Double: {
-		this->Write<AMFDoubleValue&>(*static_cast<AMFDoubleValue*>(&value));
+		this->Write<AMFDoubleValue&>(static_cast<AMFDoubleValue&>(value));
 		break;
 	}
 
 	case eAmf::String: {
-		this->Write<AMFStringValue&>(*static_cast<AMFStringValue*>(&value));
+		this->Write<AMFStringValue&>(static_cast<AMFStringValue&>(value));
 		break;
 	}
 
 	case eAmf::Array: {
-		this->Write<AMFArrayValue&>(*static_cast<AMFArrayValue*>(&value));
+		this->Write<AMFArrayValue&>(static_cast<AMFArrayValue&>(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&>(AMFIntValue& value) {
 // Writes an AMFDoubleValue to BitStream
 template<>
 void RakNet::BitStream::Write<AMFDoubleValue&>(AMFDoubleValue& value) {
-	double d = value.GetValue();
-	WriteAMFU64(*this, *reinterpret_cast<uint64_t*>(&d));
+	WriteAMFU64(*this, std::bit_cast<uint64_t>(value.GetValue()));
 }
 
 // Writes an AMFStringValue to BitStream

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<uint32_t*>(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) {

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<const uint8_t*>(&slice.front());
+	const auto* bytes = &slice.front();
 	if (rem > 0) {
 		const uint8_t first = bytes[0];
 		if (first < 0x80) { // 1 byte character

dCommon/GeneralUtils.h

@@ -137,7 +137,7 @@ namespace GeneralUtils {
 	template <typename... Bases>
 	struct overload : Bases... {
 		using is_transparent = void;
-		using Bases::operator() ... ;
+		using Bases::operator()...;
 	};
 
 	struct char_pointer_hash {

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

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<float*>(&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<time_t*>(&data->values[1]));
-
-		data->values[2] = *reinterpret_cast<float*>(&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<MissionComponent>();
 		if (missionComponent != nullptr) {

dWorldServer/WorldServer.cpp

@@ -1396,7 +1396,10 @@ void HandlePacket(Packet* packet) {
 	}
 
 	default:
-		const auto messageId = *reinterpret_cast<MessageType::World*>(&packet->data[3]);
+		// Need to use memcpy instead of reinterpret_cast to avoid UB
+		auto messageId = MessageType::World::INVALID;
+		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());
 	}

tests/dCommonTests/Amf3Tests.cpp

@@ -71,7 +71,7 @@ TEST(dCommonTests, AMF3InsertionAssociativeTest) {
 	array.Insert<std::vector<uint32_t>>("Undefined", {});
 	array.Insert("Null", nullptr);
 
-	ASSERT_EQ(array.Get<const char*>("CString")->GetValueType(), eAmf::String);
+	ASSERT_EQ(array.Get("CString")->GetValueType(), eAmf::String);
 	ASSERT_EQ(array.Get<std::string>("String")->GetValueType(), eAmf::String);
 	ASSERT_EQ(array.Get<bool>("False")->GetValueType(), eAmf::False);
 	ASSERT_EQ(array.Get<bool>("True")->GetValueType(), eAmf::True);
@@ -95,7 +95,7 @@ TEST(dCommonTests, AMF3InsertionDenseTest) {
 	array.Push<std::vector<uint32_t>>({});
 
 	ASSERT_EQ(array.Get<std::string>(0)->GetValueType(), eAmf::String);
-	ASSERT_EQ(array.Get<const char*>(1)->GetValueType(), eAmf::String);
+	ASSERT_EQ(array.Get(1)->GetValueType(), eAmf::String);
 	ASSERT_EQ(array.Get<bool>(2)->GetValueType(), eAmf::False);
 	ASSERT_EQ(array.Get<bool>(3)->GetValueType(), eAmf::True);
 	ASSERT_EQ(array.Get<int32_t>(4)->GetValueType(), eAmf::Integer);

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