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,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
@@ -272,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")
+	add_compile_options("-Wuninitialized" "-Wold-style-cast" "-Wstrict-aliasing=01")
 else()
 	message(WARNING "Unknown compiler: '${CMAKE_CXX_COMPILER_ID}' - No warning flags enabled.")
 endif()

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}")

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,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<const uint8_t*>(&slice.front());
+	const char* const bytes = slice.data();
 	if (rem > 0) {
-		const uint8_t first = bytes[0];
+		const uint8_t first = static_cast<uint8_t>(bytes[0]);
 		if (first < 0x80) { // 1 byte character
 			out = static_cast<uint32_t>(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<uint8_t>(bytes[1]);
 				if (_IsSuffixChar(second)) {
 					out = (static_cast<uint32_t>(first & 0x1F) << 6)
 						+ static_cast<uint32_t>(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<uint8_t>(bytes[1]);
+				const uint8_t third = static_cast<uint8_t>(bytes[2]);
 				if (_IsSuffixChar(second) && _IsSuffixChar(third)) {
 					out = (static_cast<uint32_t>(first & 0x0F) << 12)
 						+ (static_cast<uint32_t>(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<uint8_t>(bytes[1]);
+				const uint8_t third = static_cast<uint8_t>(bytes[2]);
+				const uint8_t fourth = static_cast<uint8_t>(bytes[3]);
 				if (_IsSuffixChar(second) && _IsSuffixChar(third) && _IsSuffixChar(fourth)) {
 					out = (static_cast<uint32_t>(first & 0x07) << 18)
 						+ (static_cast<uint32_t>(second & 0x3F) << 12)

dCommon/GeneralUtils.h

@@ -3,6 +3,7 @@
 // C++
 #include <charconv>
 #include <cstdint>
+#include <cstring>
 #include <ctime>
 #include <functional>
 #include <optional>
@@ -137,7 +138,7 @@ namespace GeneralUtils {
 	template <typename... Bases>
 	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 <typename To, typename From>
+    [[nodiscard]]
+    inline To FromBitsUnchecked(const From* from) noexcept 
+    requires (std::is_trivially_copyable_v<To>
+              && std::is_trivially_copyable_v<From>)
+    {
+        To to{};
+        std::memcpy(&to, from, sizeof(To));
+        return to;
+    }
+
 	// Concept constraining to enum types
 	template <typename T>
 	concept Enum = std::is_enum_v<T>;

dCommon/dClient/AssetManager.h

@@ -54,7 +54,8 @@
 	}
 
 	operator bool() {
+		// NEED TO FIX THIS
 		return reinterpret_cast<AssetMemoryBuffer*>(rdbuf())->m_Success;
 	}
 };
 

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 @@
 
 		if (data->values[1] != 0) return;
 
-		time_t startTime = std::time(0) + 4; // Offset for starting timer
-
-		data->values[1] = *reinterpret_cast<float*>(&startTime);
+		// 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
+		data->values[1] = std::bit_cast<float>(static_cast<int32_t>(startTime));
 
 		Game::entityManager->SerializeEntity(self);
 	} else if (identifier == u"FootRaceCancel") {
@@ -80,10 +80,10 @@
 			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);
+		const time_t startTime = std::bit_cast<int32_t>(data->values[1]);
+		const time_t finish = endTime - startTime;
+		data->values[2] = std::bit_cast<float>(static_cast<int32_t>(finish));
 
 		auto* missionComponent = sender->GetComponent<MissionComponent>();
 		if (missionComponent != nullptr) {

dWorldServer/WorldServer.cpp

@@ -1396,7 +1396,9 @@ void HandlePacket(Packet* packet) {
 	}
 
 	default:
-		const auto messageId = *reinterpret_cast<MessageType::World*>(&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<MessageType::World>(&packet->data[3]);
+
 		const std::string_view messageIdString = StringifiedEnum::ToString(messageId);
 		LOG("Unknown world packet received: %4i, %s", messageId, messageIdString.data());
 	}